Request write scopes only when needed

[felixarntz]

Feature Description

Site Kit currently requests scopes on a per-module basis. Whenever you activate a module, you have to grant access to all scopes that using this module may need. The "may" is an important word here: For example, you will only need Analytics write permissions if you're creating a new account, property, or profile. However, currently all users need to grant these permissions even if they'll never use this functionality. Using incremental scopes is a best practice with OAuth, and while we're doing a decent job at it handling it per module, we should improve that specifically for scopes that allow write access, so that these are really only requested when they're needed.

This is especially time-critical because of the Analytics provisioning workstream which introduced a new write scope (for the first time since original plugin release). With the current logic in place, every existing user with Analytics active would see a prompt to grant that scope, which would be very much out of context, and they may not want to use this feature at all.

As part of this issue, we will mainly focus on Analytics, since the implementation relies on the write-related client logic being refactored already (using datastores). We can also add the change to AdSense, although that is trivial here because we don't expose any write logic (the API only has a single route that writes data anyway). This issue will only partially address the issue for Tag Manager though, since we'll need to complete #1386 first, but it must not be a blocker for this one.

Two modules that should not be changed are Search Console and Site Verification. Site Verification doesn't differentiate between read and write scopes anyway, and for Search Console we can continue to always request write permissions, since they are already needed during setup. We can reconsider that separately in the future.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The get_scopes methods of the following modules should be modified to only include non-write scopes:
  • For AdSense, https://www.googleapis.com/auth/adsense should be replaced with https://www.googleapis.com/auth/adsense.readonly.
  • For Analytics, all scopes other than https://www.googleapis.com/auth/analytics.readonly should be removed.
