CentralAuth does not reject a request because CentralAuth does not use hook for "ChangeUserGroups"

Actually, its because central auth will block anything using that uses $title->userCan, but not things that use $wgUser->isAllowed()

In T194605#4203223, @Bawolff wrote:

T194605.patch3 KBDownload

+2

[20:09] bawolff !log Deployed patch for T194605

This should just be considered the emergency patch - Come monday, we need to take a hard look at everything botpasswords, and also how account locking works (Should also use hooks to kill all rights (except possibly read) from $wgUser->isAllowed()).

So outstanding concerns:

• Locking an account should probably remove all the rights (via $wgUser->isAllowed() )
• Unclear if locking an account rotates gu_auth_token
  • Would that even be enough with redis caching, and what about local user_token session.
• Locking does not appear to rotate bp_token, or otherwise cancel ongoing bot sessions.

Given the recent abuse of botpasswords maybe T150526: BotPasswords: grant all rights is not a good idea?

Same considerations probably apply to owner-only OAuth consumers (non-owner-only too, but that's hard to exploit).

More generally, authentication methods like bot passwords and OAuth consumers are handled by SessionManager but their credentials are "self-contained" (unlike sessions which can be reset to force authentication to fall back to AuthManager-handled credentials), and thus SessionManager::invalidateSessionsForUser ignores them.

Also, ideally SessionManager would call User::isLocked but that method probably needs better caching first. (OTOH CheckBlocksSecondaryAuthenticationProvider not calling it either seems like an oversight that's trivial to fix.)

In T194605#4203280, @Bawolff wrote:

and also how account locking works

That's probably the more useful thing to do. It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.

I note that CentralAuth locks don't necessarily try to prevent login to locked accounts, although it does on WMF wikis, at least if CentralAuthUser::canAuthenticate() is anything to go by. If $wgCentralAuthLockedCanEdit is non-empty it doesn't by itself prevent login.

In T194605#4203351, @Bawolff wrote:

So outstanding concerns:

• Locking an account should probably remove all the rights (via $wgUser->isAllowed() )

See above. This does not seem to be what "locking" is currently intended to mean, at least for CentralAuth, since $wgCentralAuthLockedCanEdit exists.

• Unclear if locking an account rotates gu_auth_token
  • Would that even be enough with redis caching, and what about local user_token session.

It appears that it does not.

• Locking does not appear to rotate bp_token, or otherwise cancel ongoing bot sessions.

To cancel ongoing sessions, an extension should call SessionManager::invalidateSessionsForUser().

In T194605#4204401, @Tgr wrote:

More generally, authentication methods like bot passwords and OAuth consumers are handled by SessionManager but their credentials are "self-contained" (unlike sessions which can be reset to force authentication to fall back to AuthManager-handled credentials), and thus SessionManager::invalidateSessionsForUser ignores them.

That "thus" shouldn't be true, and as far as I can tell it isn't since SessionManager itself resets the user's token which will cause SessionManager::loadSessionInfoFromStore() to be unable to load any stored session data.

However, invalidating the existing session does not prevent a new session from being created on the next request.

Also, ideally SessionManager would call User::isLocked but that method probably needs better caching first. (OTOH CheckBlocksSecondaryAuthenticationProvider not calling it either seems like an oversight that's trivial to fix.)

As I mentioned above, it's very unclear what "locking" a user is even supposed to do.

In T194605#4204852, @Anomie wrote:

In T194605#4203280, @Bawolff wrote:

and also how account locking works

That's probably the more useful thing to do. It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.

I note that CentralAuth locks don't necessarily try to prevent login to locked accounts, although it does on WMF wikis, at least if CentralAuthUser::canAuthenticate() is anything to go by. If $wgCentralAuthLockedCanEdit is non-empty it doesn't by itself prevent login.

My understanding is that $wgCentralAuthLockedCanEdit was intended as an appeal feature. You'd be locked out of all other wikis, but you'd be allow to appeal your case on a specific page on e.g. Meta-Wiki. Given that the feature isn't used, I don't think it's worth keeping around...

AIUI the definition of a lock *should* be that it is impossible to log in to the account, and you're unable to perform any actions with it.

AIUI the definition of a lock *should* be that it is impossible to log in to the account, and you're unable to perform any actions with it.

yes, that's exactly what happens. If locked accounts can perform any action right now, this is not how it should work. Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

I think the fact that there is ambiguity around these concepts in our authentication flow is problematic in and of itself.

Im not super attached to lock preventing bot logins if we decide thats not the definition of "lock", but i think its a good immediate term solution well we figure this out.

In T194605#4204852, @Anomie wrote:

It seems rather ill-defined at the moment what "locking" even means and how it's different from "blocking" the user.

Lock is supposedly a "temporary" solution until T17294 is fixed.

cc'ing @TBolliger as this arguably is somewhat related to Anti-Harassment team.

In T194605#4203223, @Bawolff wrote:

T194605.patch3 KBDownload

I ran into merge conflicts when applying this patch to 1.32.0-wmf.4. I think I resolved the conflict in a sane manner, however, I'd like someone else to take a look. @Bawolff: Can you look at the patch as applied on tin with commit hash of rMWf4976d9e4049 and verify that I did't mess it up?

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

In T194605#4253196, @Aklapper wrote:

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

I replied to him on irc

@Bawolff the patch in /srv/patches/1.32.0-wmf.7/core/02-T194605.patch doesn't seem to apply cleanly to the newly cut /srv/mediawiki-staging/php-1.32.0-wmf.7 branch. It looks like there's a difference in the messages used between what's in core and what's in the patch. Could you take a look?

When I try to apply the patch, the differences I see are the patch has botpasswords-locked and wmf.7 has botpasswords-needs-reset. Did a patch for this merge into master recently?

In T194605#4205086, @MarcoAurelio wrote:

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

I got the patch applied like I think makes sense, but could still use verification/sign-off. Patch is on deploy1001 at /srv/patches/1.32.0-wmf.7/core/02-T194605-alternative.patch

In T194605#4258878, @thcipriani wrote:

Did a patch for this merge into master recently?

https://github.com/wikimedia/mediawiki/commit/ff6b4cb35c1944870fcd3cc525884790c20819b3

In T194605#4253881, @Bawolff wrote:

In T194605#4253196, @Aklapper wrote:

@Bawolff: Could you reply to mmodell's last comment, please? TIA!

I replied to him on irc

In the future please reply on task instead of breaking the communication across multiple mediums.

In T194605#4259027, @demon wrote:

In T194605#4205086, @MarcoAurelio wrote:

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

Mainly due to the lack of global blocking for accounts. Over the years, some elements of locking have grown to be more of a feature than a bug. For example, it's useful when an account is compromised to prevent the attacker from changing any personal settings once the account is locked. If global blocking was extended to accounts, it would significantly change the workflow and need for this feature.

In T194605#4259027, @demon wrote:

In T194605#4205086, @MarcoAurelio wrote:

Locked means account disabled, fully; not even able to log-in (and session terminated if locked while logged in fwiw).

Why on earth do we even bother with such a distinction?

I don't understand what do you mean. It is not a distinction but a CentralAuth feature, and the only way as of today to globally disable an account. As @Ajraddatz said, GlobalBlocking cannot be used for accounts.

(Which, again, is a known bug: T17294.)

There's current 2 patches 02-T194605.patch and 02-T194605-alternate.patch on deploy1001

Should we delete one of them?

Any updates? Thanks.

Also, there are two restricted tasks linked here. Could I be added to them if that's okay? (if not, I understand).

In T194605#4471617, @MarcoAurelio wrote:

Also, there are two restricted tasks linked here. Could I be added to them if that's okay? (if not, I understand).

I only see one (T194204) and you are already added to that.

@Tgr I mean, this task has two parents. Both of them appear to me as Restricted Task.

@MarcoAurelio These are both tracking tasks for what needs to be in future security releases, so nothing specifically relevant for this.