Posted by sanduhrs on September 29, 2008 at 11:00am
| Project: | OpenID Provider |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
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
Comments
#1
Some 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_responseand_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.
#2
I think this should be a system-wide policy set by admins... patches welcome :-)
#3
Admins' global setting.
#4
Thanks 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:
+++ b/openid_provider.moduleundefined@@ -185,6 +185,29 @@ function openid_provider_admin_settings() {
+ '#description' => t('Site on this list will be automatic login using OpenID from this site without confirmation page. Please fill in full site name e.g. <i>http://www.example.com/</i>'),
login -> logged in?
"from this site"..
this description isn't quite clear, and should be rephrased.
+++ b/openid_provider.moduleundefined@@ -243,6 +266,19 @@ function openid_provider_form(&$form_state, $response = array(), $realm = NULL)
+ // If realm on whitelist, by pass the confirm page and continue login.
+ // If realm on blacklist, or whitelist_only, cancle the login request. ¶
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.
#5
Thanks for the guide. So would you please do the English for me. :)
#6
Alright, so here's the proper phrasing:
Sites on this list will be automaticcally logged in through the OpenID provider without confirmation page. Please fill in full site name e.g. <em>http://www.example.com/</em>Only allow sites on the whitelist to login using the OpenID provider.I also throw in a description for the blacklist:
Sites on this list will be completely forbidden to login through the OpenID provider.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.
#7
Since 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.
#8
Update: check for blacklist before asking for login.
#9
Seeking #1217826: Skip openid_provider_form and I though this is quite relate. So I add more option for auto-login.
#10
I took a look at the code, but didn't actually test it. These are my suggestions:
+ $whitelist = explode("\r\n", variable_get('openid_provider_whitelist', ""));+ $blacklist = explode("\r\n", variable_get('openid_provider_blacklist', ""));
I would save this variables as arrays, so this processing would't be needed for every request.
+ header('Location: '. $request['openid.return_to'], TRUE, 302);Why don't you use drupal_goto() here?
+ 'all' => 'Anoynemouse sites',I think this should be spelled "Anonymous sites".
+ '#title' => t('Disable anonymouse sites'),Same here: "Disable anonymous sites".
+ '#description' => t('Sites on this list can be logged in through the OpenID provider without confirmation page. Please fill in full site name e.g. <em>http://www.example.com/</em> and separate each site with newline.'),+ );
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/."
+ '#description' => t('Sites on this list will be completely forbidden to login through the OpenID provider. Duplicated sites on whitelist will be consider a blacklist.'),Correct me if I'm wrong, but I'd write this: "...will be considered blacklisted.".
#11
Thanks. 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.
#12
Here's what I had in mind... Surely there are other, more optimal solutions...
#13
+++ b/openid_provider.moduleundefined@@ -185,10 +185,62 @@ function openid_provider_admin_settings() {
+ $form['sitelist']['openid_provider_sites_auto_release'] = array(
+ '#type' => 'radios',
+ '#title' => t('Automatically login without confirmation page'),
+ '#options' => array(
+ 'all' => 'Anonymous sites',
+ 'whitelist' => 'Whitelist only',
+ 'none' => 'Always ask',
+ ),
+ '#default_value' => variable_get('openid_provider_sites_auto_release', 'none'),
+ );
I 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. :)
+++ b/openid_provider.incundefined@@ -157,7 +167,8 @@ function openid_provider_authentication_response($request) {
+ $sites_auto_release = variable_get('openid_provider_sites_auto_release', 'none');
+ if ($rp->auto_release || ($sites_auto_release == 'all') || (in_array($realm, $whitelist) && ($sites_auto_release == 'whitelist'))) {
$response = _openid_provider_sign($response);
_openid_provider_rp_save($user->uid, $realm, TRUE);
_openid_provider_debug('automatic response authentication success using redirect to %url (request dump:
Make the $sites_auto_release == 'whitelist' check first to avoid an array lookup with big whitelists.
+++ b/openid_provider.moduleundefined@@ -185,10 +185,62 @@ function openid_provider_admin_settings() {
+ '#default_value' => @implode(PHP_EOL, variable_get('openid_provider_blacklist', array())),
+ '#description' => t('Sites on this list will be completely forbidden to login through the OpenID provider. Enter one site per line with the full URL e.g. <em>http://www.example.com/</em>. Duplicated sites on whitelist will be considered blacklisted.'),
+ );
return system_settings_form($form);
}
Please rephrase the last sentence to "The blacklist has precendence over the whitelist, that is: sites also on the whitelist will be considered blacklisted."
#14
What about this radio option and description?
'#options' => array('all' => 'Anonymous sites (unrecommended)',
'whitelist' => 'Whitelist only',
'none' => 'Always ask',
),
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.
#15
Good idea!
#16
Time for tester, I though. :)