Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Add black-, whitelist functionality, e.g. as on ?q=admin/user/rules, or a simple textarea with new line seperated items.
See the specs for REALM [1].
Desired behaviour:
- Allow all, deny all setting
- Add exceptions
- Return error on deny [2]
[1] http://openid.net/specs/openid-authentication-2_0.html#rfc.section.9.2
[2] http://openid.net/specs/openid-authentication-2_0.html#rfc.section.5.2.3
Comment | File | Size | Author |
---|---|---|---|
#19 | openid_provider-314781-19-access-rules-realms.patch | 6.27 KB | kotnik |
#18 | openid_provider-314781-18-access-rules-realms.patch | 6.26 KB | kotnik |
#16 | whitelist-314781-16.patch | 5.75 KB | neizod |
#12 | whitelist-314781-12.patch | 5.69 KB | paranojik |
#11 | whitelist-314781-11.patch | 4.33 KB | neizod |
Comments
Comment #1
anarcat CreditAttribution: anarcat commentedSome work regarding this feature has been done in #396508: Make user/N/openid-sites themable and default to table display, mainly the ability to deny access to sites already visited.
It should be fairly trivial to add to that patch to some discretionary "deny" items. Now the default policy is "ask" (defined in
openid_provider_authentication_response
and_openid_provider_rp_save
), maybe that should be made a per user setting?So I guess this feature should focus on the per-user (or system-wide!) policy settings feature.
Comment #2
walkah CreditAttribution: walkah commentedI think this should be a system-wide policy set by admins... patches welcome :-)
Comment #3
neizod CreditAttribution: neizod commentedAdmins' global setting.
Comment #4
anarcat CreditAttribution: anarcat commentedThanks for the patch!
First: please mark an issue "needs review" when you submit a patch.
Second, there are some issues with the spelling and wording:
login -> logged in?
"from this site"..
this description isn't quite clear, and should be rephrased.
typos: "bypass" and "cancel". also extra whitespace at the end of second line ;)
if you have trouble with english, I can probably take the patch as-is and rephrase as I see fit, i don't mind much.
let me know, and thanks again.
Comment #5
neizod CreditAttribution: neizod commentedThanks for the guide. So would you please do the English for me. :)
Comment #6
anarcat CreditAttribution: anarcat commentedAlright, so here's the proper phrasing:
I also throw in a description for the blacklist:
And now looking at the code, it seems to me that the check for the whitelist is not in the right place. It shouldn't be done when the form is rendered, it should be done even before that form is rendered. I *think* this belongs more in
openid_provider_authentication_response(), in openid_provider.inc.
Because it's in the form, I am not sure the blacklist would be effective. I also suspect that function wouldn't be enough, I am thinking of openid_provider_unsolicited_assertion(), for example. Look for other http requests or gotos in the code - if we make a blacklist we need to block everything.
Comment #7
neizod CreditAttribution: neizod commentedSince I don't know how to print param, I put my code in the form section, sorry.
I did't get idea how openid_provider_unsolicited_assertion() work either. The blacklist's check still belong with the whitelist's, and user must be logged in before the knew they are rejected by the blacklist. :( I'm going to work furthermore on this.
Comment #8
neizod CreditAttribution: neizod commentedUpdate: check for blacklist before asking for login.
Comment #9
neizod CreditAttribution: neizod commentedSeeking #1217826: Skip openid_provider_form and I though this is quite relate. So I add more option for auto-login.
Comment #10
paranojik CreditAttribution: paranojik commentedI took a look at the code, but didn't actually test it. These are my suggestions:
I would save this variables as arrays, so this processing would't be needed for every request.
Why don't you use drupal_goto() here?
I think this should be spelled "Anonymous sites".
Same here: "Disable anonymous sites".
What do you think about this phrasing: "Sites on this list can be logged in through the OpenID provider. Enter one site per line with the full URL e.g. http://www.example.com/."
Correct me if I'm wrong, but I'd write this: "...will be considered blacklisted.".
Comment #11
neizod CreditAttribution: neizod commentedThanks. I've follow your suggestion except the 1st one. I'd love to fix that but I don't know how to properly prepare array for the textarea. I've try it a little bit and found out the code is too heavy.
Comment #12
paranojik CreditAttribution: paranojik commentedHere's what I had in mind... Surely there are other, more optimal solutions...
Comment #13
anarcat CreditAttribution: anarcat commentedI think this should have a description saying how setting this violates the standard. As mentionned in #1161698: allow users to blacklist certain relying parties:
... although this is a "SHOULD" not "MUST". ;) Still, a little "description" wouldn't hurt here. :)
Make the $sites_auto_release == 'whitelist' check first to avoid an array lookup with big whitelists.
Please rephrase the last sentence to "The blacklist has precendence over the whitelist, that is: sites also on the whitelist will be considered blacklisted."
Comment #14
neizod CreditAttribution: neizod commentedWhat about this radio option and description?
Description: Select "Whitelist only" to allow trusted partner sites logged in automatically. Select "Anonymous sites" is unrecommended due to its violating the standard for user to be asked before logged in into unregistered sites.
Comment #15
anarcat CreditAttribution: anarcat commentedGood idea!
Comment #16
neizod CreditAttribution: neizod commentedTime for tester, I though. :)
Comment #17
anarcat CreditAttribution: anarcat commentedHi,
Code looks good! However, after my recent review of the standard, I have noticed that the way the authentication is refused is incorrect. I believe we need to send a proper unsuccessful response, as detailed in association session response.
It's nothing complicated, replace the drupal_goto and an openid_redirect_http() with a proper response parameter. look further down the function for an example.
sorry for bouncing back on you like this. :)
Comment #18
kotnik CreditAttribution: kotnik commentedPatch from #16 works excellent, and this patch attached is that one with the suggestion from #17.
Comment #19
kotnik CreditAttribution: kotnik commentedNoticed a small Drupal coding standard issue, and fixed it.
Comment #20
anarcat CreditAttribution: anarcat commentedGreat work, thanks!!
@paranojik, @neizod: can we get a second tester to confirm everything is gold here? I'd like to make this part of the final 1.0 release. :)
Thanks for the hard work everyone, awaiting the final RTBC!
Comment #21
Ross-Hunter CreditAttribution: Ross-Hunter commentedThis appears to be working for me. Thanks for the hard work everybody.
Comment #22
anarcat CreditAttribution: anarcat commentedplease mark status accordingly!
Comment #23
anarcat CreditAttribution: anarcat commentedMerged into 6.x and 7.x, thanks to all and sorry for the delay.