I was looking at the code for OpenID, and noticed that during user authentication, user_login_name_validate() is called with an incorrect set of parameters, which seems like it will lead blocked users to not be blocked if they log in via OpenID.
This is presumably a security issue; however, it's D7 only so can be a public issue. The equivalent code in D6 is in http://api.drupal.org/api/function/user_external_login/6 and appears to work correctly (although it has a minor bug where it passes along an extra unneeded parameter).
Patch coming in a second.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | openid-registration-checking.patch | 1.31 KB | gábor hojtsy |
| #4 | openid-blocked-users-3.patch | 3.6 KB | c960657 |
| #3 | openid_blocked_users-2.patch | 3.73 KB | c960657 |
| #1 | openid_blocked_users.patch | 857 bytes | David_Rothstein |
Comments
Comment #1
David_Rothstein commentedHere is the patch.
The code in this area seems somewhat fragile, so it could probably use a test, although that might require a fair bit of rewriting. Sounds like something to deal with after the code freeze :)
Comment #2
brianV commentedSeems correct to me, but definately needs tests. Tagging.
Comment #3
c960657 commentedAdded a test.
Comment #4
c960657 commentedReroll.
Comment #5
brianV commentedPerfect!
Comment #6
webchickOuch! That's a nasty little bug.
Committed to HEAD, thanks! :)
Comment #8
gábor hojtsySame bug appears in the initial registration too. D6 is not affected, since it uses user_external_login() which will not log the user in, if she is blocked. However, D7 just plain logs in the user regardless of user status. Bad.
This suggested patch adds an explicit check for user status right before we attempt to log the user in (but after we saved the OpenID association). If the newly registered user was blocked, we inform her that the account is awaiting admin approval.
Comment #9
gábor hojtsyLooks like the refactoring at #395340: Email verification not enforced with OpenID auto-registration solves this issue as well, but let's ensure that its solved, cause otherwise this is a security issue (therefore critical).
Comment #10
dries commentedAsking for a retest because #395340: Email verification not enforced with OpenID auto-registration was committed.
Comment #11
gábor hojtsyWell, it will not even apply anymore then. I think we need to retest this manually, since the process is very different now for registrations. We might also want to extend the tests for registrations to test for this condition. Anyway, we need to verify that this is not an issue anymore with the updated code.
Comment #12
gábor hojtsyI've verified on a test install site, and #395340: Email verification not enforced with OpenID auto-registration did indeed solve this problem as well. We can consider this fixed as far as I see.