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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chris Johnson’s picture

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.

christefano’s picture

Status: Reviewed & tested by the community » Closed (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.

sumitk’s picture

Status: Closed (duplicate) » Reviewed & tested by the community

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

Aron Novak’s picture

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.

Aron Novak’s picture

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().

lourenzo’s picture

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

wundo’s picture

Issue tags: +dc2009 code sprint

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

walkah’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -dc2009 code sprint
FileSize
5.19 KB

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

sumitk’s picture

Title: Adding patch to support new OpenID Provider Path » Adding patch to support new OpenID Provider Path + admin settings for sreg
FileSize
6.57 KB

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.

alex_b’s picture

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 ?

anarcat’s picture

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?

alex_b’s picture

* 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

anarcat’s picture

Status: Needs work » Needs review
FileSize
6.22 KB

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.

anarcat’s picture

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?

Chris Johnson’s picture

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.

anarcat’s picture

FileSize
6.22 KB

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.

anarcat’s picture

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.

alex_b’s picture

Status: Needs review » Needs work

user/%/openid is used by openid module

anarcat’s picture

Status: Needs work » Needs review

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

alex_b’s picture

Status: Needs review » Needs work
FileSize
8.93 KB

#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.

alex_b’s picture

Status: Needs work » Needs review

did not mean to reset status.

Aron Novak’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
9.32 KB

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

alex_b’s picture

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

Chris Johnson’s picture

@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.

anarcat’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.07 KB

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.

Aron Novak’s picture

#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.

walkah’s picture

@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.

walkah’s picture

Status: Reviewed & tested by the community » Fixed

committed. thanks everyone!

Status: Fixed » Closed (fixed)

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