Access rules for realms

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

I think this should be a system-wide policy set by admins... patches welcome :-)

Admins' global setting.

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.

Thanks for the guide. So would you please do the English for me. :)

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.

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.

Update: check for blacklist before asking for login.

Seeking #1217826: Skip openid_provider_form and I though this is quite relate. So I add more option for auto-login.

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

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.

Here's what I had in mind... Surely there are other, more optimal solutions...

+++ 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:

Users should opt-in to allowing checkid_immediate for each RP that they want to automatically sign into. OPs should NOT enable checkid_immediate for RPs that the user had not previously signed into.

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

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.

Good idea!

Time for tester, I though. :)

Hi,

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

Patch from #16 works excellent, and this patch attached is that one with the suggestion from #17.

Noticed a small Drupal coding standard issue, and fixed it.

Great 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!

This appears to be working for me. Thanks for the hard work everybody.

please mark status accordingly!

Merged into 6.x and 7.x, thanks to all and sorry for the delay.