Page MenuHomePhabricator

Prepare OAuth extension for IP Masking
Closed, ResolvedPublic

Description

OAuth has a bunch of special pages and API endpoints which deal with user permissions so it's affected by the introduction of temporary account.

Most notably, we need to decide whether temp users can use OAuth tools, and if so, how should they be shown on the profile endpoint.

Event Timeline

Existing OAuth logic already expects users to at least have email defined. Some parts of the system go a little bit further and require a confirmed email. Temporary accounts cannot log in as those do not have passwords - further more - they cannot update their email which already prevents them from using OAuth.

My proposal is to allow OAuth only for named users. Any thoughts @Tgr @Ammarpad

There is no immediate action required to take, although it's great to clean up the code a bit and update the isAnon() and isRegistered() calls for clarity.

References:

	public function userCanExecute( User $user ) {
		return $user->isEmailConfirmed();
	}

https://gerrit.wikimedia.org/g/mediawiki/extensions/OAuth/+/903f6685ce687d378e9c8dd82c49179759a54dc3/src/Frontend/SpecialPages/SpecialMWOAuthConsumerRegistration.php#80

			// Note: Owner-only clients can only use client_credentials grant
			// so would be rejected from this endpoint with invalid_client error
			// automatically, no need for additional checks
			if ( !$this->user instanceof User || $this->user->isAnon() ) {
				return $this->getLoginRedirectResponse();
			}

https://gerrit.wikimedia.org/g/mediawiki/extensions/OAuth/+/903f6685ce687d378e9c8dd82c49179759a54dc3/src/Rest/Handler/Authorize.php#48

		if ( $user->getEmail() === '' ) {
			$this->fatalError( 'User must have an email' );
		}

https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/OAuth/+/903f6685ce687d378e9c8dd82c49179759a54dc3/maintenance/createOAuthConsumer.php#68

Change 993162 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/extensions/OAuth@master] OAuth logic requires users to be named

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

The only thing I would consider at this stage is to see if we want to show identical error messages to temporary users and anons.

At this moment temporary users will see the same messages -> maybe we want to make them different, but that probably would require PO input.

My proposal is to allow OAuth only for named users. Any thoughts @Tgr @Ammarpad

Sorry for taking so long to respond!

There are two groups of use cases here:

  • Creating OAuth apps (Special:OAuthConsumerRegistration). This already requires an email address, and even if it didn't, it makes no sense to allow tying app ownership to an account which will self-destruct. We don't necessarily need to do anything here though, the permission system will take care of it.
  • Using OAuth apps (Special:OAuth, Special:OAuthManageMyGrants, some of the REST APIs including Authorize.php). This doesn't require anything beyond having a user account. AFAICS it does currently work for temp accounts.

The second one is more of a product decision. I would lean on allowing it, but then we have to figure out how to expose Special:OAuthManageMyGrants (which is where they can revoke permissions from apps) since it's normally available via Special:Preferences, but temp users can't access that. We should probably also tell apps that the user is a temp user (add a flag to the profile endpoint).

If we disallow then we'll need an error message telling the user to register.

  • Using OAuth apps (Special:OAuth, Special:OAuthManageMyGrants, some of the REST APIs including Authorize.php). This doesn't require anything beyond having a user account. AFAICS it does currently work for temp accounts.

...
If we disallow then we'll need an error message telling the user to register.

I think we should disallow using OAuth apps for temp accounts. If there's a clear product need to have that enabled at some point in the future, we could revisit the discussion. But for now, it would simplify the scope of things we need to worry about for the temp accounts project if OAuth app interactions are disabled.

cc @Tchanders @Madalina

  • Using OAuth apps (Special:OAuth, Special:OAuthManageMyGrants, some of the REST APIs including Authorize.php). This doesn't require anything beyond having a user account. AFAICS it does currently work for temp accounts.

...
If we disallow then we'll need an error message telling the user to register.

I think we should disallow using OAuth apps for temp accounts. If there's a clear product need to have that enabled at some point in the future, we could revisit the discussion. But for now, it would simplify the scope of things we need to worry about for the temp accounts project if OAuth app interactions are disabled.

cc @Tchanders @Madalina

I'm not aware of any particular apps that temp accounts would need access to that anons can't currently access. For that reason it feels like an enhancement and therefore not necessary to do straight away. But I may have missed something.

I'm happy to leave this to the judgement of the MediaWiki-Platform-Team. It would be helpful for us to know whether this would need to be done before testwiki or pilot wiki deployment, or whether it can be done at any time regardless of the deployment schedule.

After checking https://www.mediawiki.org/wiki/User_account_types - Temporary Users feature set is more aligned with Anon user, not registered. The only difference between IP and Temp is that Temp can receive notifications -> which if I'm right was already happening for edits coming from IP - therefore it's not anything new.

@Tchanders we're seeking a production decision on whether to allow Temporary users access OAuth Apps. I'm leaning towards not allowing access to OAuth apps and returning an error message that users must register to use this feature. My suggestion is based on the following reasons:

  • temp users do not have user groups/preferences/watchlists -> allowing OAuth Apps access Tempo users would increase the complexity on the app side too as those apps should know how to handle temp users (like no access to watchlist even user has an account)
  • It would be good to ask users to create an account as they will get a better experience in general. I'm sure that plenty of users won't understand the difference between Permanent Users and Temporary Users. Those people might be surprised that after some time they just lose access to the site and they cannot log in or that some features just don't work and they will struggle to understand why.
  • Mediawiki & Extensions are already getting complex, we should come up with some basic rules on what Temporary Users can access. Going and checking every feature one by one and deciding if that specific is "allowed" will cause us lots of confusion in the future as we won't have good clarity on what Temp users can and cannot.
  • Temporary accounts cannot log in, those accounts are created under the hood. OAuth allows authorization to perform specific flow to authenticated users. But Temp users cannot authenticate as this happens under the hood - therefore I'm a little bit uncertain if giving OAuth access is a valid approach.
  • Additional work won't bring much value, as we can just ask users to create an account instead. Less code/less complexity.

Looks like @Tgr is leaning towards allowing Temp users to use OAuth but this won't work out of the box. We would have to tackle how to allow them access Special:Preferences and in general it would increase the complexity. Plus still some apps might not work properly due to Temporary Users limitations.

Therefore I suggested not allowing access to OAuth and treating Temp Users the same as Anon, and looks like I also have @kostajh I support.

My proposal: If the "Temporary User" accesses any of the Special Pages/API Endpoints provided by the OAuth extension - we will redirect them to the Login page with a message similar to the one shown when they try to access Special:Preferences.

Proposed error message: Please login to access OAuth feature.

@Madalina @Niharika - do you have any thoughts on this?

Looks like we already have a pretty similar error message

	"mwoauth-login-required-reason": "You need to log into your {{SITENAME}} account to authorize applications to access it.",

Maybe we can just re-use it.

Looks like we already have a pretty similar error message

	"mwoauth-login-required-reason": "You need to log into your {{SITENAME}} account to authorize applications to access it.",

Maybe we can just re-use it.

I think for a temp user we should assume that they don't have an account, so we should say something like "you need to create a permanent account".

Therefore I suggested not allowing access to OAuth and treating Temp Users the same as Anon, and looks like I also have @kostajh I support.

My proposal: If the "Temporary User" accesses any of the Special Pages/API Endpoints provided by the OAuth extension - we will redirect them to the Login page with a message similar to the one shown when they try to access Special:Preferences.

Proposed error message: Please login to access OAuth feature.

Yes to disallowing using OAuth apps for temp accounts. The proposal makes sense to me.

Change 993162 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] OAuth logic requires registered users

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