Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
user/x/openid_sites should be a table with
Site|Always approve|Last access
rather than the current list with unlabeled check boxes.
http://img.skitch.com/20090309-x68fsx3f1bfk8c7dx87pa696ni.preview.jpg
Comment | File | Size | Author |
---|---|---|---|
#32 | 396508_access.patch | 13.22 KB | anarcat |
#27 | 396508_openid_provider_sites_page.patch | 12.22 KB | anarcat |
#23 | 396508_openid_provider_sites_page.patch | 11.83 KB | Aron Novak |
#15 | openid_provider_sites_page.patch | 11.41 KB | anarcat |
#14 | openid_provider_sites_page.patch | 10.99 KB | anarcat |
Comments
Comment #1
alex_b CreditAttribution: alex_b commentedBigger picture http://img.skitch.com/20090309-x68fsx3f1bfk8c7dx87pa696ni.jpg
Comment #2
Aron NovakHere is the patch which implements this style of openid_sites listing and UI.
Comment #3
Chris Johnson CreditAttribution: Chris Johnson commentedNice improvement!
Comment #4
Gábor HojtsyMarked http://drupal.org/node/395010 as duplicate.
Comment #5
anarcat CreditAttribution: anarcat commentedpatch doesn't apply cleanly.
Comment #6
anarcat CreditAttribution: anarcat commentedThis 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.Comment #7
anarcat CreditAttribution: anarcat commentedSo 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.
Comment #8
anarcat CreditAttribution: anarcat commentedI 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.
Comment #9
anarcat CreditAttribution: anarcat commentedSmall tweak: remove debugging code.
Comment #10
anarcat CreditAttribution: anarcat commentedI tested this on our provider against the D6 openid.module and it works well.
Comment #11
anarcat CreditAttribution: anarcat commentedOne last try: this one fixes the documentation in the form.
Comment #12
anarcat CreditAttribution: anarcat commentedI'm reworking this.
Comment #13
anarcat CreditAttribution: anarcat commentedI 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.
Comment #14
anarcat CreditAttribution: anarcat commentedTricky 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.
Comment #15
anarcat CreditAttribution: anarcat commented'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.
Comment #16
mitchell CreditAttribution: mitchell commentedI tried #14 and was surprised that it stopped working. #15 works now when testing at http://openidenabled.com/resources/openid-test/checkup
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.
Comment #17
anarcat CreditAttribution: anarcat commentedSo 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?
Comment #18
Aron NovakI tested the patch and it works reliably.
When you try to test it, do a reinstall to make the schema up-to-date.
Comment #19
anarcat CreditAttribution: anarcat commentedActually, 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.
Comment #20
Gábor HojtsySetting the OpenID module up on drupal.org is a cornerstone of the redesign efforts. So making the server interface usable would be pretty important.
Comment #21
anarcat CreditAttribution: anarcat commentedActually, 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.
Comment #22
Aron NovakI can confirm, i could reliably reproduce openid login issues. I get OpenID login failed messages all the time after applying the patch.
Comment #23
Aron NovakI could reproduce the problem described in #21, so I fixed the issue.
The problem was here:
(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.
Comment #24
ceardach CreditAttribution: ceardach commentedI'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.
Comment #25
anarcat CreditAttribution: anarcat commentedAt 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...
Comment #26
anarcat CreditAttribution: anarcat commentedOf 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.
Comment #27
anarcat CreditAttribution: anarcat commentedI'm daring to repost this as RTBC, as it's a pretty straightforward port of the patch to HEAD. Thank god for git...
Comment #28
walkah CreditAttribution: walkah commentedthis patch fails for me on HEAD ... but I'd like to gt this in... having a look.
Comment #29
walkah CreditAttribution: walkah commentedOK, 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).
Comment #30
walkah CreditAttribution: walkah commentedCommitted changes as per #29 ... again, if someone can make a strong case for the 3 options ... then let's start a new issue.
Comment #32
anarcat CreditAttribution: anarcat commentedSorry 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..
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... ;)
Comment #33
anarcat CreditAttribution: anarcat commentedThe above patch introduces a regression on beta2 - everybody gets constantly asked to reconfirm their choices (although only for certain sites). Odd.
Comment #34
anarcat CreditAttribution: anarcat commentedi'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.