Page MenuHomePhabricator
Create Task
Maniphest T404257

Add a way to lazy load OATHUser or provide a cheap "user has 2FA enabled" check
Closed, ResolvedPublic
Actions

Description

Per @Catrope's CR in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OATHAuth/+/1187077/1/src/Hook/HookHandler.php as a followup to T404252: OATHAuth loading more from the database than needed...

Note that if OATHRequiredForGroups is non-empty (as it is in production), this function call will lead to an oathUser lookup still happening for users who are in those groups. So this change won't completely eliminate oathUser lookups on random unrelated page views, but it will limit them to users who are members of sensitive groups. If we wanted to fix that, we'd probably need more lazy-loading in OATHUser, or a more efficient way of looking up whether the current user has 2FA enabled at all without loading the details of all their 2FA keys.

We should either add a way a lazy load to OATHUser, and/or a way to just check if the user has 2FA enabled (1 or more rows in oad_devices as of writing), rather than constructing a full OATHUser object, which, for example, when T145915: OATHAuth OTP shouldn't be stored in cleartext in the DB is deployed, could result in unnecessary decryption of data from the database when it's not required

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
OATHUserRepository: Cache 2FA status in WANObjectCachemediawiki/extensions/OATHAuthmaster+58 -27
OATHUserRepository: Fixup userHas2FAEnabled descriptionmediawiki/extensions/OATHAuthREL1_46+9 -3
OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabledmediawiki/extensions/OATHAuthREL1_46+48 -0
OATHUserRepository: Fixup userHas2FAEnabled descriptionmediawiki/extensions/OATHAuthmaster+9 -3
OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabledmediawiki/extensions/OATHAuthmaster+48 -0
zuul: Add OATHAuth as a dependancy for EmailAuthintegration/configmaster+2 -0
Customize query in gerrit

Related Objects

Event Timeline

Reedy created this task.Sep 10 2025, 7:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 10 2025, 7:27 PM
Reedy updated the task description. (Show Details)Sep 10 2025, 7:28 PM
Reedy edited projects, added: FY2025-26 WE4.6.2 Multiple Authenticators; removed: FY2025-26 WE 4.6 - Account Security.Sep 10 2025, 9:01 PM
Reedy moved this task from Backlog to Features/Improvements on the MediaWiki-extensions-OATHAuth board.Sep 17 2025, 9:30 AM
Catrope moved this task from Backlog to Consider for future work on the FY2025-26 WE4.6.2 Multiple Authenticators board.Sep 29 2025, 5:29 PM
Catrope edited projects, added: FY2025-26 WE 4.6 - Account Security (WE 4.6.4 - 2FA improvements and passkey support); removed: FY2025-26 WE4.6.2 Multiple Authenticators.Oct 2 2025, 4:11 AM
Catrope moved this task from Inbox to Tech debt cleanup on the FY2025-26 WE 4.6 - Account Security (WE 4.6.4 - 2FA improvements and passkey support) board.Oct 2 2025, 6:19 PM
Mstyles claimed this task.Oct 2 2025, 6:37 PM
Mstyles added a project: Product Safety and Integrity (Sprint Apfel Strudel (Sep 29 - Oct 17)).
Reedy added a comment.Oct 9 2025, 9:47 PM

T406932: TypeError: MediaWiki\Extension\OATHAuth\Key\TOTPKey::__construct(): Argument #3 ($recoveryCodes) must be of type array, string given, called in /srv/mediawiki/php-1.45.0-wmf.22/extensions/OATHAuth/src/Key/TOTPKey.php on line 12 just reminded me of this. A user viewing the main page was getting fatals because of missing "schema" updates...

Obviously missing schema changes aren't exactly helpful. Neither are fatals, but...

	private function getDisabledGroups( User $user, array $groups ): array {
		$requiredGroups = $this->config->get( 'OATHRequiredForGroups' );
		// Bail early if:
		// * No configured restricted groups
		// * The user is not in any of the restricted groups
		$intersect = array_intersect( $groups, $requiredGroups );
		if ( !$requiredGroups || !$intersect ) {
			return [];
		}

		$oathUser = $this->userRepo->findByUser( $user );
		if ( !$oathUser->isTwoFactorAuthEnabled() ) {
			// Not enabled, strip the groups
			return $intersect;
		}

		return [];
	}

We were getting and loading their recovery tokens and stuff... just to see if they have 2FA enabled.

A little better if it's cached (in the same request), but still pretty wasteful overall.

