Adding patch to support new OpenID Provider Path

darren.ferguson - October 17, 2008 - 19:02
Project:OpenID Provider
Version:6.x-1.x-dev
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:closed
Description

Patch for supporting the new path in OpenID for one that does not have the access denied message coming back if the user is not logged into the site initially.

AttachmentSize
openid_provider.module.patch1.79 KB

#1

Chris Johnson - October 28, 2008 - 21:04
Status:active» reviewed & tested by the community

This patch, or the patch in http://drupal.org/node/322767 is required for the OpenID provider to even really function.

#2

christefano - November 2, 2008 - 09:08
Status:reviewed & tested by the community» duplicate

I think this is a duplicate of #322767 since this patch is required by (and already included in) the patch in that issue. This can be reopened if necessary.

#3

sumitk - February 6, 2009 - 19:21
Status:duplicate» reviewed & tested by the community

no its not duplicate of #322767 you need to apply this one too

#4

Aron Novak - February 27, 2009 - 09:36

I tested the stuff and it seems #322767 is not really neccessary.
As i see, to make the openid_provider work you need to apply this patch (322764) and #322769.
However while I tried to login to another Drupal site, i received a message that there are invaild characters in the username, i had to choose another one. I assume this is not a critical issue.

#5

Aron Novak - February 27, 2009 - 22:28

Better testing, this patch is not critical, openid_provider works fine without it. However it's important to tune access privileges correctly. See openid_provider_requirements().

#6

lourenzo - March 7, 2009 - 17:30

I made a test with this patch, and works for me. I've got the invalid chars when testing with another drupal, too.

#7

wundo - March 7, 2009 - 18:18

The invalid chars is another issue (not related with this patch), the guys are working on that other issue, right now.

#8

walkah - March 7, 2009 - 19:00
Status:reviewed & tested by the community» needs review

I've changed this patch slightly to change the format of the URL to be user/%/openidurl - this removes the requirement to have a patch for XRDS-simple module.

The patch also includes removing the hook_requirements implementation in .install

AttachmentSize
openid-provider.diff 5.19 KB

#9

sumitk - March 7, 2009 - 19:35
Title:Adding patch to support new OpenID Provider Path» Adding patch to support new OpenID Provider Path + admin settings for sreg

Above patch applied successfully. I added admin settings for sreg request so I am combining both patches and pasting a newer one to test.

Explaining a bit more earlier using openid_provider does not include your username and email in openid call (which was already present in core openid module) so we added those 2 parameters (niackname and email from $user object) with some admin settings.

AttachmentSize
openid-admin.diff 6.57 KB

#10

alex_b - March 7, 2009 - 19:37
Title:Adding patch to support new OpenID Provider Path + admin settings for sreg» Adding patch to support new OpenID Provider Path

Just reviewed and tested. Looks good.

Not fond of 'openidurl' -

user/%/openid/identity user/%/openid/openid-identity ?

#11

anarcat - March 7, 2009 - 20:28
Status:needs review» needs work

The patch works and i think can go in.

I am not fond of "openidurl" either at all though, so I'm marking "needs work".

In building our provider here, we are wanting something really user-friendly, something like http://id.example.com/username

Should I build a patch with that instead?

#12

alex_b - March 7, 2009 - 20:37

* Changed the default openid provider path to user/%/identity (as discussed at code sprint between walkah, sumitka and alex_b)
* Added pathauto support so that provider path can be aliased automatically

AttachmentSize
322764_openid_provider_path.patch 8.26 KB

#13

anarcat - March 7, 2009 - 21:08
Status:needs work» needs review

So here's a patch that further improves on the path. I enclosed the path generation in a function so we can play around (and maybe add hooks?) more easily with the URL. I chose user/<uid>/id to keep it simple.

I thought of making the pattern a variable (variable_get('openid_provider_url') + associated changes to the settings) but that doesn't work well since the form settings submission would need to be changed to regen the menus when that value changes, which maybe possible.

I feel, however, that url aliases or thing like that would make this cleaner.

AttachmentSize
openid_url.diff 6.22 KB

#14

anarcat - March 7, 2009 - 21:10

I actually question the validity of this approach altogether: shouldn't we just allow anonymous users to access user profiles? Isn't this what OpenID is about in the first place?

#15

Chris Johnson - March 7, 2009 - 21:26

I'm imagining a case where a site has extensive user profiles which contain a lot more information than they provide in their OpenIDs. Hence, the data in the OpenID (name, email, ID, maybe a few others) is public, but the rest of the profile (expertise, cell phone number, address, etc.) is private. Each field in the profile has its own privacy settings. I want to avoid having OpenID bypass those privacy settings.

#16

anarcat - March 7, 2009 - 22:10

Very simple update: change the canonical url to user/%s/openid.

@Chris Johnson : agreed. This is different from user/%s. I don't see the controls you're mentionning in place, however... (?)

Note that I resolved my "cleaner URL" issue by adding Pathauto support into the module, see #394690: pathauto support, which depends on this patch here.

AttachmentSize
openid_url.diff 6.22 KB

#17

anarcat - March 7, 2009 - 22:43

The above patch forgot one place for the URL change, in the .inc file. I have fixed that and factored out the URL generation in a function so it's easier to change it module-wide.

The patch otherwise works very well and I'm able to use http://id.koumbit.net/anarcat to connect to remote sites.

A bit annoying to see we had some major work duplication here, I am closing #394690: pathauto support.

AttachmentSize
322764_openid_provider_path+func.patch 8.95 KB

#18

alex_b - March 9, 2009 - 12:10
Status:needs review» needs work

user/%/openid is used by openid module

#19

anarcat - March 9, 2009 - 15:41
Status:needs work» needs review

The patch uses the recommended user/%s/identity.

#20

alex_b - March 9, 2009 - 18:31
Status:needs review» needs work

#19: thanks for clarification.

* Rerolled after #394996: Initial cleanup of module code went in.
* Adjusted output on hook_user('view') to be more user module like http://skitch.com/alexbarth/b8ppm/admin-provider

This is RTBC from my point of view.

AttachmentSize
322764_openid_provider_path+func.patch 8.93 KB

#21

alex_b - March 9, 2009 - 18:32
Status:needs work» needs review

did not mean to reset status.

#22

Aron Novak - March 9, 2009 - 20:07
Status:needs review» reviewed & tested by the community

Here is a version what does not interfere with sites without pathauto.
Now it's RTBC.

AttachmentSize
322764_openid_provider_path+func1.patch 9.32 KB

#23

alex_b - March 9, 2009 - 20:14
Status:reviewed & tested by the community» needs work

#15/#16:

I just realize that in certain use cases you want to have the user profile page as the provider path. Think about rich user profiles that are meant to be passed around - much as I can bling out my OpenID page here http://lxbarth.myopenid.com/.

Unfortunately, you can't use pathauto to alias the identity path so that it points to the user profile instead of the openid page. If you try to enter the same pattern as the user profile pattern, you will get still a different path (suffixed by -0, -1, etc.).

When you think about it, you should only use the user/%id/identity page if you /can't/ access the user/%id page.

Solution:

* The path user/%id/identity should be provided in any case.
* user/%id should be the default path offered as openid identity path on user/%id
* user/%id/identity should be the fallback if anonymous users don't have 'access user profiles' permissions
* Add appropriate help text

#24

Chris Johnson - March 10, 2009 - 17:34

@anarcat: sorry for the delay in following up on your question. I got stranded in the Chicago airport for about 18 hours.

The per-field permission controls are provided by contributed modules such as in the user_relationships module.

#25

anarcat - March 11, 2009 - 03:47
Status:needs work» reviewed & tested by the community

I agree with #23, but still thinks the code is RTBC. This is blocking and pretty annoying, and I'm not sure there's a good way to workaround the issue mentionned.

I've rerolled the patch since it didn't apply cleanly here for some reason.

AttachmentSize
322764_openid_provider_path+func2.patch 9.07 KB

#26

Aron Novak - March 11, 2009 - 12:47

#23:
* user/%id/identity should be the fallback if anonymous users don't have 'access user profiles' permissions

I see one serious problem here.
Maybe openid experts will correct me, but if the provider path changes, clients will believe that this is a different user (result: user already exists for example at Drupal login). So if the permissions are changed, it causes headache to the users.

#27

walkah - March 11, 2009 - 18:04

@Aron - yes, I shared this with anarcat last night - changing identity URLs will break things for people who have used their identity will no longer be able to login if their identity changes.

I think we should hold off on this for now.

the patch in #25 currently doesn't cleanly apply, but i'm going to clean it up and commit.

#28

walkah - March 11, 2009 - 18:18
Status:reviewed & tested by the community» fixed

committed. thanks everyone!

#29

System Message - March 25, 2009 - 18:20
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

 
 

Drupal is a registered trademark of Dries Buytaert.