Make user/N/openid-sites themable and default to table display

Here is the patch which implements this style of openid_sites listing and UI.

Nice improvement!

Marked http://drupal.org/node/395010 as duplicate.

patch doesn't apply cleanly.

$ patch < openid_provider_sites_page.patch 
patching file openid_provider.module
Hunk #1 succeeded at 62 with fuzz 2 (offset 6 lines).
patching file openid_provider.pages.inc
Hunk #1 succeeded at 79 (offset -2 lines).
Hunk #2 FAILED at 116.
1 out of 2 hunks FAILED -- saving rejects to file openid_provider.pages.inc.rej

This patch applies cleanly on head. But it seems to reverse the way the checkbox behave. The checkboxes are severly broken: they don't persist and don't seem to do anything.

So I have improved the patch so that it actually works. I have also made the links clickable and improved the help by removing the negation in the text.

The original patch also removed the "No sites yet" behavior when the openid hasn't been used, that is not yet fixed and the form is likely to look odd when the openid_provider hasn't been used.

I have built another patch which adds the "Deny" feature. It turns the checkboxes into radios and allows the user to deny access to some providers altogether.

Small tweak: remove debugging code.

I tested this on our provider against the D6 openid.module and it works well.

One last try: this one fixes the documentation in the form.

I'm reworking this.

I think this is really my final jab at it now. I rerolled the patch so it applies cleanly again, cleaned up a little bit of code, fixed the way the form was handled, and clarified the options presented to the user when prompted.

Tricky one: i forgot that the original behavior was to ask the user, I made sure this was still the case in this (hopefully) last patch.

'sigh... So the patch was making it so that by default, the behaviour was to deny access (since '' = 0 in PHP), so I have fixed the default.

I also included some code to add watchdog entries when allowing access to the openid_provider.

I tried #14 and was surprised that it stopped working. #15 works now when testing at http://openidenabled.com/resources/openid-test/checkup

Success Failure Incomplete
Associate (DH-SHA1 session) 1 0 0 Try again?
Successful checkid_setup 1 1 0 Try again?

The new interface also works. The only problem was that I had the setting on Ask, and then at the prompt said always allow, and even though it then always allowed, the interface was still set on Ask.

So this is weird. I was able to reproduce the issue you mentionned only once: one of my relaying parties was set to 'ask' but wasn't asking. I set it to 'allow', saved the page, set it to 'ask', and saved the page again and then it asked.

All the other RPs didn't behave this way.

This is rather weird, i don't think there's an issue with the code... and now i can't reproduce that issue..

Anyone else can provide a reliable test case for that issue?

I tested the patch and it works reliably.
When you try to test it, do a reinstall to make the schema up-to-date.

Actually, just running update.php should be enough.

Also, I must say I have witness some /instability/ regarding website approval. I could swear I have repeatedly approved identi.ca to always have access and it's *sometimes* prompting me, I'm not sure I understand why or if this behavior was present before the patch.

Setting the OpenID module up on drupal.org is a cornerstone of the redesign efforts. So making the server interface usable would be pretty important.

Actually, I'll reset this to 'needs work' because the issue i confirmed earlier is still there and very annoying. Basically, it's a regression: you always get prompted to accept the consumer even if you choose 'yes, always'.

Very annoying bug, we need to fix this.

I can confirm, i could reliably reproduce openid login issues. I get OpenID login failed messages all the time after applying the patch.

I could reproduce the problem described in #21, so I fixed the issue.
The problem was here:

_openid_provider_rp_save($user->uid, $realm, TRUE); 

(line 150)
TRUE is not appropiate here, because it can be multiple values. I replaced it to $rp->access and it works fine now.
Also I removed watchdog calls, I assume those messages mostly useful in a debugging environment.

Marked http://drupal.org/node/395010 as duplicate.

[snip] ... For example, would it make sense to be able to delete items from here? So that if for example you just test stuff out, it is not gonna litter your list of sites? Or should we just keep to the setting for automated assignments?

I'd like to see the question of whether or not a user can remove an entry addressed again. I looked through the code and didn't see an option for removing entries.

At last! I had time to test this patch!

And I am very happy to announce that IT WORKS! Darn, it's been soooo long :)

Please commit this... It's a much needed cleanup. I am now maintaining a git branch named "production" of this module that will feature this patch and other necessary improvements. All the gory details at:

http://git.koumbit.net/?p=drupal/contrib/modules/openid_provider.git;a=s...

Of course now this needs to be ported because of the fix applied in #621956: Redirecting fails when not logged in on the provider site... I'll see if I can reroll from git.

I'm daring to repost this as RTBC, as it's a pretty straightforward port of the patch to HEAD. Thank god for git...

this patch fails for me on HEAD ... but I'd like to gt this in... having a look.

OK, my thoughts on this patch:

* I am absolutely +1 making the openid_provider_form themable - and I like the table layout.
* "Deny always" doesn't actually make any sense. checkid_immediate is the only non-interactive mode for openid - and it fails if auto_release isn't set. If a user wanted to always deny access to a certain RP - they wouldn't keep trying to authenticate it, right? If the goal is to blacklist - I think that's better done by an administrator.
* The 3 states confuse and needlessly complicate the code *and* the user interface.

I'm renaming this to just be the themable + table bit. If folks are really passionate about the three mode, we can duke it out in a new issue.

(commit forthcoming with reduced scope).

Committed changes as per #29 ... again, if someone can make a strong case for the 3 options ... then let's start a new issue.

Sorry for the very late reply, but it's only since beta2 that I had to merge in the new code and test the latest commits..

* "Deny always" doesn't actually make any sense. checkid_immediate is the only non-interactive mode for openid - and it fails if auto_release isn't set. If a user wanted to always deny access to a certain RP - they wouldn't keep trying to authenticate it, right? If the goal is to blacklist - I think that's better done by an administrator.

I disagree here: someone may be impersonating the user and trying to access the openid provider, or be coerced of doing so from a malicious site. The user may be aware of that fact and may want to block some of those sites, which would also alleviate the job of the administrator. (Is there a global blacklist anyways? I don't think so...)

The fact that the code is more complicated is not a big issue for me, we're talking about a 300 line patch (unified), and that actually clarifies the "auto-release" flag (which I still don't understand the meaning after all this time): "access" is clearer for me.

I agree the UI is a bit more complicated, but it's understandable and anyways it's not an area a lot of users will visit. Improvements can be done to have a nicer interface (something like icons saying (X) (?) (./) (a checkmark) would be nicer, for example).

I am daring to resubmit a patch to improve the flexibility of this area of the module. I sure hope it will be considered because otherwise I will have to migrate back my database to the original version of the code, which could be complicated... ;)

The above patch introduces a regression on beta2 - everybody gets constantly asked to reconfirm their choices (although only for certain sites). Odd.

i'm going to close this as fixed now as the original issue has been resolved. I still think we want to allow users to opt-out once they have chosen auto-release, but that's a different issue: #1161698: allow users to blacklist certain relying parties.