• The Authentication::needs_reauthenticate() method in PHP needs to be updated so that lower permission required scopes we request don't result in true if the user already has granted access to a higher permission scope (e.g. if we request adsense.readonly but the user already granted adsense). This is so that users from older versions are not unexpectedly and unnecessarily asked to re-authenticate.
• All module datapoints should be annotated with specific scopes that they require if their functionality requires more scopes than the regular module ones (the read-only ones). This should use an alternate version of the get_datapoint_services() method, while not breaking backward-compatibility.
• The general datapoint handler logic in the Module base class should implement a mechanism to check for such datapoints that the user has these scopes. If not, a missing_required_scopes error should be returned, and the array of required scopes should be included in the error data.
• Client-side module datastore functionality should be implemented (potentially via createModuleStore, related: Add getAdminScreenURL and getAdminReauthURL selectors to base module store #1559) that handles incoming errors.
  • If the error is a missing_required_scopes one, the client should open a modal dialog informing the user they need to grant additional permissions (exact wording TBD, let's just put something basic for now and review later).
    • The dialog should have a button to go to OAuth. When clicked, the datastore's state should be temporarily persisted on the client, and the current fetch action that caused this should be stored as well so that it can be retried later. It should then redirect the user to the OAuth consent screen via the reauth URL functionality (depending on whether Add getAdminScreenURL and getAdminReauthURL selectors to base module store #1559 is done or not, alter the new datastore function or the existing getReAuthURL utility function), with the redirect URL set to the same URL they're currently on (pretty much like today).
    • The dialog should also have a cancel button. When clicked, it should remove the error and just display the same UI as before.
• Once the user comes back from OAuth, the client should restore the persisted state and retry the action from before, and then proceed as usual. If the user didn't grant the scopes, just do the same thing again (they just have to grant it).
  • If the user doesn't grant scopes (i.e. access_denied error) for such a case of additional scopes being requested (non-default module write scopes), they should not end up on the dashboard as usual, but on the original screen they came from, like in the success case.

Implementation Brief

• Modify required module scopes lists per the first bullet point of the ACs.
• Introduce Scopes utility class with a map of scopes mapped to a list of other scopes that satisfy them (user needs to have all of these scopes), and introduce a static method has_scopes( $required_scopes, $granted_scopes ) that returns true|false. If a required scope is in the scope map, it should be considered covered if $granted_scopes includes all of these scopes or the scope itself; otherwise the scope itself needs to be actually included in $granted_scopes. The scopes covered should be:
  • adsense.readonly --> adsense
  • analytics.readonly --> analytics.edit and analytics
  • analytics.manage.users.readonly --> analytics.manage.users
  • tagmanager.readonly --> tagmanager.edit.containers
  • webmasters.readonly --> webmasters
• Move Authentication::need_reauthenticate() to OAuth_Client as a public method, and modify it to only retrieve the option values and then pass them to the above Scopes::has_scopes method (the current array_intersect logic will be replaced with logic from there).
• Introduce Module::get_datapoints_definition() method, which should return a map of $datapoint_method:$datapoint => $datapoint_definition pairs, with the latter being an associative array. For now, the only thing required in that array should be a service key, and it should optionally have a scopes key and a request_scopes_message key. This should eventually replace the existing Module::get_datapoint_services(), but we should maintain backward-compatibility here. Let's make Module::get_datapoint_services() non-abstract and return an empty array by default. The default implementation of Module::get_datapoints_definition() should call Module::get_datapoint_services() and transform the return value so that it has the expected format (values should be associative arrays with service slug, instead of just the service slug). The current get_datapoint_services() does not differentiate between GET and POST on a code-level basis, so it should for this BC cases be assumed to cover both.
• Modify Module logic for handling datapoint requests to see if the datapoint has a scopes entry, and if so, ensure the user has granted all these scopes (via Scopes::has_scopes( $datapoint_scopes, OAuth_Client::get_granted_scopes() )). If not, return a missing_required_scopes error, and include the array of required scopes in data.requiredScopes. If the datapoint also has a request_scopes_message (see above), including that in the error as data.requestScopesMessage. Use a specific scopes exception that implements a new WP_Errorable contract, taking care of the additional error data.
• Replace implementations of Module::get_datapoint_services() with Module::get_datapoints_definition() in Analytics and Tag Manager module, annotating datapoints that require additional scopes with a list of them and add request_scopes_message annotations on a case-by-case basis (TBD). Even though we will for now keep the regular list of Tag Manager scopes as immediately requested, the datapoints can already be updated to have the annotations (the user will just never run into the "error" here because they will already have granted scopes).
  • A secondary concern here is to also migrate the other module classes so that they include get_datapoints_definition() implementations that replace the current get_datapoint_services() implementations. All these other modules do not need to request any additional scopes currently, so they should just return the correct $datapoint_method:$datapoint entries with the respective service key.
• Introduce OAuth_Client::get_granted_additional_scopes() method that looks at a new option googlesitekit_additional_auth_scopes and returns that array. OAuth_Client::get_granted_scopes() should be modified to include these scopes too (see below why we need this extra separation).
  • In the ?oauth2callback=1 handler, separate granted scopes by whether they are in get_required_scopes() (the generally required scopes) or not. If not, put them into the googlesitekit_additional_auth_scopes option, otherwise into the regular googlesitekit_auth_scopes option.
• Modify OAuth_Client::get_authentication_url() to support an additional optional parameter $additional_scopes. The method should call setScopes with not only get_required_scopes(), but the combination of get_required_scopes(), get_granted_additional_scopes(), and $additional_scopes. This is why we need the additional scopes handling. We also need to keep requesting the additional scopes from elsewhere that the user already granted, otherwise they will be "un-granted" again.
• Authentication::handle_oauth() should be modified to support an additional_scopes query parameter for the googlesitekit_connect bit, to be passed on to OAuth_Client::get_authentication_url(). The client will need to pass the additional scopes needed for the specific API datapoint in this parameter.

Client Implementation

• Add an action setPermissionScopeError() to the core/user data store that would allow missing scopes to be set by any component/datastore when a permissions error is encountered.
• Add a selector getPermissionScopeError() that checks for missing permissions/scopes for this user based on any attempted actions on the page. This would be used in the PermissionsModal component, prompting users to navigate to an OAuth page that requests the missing scopes for their account.
• In code where we submit requests that require write-access to a Google API (eg. create-account in Analytics), check for a the missing_required_scopes error and dispatch a notification to dispatch( 'core/user' ).setPermissionScopeError(). An example would be in https://github.com/google/site-kit-wp/blob/developer/assets/js/modules/analytics/datastore/accounts.js#L171. If the WP REST API returns missing_required_scopes, we would set the error in the data store.
• Create a PermissionsModal component that conditionally renders a modal when getPermissionScopeError() returns an error. This will show some kind of modal/error message/etc. that prompts the user to request more permissions with text/a link from the WP REST API, based on the data set it getPermissionScopeError(). This should be inside a common, Root component going forward (see: Add RegistryProvider to all React apps #1530), but for now can wrap Analytics along with our <RestoreSnapshots> component wrapper.

Error modal

The permissions error is important enough to be a blocking UX and warrants a modal window. We should add a PermissionsModal component that displays:

• a header
• body text
• required scopes to use to construct a URL to the OAuth request page

The last prop is required because there's no generic URL to send a user to update their scopes. This would be provided by the PHP function Authentication::get_connect_url() with the scopes needed and should be sent by the API in a missing_required_scopes error.

For the non-optional props, the modal error component can have generic, but non-module-specific messages about missing permissions. If the API wants to specify what the permissions are and why they're being requested, they'll be returned in the error set by dispatch( 'core/user' ).setPermissionScopeError().

State restoration

• Create an asynchronous takeSnapshot() action available on Data.commonActions to use that will save a snapshot of the entire data store's state to our Cache API (

  site-kit-wp/assets/js/googlesitekit/api/cache.js

  Line 17 in b5b4f28

  */

  ).
• Create an action that can attached to any data store to allow for state restoration (restoreSnapshot( { clearAfterRestore = true } )). It should check for cached data, set it using a overwriteState(newState) action that entirely overwrites state, and then clear the cache so the data is not kept. In the future we'll likely want to extend this action to support the argument clearAfterRestore: false, but that can be marked as TODO: and left unsupported for now. If this action is attached to a data store: it supports state restoration.
• Create a utility function getStoresWithSnapshots( registry = googlesitekit.data ) that iterates through all data stores (using registry.namespaces) and checks each one for an action with the name restoreSnapshot. This action should return all data store names that support state restoration with the above action and list them in an array.
• Create a utility function restoreSnapshotsForStores( registry, ...storeNames ) that waits for all calls to restoreSnapshot() on every store in getStoresWithSnapshots() to complete. This will be used to block rendering of components beneath <Root> until completed.
• Add a <RestoreSnapshots> component that blocks rendering of children component (all datastore-reliant components) until all state snapshots (restoreSnapshotsForStores()) are restored.

Potential issues

For edit views in settings this might be problematic though: we don't currently have a way to open a particular screen on the settings view to my knowledge. So if you were editing a module, needing write scopes, but were on this screen:

we wouldn't have a good way to send you back there right now. The editing state isn't hooked up to our data store so restoring it via state isn't possible (though would be my preferred approach).

• A likely solution to this for now is to set a separate cache value when redirecting in the modal to point to which page we should open, reading that when the Settings component first mounts, use it to set the isEditing prop, and then clear it after the first time it's checked.

QA Brief

Changelog entry

• Only request readonly OAuth scopes for each module by default, and prompt for additional scopes when needed for a specific action.

[aaemnnosttv]

@felixarntz the IB looks good for the most part so far.

I'm not sure we need to separate required scopes from extra granted scopes in separate options. It seems like this is only needed to ensure the extra granted scopes are requested again when the user re-authenticates since those will not be required. Why not just merge all granted scopes with the required scopes when calling setScopes? That way all granted scopes will be included in the request, in addition to whatever is being asked for (required or not).

I think we still need the concept of "additional scopes" i.e. "scopes to request" but given the above, I think we can limit their footprint to parameters rather than add additional user options.

How about something like this?

<?php
/**
 * $this = OAuth_Client
 * @param array $scopes        Extra scopes to request.
 * @param string $redirect_url Redirect URL after authentication.
 */
public function request_scopes( array $scopes, $redirect_url = '' ) {
	if ( $this->need_reauthenticate( $scopes ) ) { // Update to accept optional $scopes.
		return $this->get_authentication_url( $redirect_url, $scopes ); // Update to accept optional $scopes.
	}

	return false; // Scopes already granted.
}

Aside: we should move Authentication::need_reauthenticate to OAuth_Client.

I feel like one of the more complicated bits will be determining if one of the current granted scopes satisfies the requirement for read access.
The higher permission actions all seem to require one specific scope, but it's the lower permission requests that can be authorized by multiple scopes.
For example, for some resources, analytics.edit grants read permission, while for others it does not. This doesn't apply to the resources we currently use, but it's worth noting.

Also, not all modules have corresponding {service} and {service}.readonly scopes. (E.g. Tag Manager), so simply checking if the same scope was granted without .readonly is insufficient.

For this reason, I think we'll need a different strategy for determining if granted scopes are sufficient or not.

Maybe get_scopes could return an associative array of { requiredScope => SatisfyingScope[] } where keys are the scope required, and values are lists of additional scopes that satisfy it.
For modules that only require a specific scope, returning a simple array as a list of scopes as it does now could still be supported. Then the actual logic for determining scope sufficiency should only be needed in need_reauthenticate.

---

I'll start working on some of the more straightforward details while we work on refining the more complicated details here.

[aaemnnosttv]

@felixarntz not sure what you mean by this:

Even though we will for now keep the regular list of Tag Manager scopes as immediately requested, the datapoints can already be updated to have the annotations

Why would we not change the required scopes only for Tag Manager (possibly issue I mentioned above?). This isn't mentioned in the AC One of the benefits here is that we can request less scopes as it seems we were already requesting a few that we didn't need.

Regarding extra scopes needed per-datapoint in the datapoint definitions, we might also want to allow for request method-specific definitions as some datapoints are available with both methods, but only require the extra scopes for POST/data.set requests. E.g. SV verification datapoint. I realize SV is a bit of an exception here, but you get the idea.

Maybe the definition should have a structure like this:

<?php
[ $datapoint => [
    'service' => $service,
    'GET' => [ "scopes" => $scopes ],
    'POST' => [ "scopes" => $scopes ],
]

[felixarntz]

@aaemnnosttv I've updated the current IB based on our conversation earlier. I've also checked all the Google API endpoints and included the correct list of scopes to cover in the IB (for Analytics you need two different scopes to cover the whole spectrum of .readonly, for all the other it's just one scope that covers all of .readonly).

[tofumatt]

I've added a clientside IB… apologies if it's too vague but I wanted to walk through the broad strokes here and not get too in-the-weeds about some of the "how" to move this along. Based on our earlier discussion, I think this should match up with what was proposed. Let me know if it sounds good, or if I'm missing anything 🙂

I'm marking this one as a 30 estimate because there's a lot of moving parts and code in this one.

[tofumatt]

I messed up the IB because the settings requests are not (necessarily) where we make requests that demand write permissions. Gonna update it in a bit and then send it back to IB Review. Thanks for catching that @aaemnnosttv 🙂

[tofumatt]

Tweaked the IB—should be set for another review now.

[felixarntz]

@tofumatt

Add a persist property when we register a settings store

Can you clarify what you mean by "settings" store? The functionality where we need to listen to missing_required_scopes errors and potentially show a modal and persist store state is not specific to settings, but essentially any API fetch call that is made from a module store (e.g. modules/analytics, modules/adsense etc.).

Persist the entire core/forms (see #1510) data store

I think doing this unconditionally (or any unconditional persistence FWIW) is out of scope here. We need to make sure we persist all currently relevant state (most importantly the current module store's one) only right before the user is redirected to the OAuth flow as a result of clicking the modal button that shows due to a missing_required_scopes error. We can think about where persistence could help generally, but it's a non-goal here.

Add a selector getPermissionScopeErrors() that checks for missing permissions/scopes for this user based on any attempted actions on the page. This would be used in setup/edit components to display a modal, warning, etc. prompting users to navigate to an OAuth page that requests the missing scopes for their account.

I would think this should be singular getPermissionScopeError()? There would never be multiple scope errors, whenever an API response returns one, essentially execution should stop and the modal should be displayed. Regarding how we handle the modal, we discussed earlier to handle it in a more centralized location than setup/edit, arguably as part of Root?

In code where we submit changes of saved settings (particularly when they would involve write-access to a Google API), check for a the missing_required_scopes error and dispatch a notification to dispatch( 'core/user' ).setPermissionScopeError().

See above, this is not particular to settings, but to module stores overall. To take an example, when you create a Google Analytics account via provisioning API, no settings are involved/affected. I think this part here of how we can check for the error would be good to define a bit more. We could put it in the catch block of all of our fetch... actions, which would also pass the contextual data requiredScopes and requestScopesMessage which are included in the error. This would allow for the centrally handled modal to provide messaging as contextually relevant as possible. We should probably write a utility function for this handling of the permissions error, as we will add this to all fetch... catch blocks in all our module stores (which will eventually be improved/simplified by having #1288 take care of it in some capacity).

• a link to the OAuth page to add permission scopes (required)

The last prop is required because there's no generic URL to send a user to update their scopes.

This is not entirely correct, the URL to point to will be the "connect URL" from Authentication::get_connect_url() (currently not available in any datastore, this needs to be added), with an additional query parameter additional_scopes that will need to contain the list of scopes required (part of getPermissionScopeError() objects). See server-side IB where this URL/query parameter is outlined. In other words, if the modal receives a header, body text, and required scopes, that should be all it needs. I think it would be worth considering making this a state-driven standalone component like we have elsewhere though and not pass all this via props. The modal component could subscribe to getPermissionScopeError() from core/user and not display if there is none. And if there is one, it would have all the necessary data from the returned error object (or use some fallback text).

On a call we discussed automatically submitting the failed form again after state is resumed, but upon further reflection I feel the UX of being re-directed back to a page with the form pre-filled after adding scopes would be fine, and would guide the user to submit it again.

I'm not sure I agree. The user presented a clear intent when they previously initiated an action, and they even request scopes to do it. Having them require to click again afterwards is I feel one click too much. When you do something in an app and it asks you for some permissions, are you expecting it to then proceed or are you expecting it to ask you to do it again? However, since this is an additional piece though and could also be added afterwards, I'm okay leaving it out for now and focus on the other functionality first. Once we have that we can better assess what this feels like.

A potential "quick-fix" solution is to supply a query param like isEditing=analytics or something that would be checked for when setting the isEditing prop and then cleared after the first time it's checked.

That sounds good to me, let's do that. Let's just make sure not to update the legacy code more than we need to for this (this issue is already quite big and we are on a tight schedule since this needs to go into 1.9.0).

[felixarntz]

Awesome, IB ✅

Let's get to it!

[felixarntz]

@cole10up Current state here, with all associated PRs merged, this is now ready to be QAd. Expected experience is:

• When you try to set up a GA account and haven't granted the scope, there should be an altered message above the button that says that you will need to grant additional permissions.
• Clicking the button will redirect to OAuth, then briefly back to plugin UI with a loading indicator, then to GA terms of service. From there on the rest is as before.
• For setting up a GA property or view during setup, if you click "Configure Analytics" you should see a modal that informs about additional permissions being required. Clicking "Proceed" should take you to OAuth. After granting the scopes you should end up back in the plugin with a loading indicator (where it's re-running the original action). From there on the rest is as before.
  • Note that due to technical limitations, for doing the same from within the Analytics settings panel it is expected that the action is not automatically rerun again once you return from OAuth. In there, you will end up in the same UI you were in before, and you'll have to manually click the button again, at which point it should proceed as usual. This has been approved for release because fixing will consume quite a bit of time.

Another general thing to test is that you shouldn't see the general warning that you need to grant additional scopes at any time (the one that e.g. would show on top of the dashboard). For that it will be especially good to test updating from e.g. 1.8.1 to the new version and ensure this does not happen.

[cole10up]

Tested

Installed latest SK, ran through scopes, confirmed:

Cancel during initial proxy setup passed:

Happy path setup passes:

Confirmed connecting all services passed:

Confirmed resetting SK and setting back up functions properly

Passed QA ✅

[felixarntz]

@cole10up Did you also specifically test the steps in #1566 (comment)?