OKryva-WMF edited projects, added: Product Safety and Integrity (Sprint Mint Choc Chip Ice Cream (Oct 20 - Nov 7)); removed: Product Safety and Integrity (Sprint Apfel Strudel (Sep 29 - Oct 17)).Oct 20 2025, 2:34 PM
Mstyles removed a project: Product Safety and Integrity (Sprint Mint Choc Chip Ice Cream (Oct 20 - Nov 7)).Nov 5 2025, 11:13 PM
Catrope removed Mstyles as the assignee of this task.Jan 23 2026, 12:21 AM
Catrope added a subscriber: Mstyles.
Reedy triaged this task as Low priority.May 6 2026, 2:51 PM
Restricted Application added a project: Product Safety and Integrity. · View Herald TranscriptMay 6 2026, 2:51 PM
gerritbot added a comment.May 9 2026, 8:37 PM

Change #1285497 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@master] OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabled

https://gerrit.wikimedia.org/r/1285497

gerritbot added a project: Patch-For-Review.May 9 2026, 8:37 PM

Change #1285498 had a related patch set uploaded (by Reedy; author: Reedy):

[integration/config@master] zuul: Add OATHAuth as a dependancy for EmailAuth

https://gerrit.wikimedia.org/r/1285498

gerritbot added a comment.May 9 2026, 8:45 PM

Change #1285498 merged by jenkins-bot:

[integration/config@master] zuul: Add OATHAuth as a dependancy for EmailAuth

https://gerrit.wikimedia.org/r/1285498

Dreamy_Jazz edited projects, added: Product Safety and Integrity (Sprint lily-of-the-valley (May 4 - May 22)); removed: Product Safety and Integrity.May 14 2026, 4:37 PM
Dreamy_Jazz moved this task from Backlog to Needs review on the Product Safety and Integrity (Sprint lily-of-the-valley (May 4 - May 22)) board.
Dreamy_Jazz assigned this task to Reedy.Tue, May 26, 9:11 PM
Dreamy_Jazz moved this task from Sprint lily-of-the-valley (May 4 - May 22) to Sprint Iris (May 25 - Jun 12) on the Product Safety and Integrity board.
Dreamy_Jazz edited projects, added: Product Safety and Integrity (Sprint Iris (May 25 - Jun 12)); removed: Product Safety and Integrity (Sprint lily-of-the-valley (May 4 - May 22)).
Dreamy_Jazz moved this task from Backlog to Needs review on the Product Safety and Integrity (Sprint Iris (May 25 - Jun 12)) board.
gerritbot added a comment.Tue, May 26, 11:53 PM

Change #1285497 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabled

https://gerrit.wikimedia.org/r/1285497

Maintenance_bot removed a project: Patch-For-Review.Wed, May 27, 12:30 AM
gerritbot added a comment.Wed, May 27, 1:50 AM

Change #1293837 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@master] OATHUserRepository: Fixup userHas2FAEnabled description

https://gerrit.wikimedia.org/r/1293837

gerritbot added a project: Patch-For-Review.Wed, May 27, 1:50 AM
gerritbot added a comment.Mon, Jun 1, 10:25 PM

Change #1293837 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] OATHUserRepository: Fixup userHas2FAEnabled description

https://gerrit.wikimedia.org/r/1293837

Maintenance_bot removed a project: Patch-For-Review.Mon, Jun 1, 10:30 PM
Dreamy_Jazz moved this task from Needs review to Done on the Product Safety and Integrity (Sprint Iris (May 25 - Jun 12)) board.Wed, Jun 3, 10:47 PM
Reedy closed this task as Resolved.Mon, Jun 8, 10:46 PM
Reedy mentioned this in T428518: 2FA status is being looked up on every page view.
gerritbot added a comment.Mon, Jun 8, 10:49 PM

Change #1298936 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@REL1_46] OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabled

https://gerrit.wikimedia.org/r/1298936

gerritbot added a project: Patch-For-Review.Mon, Jun 8, 10:49 PM

Change #1298937 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@REL1_46] OATHUserRepository: Fixup userHas2FAEnabled description

https://gerrit.wikimedia.org/r/1298937

gerritbot added a comment.Mon, Jun 8, 10:54 PM

Change #1298936 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@REL1_46] OATHUserRepository: Add a 'cheap' way to check if a user has 2FA enabled

https://gerrit.wikimedia.org/r/1298936

gerritbot added a comment.Mon, Jun 8, 10:54 PM

Change #1298937 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@REL1_46] OATHUserRepository: Fixup userHas2FAEnabled description

https://gerrit.wikimedia.org/r/1298937

gerritbot added a comment.Mon, Jun 8, 11:22 PM

Change #1298942 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@master] OATHUserRepository: Cache 2FA status in WANObjectCache

https://gerrit.wikimedia.org/r/1298942

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits