Restrict editing module entity settings to users who have access

[aaemnnosttv]

Feature Description

With the introduction of dashboard sharing, changing a module's connection (a.k.a. service entity) has substantial implications when shared – changing these settings requires taking on the responsibility of sharing (granting access via your Google account). Accordingly, changing these settings can only be done by admins who have access to the configured entity.

---

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

Acceptance criteria

• Module settings edit views for AdSense, Analytics, and Search Console should be updated based on the current user's access to the module (see Implement new selector for checking module access #4802)
  • If the current user does not have access to the module, the inputs that control the module's connection (i.e. owned settings) should be disabled
    • The user should still be able to change "secondary" settings (i.e. non-owned settings, e.g. useSnippet) and submit those changes even if they don't have access to the entity
    • When disabled, raw values can be displayed as necessary in place of the usual "rich" display value where API entity access is needed (e.g. a property ID in place of the ID + name) but should still appear as the same type of input (e.g. a disabled Select with single selected value)
    • An ℹ️ notice should be displayed below the inputs (see the design in Figma)

      {module owner username} configured {module name} and you don’t have access to this {connection entity}. Contact them to share access or change the {connection entity}.

    • Note {connection entity} refers to a service specific term, such as "Analytics property", "Search Console property" or "AdSense account" and should use separate translation strings rather than a formatted string to populate
  • If the current user does have access to the module
    • The inputs for owned settings should remain editable as usual
    • If dashboardSharing is enabled and the user has changed any of the owned settings (see Expose owned module settings to client #5121), and is not the owner themselves, and the module is currently being shared with one or more roles:
      display a new ⚠️ notice at the bottom of the edit view with the following content:

      By clicking {submit button text}, you’re granting other users view-only access to data from {module name} via your Google account. You can always manage this later in the dashboard sharing settings.

• Settings edit interfaces should remain in a loading state while the module access is being checked to avoid settings becoming disabled after being displayed

Implementation Brief

Implementing logic for when the user does not have access

• In assets/js/modules/analytics/components/settings/SettingsEdit.js and assets/js/modules/search-console/components/settings/SettingsEdit.js::

  • Fetch the current user's module access by using the hasModuleAccess( slug ).
  • If it is undefined, return the <ProgressBar> component.

• In assets/js/modules/search-console/components/common/PropertySelect.js, assets/js/modules/analytics/components/common/AccountSelect.js, assets/js/modules/analytics/components/common/PropertySelect.js, assets/js/modules/analytics/components/common/ProfileSelect.js and assets/js/modules/analytics-4/components/common/PropertySelect.js:

  • Fetch the current user's module access by using the hasModuleAccess( slug ) selector introduced in Implement new selector for checking module access #4802.
  • If there is a value, pass it to the disabled prop of the <Select> component so that the field is disabled if the value is true.

• In assets/js/modules/search-console/components/settings/SettingsForm.js,

  • Fetch the current user's module access by using the hasModuleAccess( slug ) selector.
  • If the value is false, render the <SettingsNotice> component.
    • Use type=TYPE_INFO and icon=WarningIcon below the select field as described in the AC/Figma design.
    • Replace {connection entity} with "Search Console property" as mentioned in the AC.
    • To fetch the owner's username, use the getModule( slug ) selector from the core/modules store. Note that the owner object will only be set if the user has sufficient permissions, although all users who can edit settings (admins) should have it, it shouldn't be assumed that the object is there. As a fallback, "Another admin" can be replaced with the username in the message.

• In assets/js/modules/analytics/components/settings/SettingsControls.js and assets/js/modules/analytics/components/settings/GA4SettingsControls.js:

  • Fetch the current user's module access by using the hasModuleAccess( slug ) selector.
  • If the value is false, render the <SettingsNotice> component as described above in search-console's <SettingsForm> component. Render it below the select fields in these components as per the Figma mock in the AC.

Implementing logic for when the user does have access

• In assets/js/components/settings/, create a new component <EntityOwnershipChangeNotice> as follows:

  • The component should have a slug prop which accepts the current module-slug for which the notice is being rendered.
  • Check if the DashboardSharing feature flag is enabled, return an empty component if false.
  • Fetch the current user's module access by using the hasModuleAccess( slug ) selector introduced in Implement new selector for checking module access #4802, return an empty component if false.
  • Use the new haveOwnedSettingsChanged selector from the current module's store (Expose owned module settings to client #5121) to find out if any of the owned settings have changed. Return an empty component if false.
  • Use the getOwnerID() selector from the current module's store. Use the getID selector from the core/user store. Compare the IDs to find out if the current user is the owner of the module. If the current user is the owner, return an empty component.
  • If none of the above conditions are met, return the <SettingsNotice> component with type=TYPE_WARNING and text as per the AC. The {submit button text} should be replaced with Confirm Changes. The module name can be passed as a prop to this component.

• In assets/js/modules/analytics/components/settings/SettingsForm.js and assets/js/modules/search-console/components/settings/SettingsForm.js,

  • Render the <EntityOwnershipChangeNotice> component created above at the end of the form.

Note: Owned settings for AdSense (accountID and clientID) are not editable in the edit settings view (SettingsEdit component). However, the Edit settings should be tested to ensure the settings still work when a user does not have access to the module.

StoryBook coverage

• Ensure the above states are added to Storybook in module-search-console-settings.stories.js, module-analytics-settings.stories.js and in the new Storybook v6 assets/js/modules/analytics/components/settings/SettingsForm.stories.js.

Test Coverage

• No new tests required.

QA Brief

• This issue affects the Settings dropdown lists of "Owned Settings", fields such as "Account", "Property", "Profile", etc. Some of these components are used by multiple settings pages. So it would be beneficial to test that existing settings pages (mainly "add" and "edit") work normally without errors as before for all modules which use these dropdown fields (Search Console, Analytics, AdSense and Tag Manager).

• Create an admin, say "Admin1" to setup SiteKit (this includes setting up a new Search Console property). Also setup Analytics and create a new property or use an existing one that is connected to Admin1's account.

• To verify Search Console settings:

  • With Access: Setup SiteKit using another admin with a separate google account (say "Admin2"). This step verifies and gives access to the site's Search Console property to Admin2's Google Account, thus giving "access" to Admin2. Verify the ACs for when "current user has access". The SC Property field will only show a single value in the dropdown and so there is no value to change. For test purposes, use the Developer Settings plugin and add a new Custom Site URL. Come back to Search Console settings and now you will be able to "change" the Property dropdown as Admin2 which should display the "Yellow Warning" as in the AC.
  • Without Access: To remove Admin2's access, on the Search Console settings accordion, click on the 'See full details in Search Console'. In the Search Console dashboard that pops up, click on "Settings" in the left menu and click on the "REMOVE PROPERTY" button on the settings page. Go back to SiteKit, refresh the settings page and try to "edit" search console again. Verify the ACs for when "current user does NOT have access".

• To verify Analytics Settings:

  • With Access: As Admin1, login to the "Analytics Dashboard" (by clicking on "See full details in Analytics" within the Analytics accordion in SiteKit's Settings). Click on the "Admin" settings cog in the left menu. Under the first "Account" column, click on "Account Access Management". In the popup, click on the + sign to add Admin2's Google Account to this account. Now login to SiteKit as Admin2, go to Analytics Settings and try editing them. Verify the ACs for when user has access.

  • 

  • Without Access: In Admin1's "Analytics Dashboard" mentioned above, follow the steps to go to Account Settings. In the "Account Access Management" popup, click on the 3 dots next to Admin2's Google Account and click on remove access. Now login to SiteKit as Admin2, go to Analytics Settings and try editing them again. Verify the ACs for when user does not have access.

• AdSense: There are no "owned settings" for AdSense and so no changes have been made specifically to the settings components here. They should just work normally as before.

• Verify the new stories in Storybook: Search Console, Analytics old story format, Analytics new story format - no access and with access.

Changelog entry

• Restrict editing module entity settings to users who have access, either by being the module owner or by having the module shared with them.

[aaemnnosttv]

@felixarntz @marrrmarrr regarding the cases above, I based them off what we have in Figma but both were adapted to be non-module specific. Also the with-access/takeover case copy was adapted from what we have in the sharing settings. Let me know if this sounds good to you, otherwise I think this is ready for an IB.

[jimmymadon]

@aaemnnosttv Just a couple of questions:

• Owned settings for AdSense (accountID and clientID) are not editable in the edit settings view (SettingsEdit component). Should we be doing anything in this case?

• We currently don't have a selector to get the user's name from their ID. I can create a separate issue. @tofumatt and I discussed that it will be easier to add the owner's name to the module's selector than trying to insert/reuse WordPress's core data/selectors which are currently only available on in the Gutenberg Block editor?

[aaemnnosttv]

• Owned settings for AdSense (accountID and clientID) are not editable in the edit settings view (SettingsEdit component). Should we be doing anything in this case?

Good point - I suppose not. This can be part of the IB I think 👍

• We currently don't have a selector to get the user's name from their ID. I can create a separate issue. @tofumatt and I discussed that it will be easier to add the owner's name to the module's selector than trying to insert/reuse WordPress's core data/selectors which are currently only available on in the Gutenberg Block editor?

The module owner's username is already available on the module.owner.login returned from getModule/getModules (we use this in a few places). Note that the owner object will only be set if the user has sufficient permissions, although all users who can edit settings (admins) should have it, it shouldn't be assumed that the object is there.

[eugene-manuilov]

In assets/js/modules/search-console/components/common/PropertySelect.js:

• Fetch the current user's module access by using the hasModuleAccess( slug ) selector introduced in Implement new selector for checking module access #4802.
• If it is undefined, return the <ProgressBar small> component.

@jimmymadon no need to do it in every component. Every module has the SettingsEdit component that returns a progress bar if the required state isn't resolved yet. That is where we need to check whether the use has access to the module or not and that is where we need to return a progress bar.

site-kit-wp/assets/js/modules/adsense/components/settings/SettingsEdit.js

Lines 34 to 36 in 8b62084

	if ( isDoingSubmitChanges ) {
	viewComponent = <ProgressBar />;
	} else {

site-kit-wp/assets/js/modules/analytics/components/settings/SettingsEdit.js

Lines 57 to 59 in 8b62084

	if ( isDoingSubmitChanges || ! hasResolvedAccounts ) {
	viewComponent = <ProgressBar />;
	} else if ( ! accounts.length || isCreateAccount ) {

site-kit-wp/assets/js/modules/optimize/components/settings/SettingsEdit.js

Lines 34 to 36 in 8b62084

	if ( isDoingSubmitChanges ) {
	viewComponent = <ProgressBar />;
	} else {

site-kit-wp/assets/js/modules/search-console/components/settings/SettingsEdit.js

Lines 45 to 47 in 8b62084

	if ( isDoingSubmitChanges || ! hasResolvedProperties ) {
	viewComponent = <ProgressBar />;
	} else {

By checking module access in those components, we ensure that hasModuleAccess selectors will be already resolved in the specific input field elements like PropertySelect, AccountSelect and so on.

Furthermore, if it is false, render the <SettingsNotice> component below the <Select> as follows:

• Use type=TYPE_INFO and icon=WarningIcon below the select field as described in the AC/Figma design.
• Replace {connection entity} with "Search Console property" as mentioned in the AC.
• To fetch the owner's username, use the getModule( slug ) selector from the core/modules store. Note that the owner object will only be set if the user has sufficient permissions, although all users who can edit settings (admins) should have it, it shouldn't be assumed that the object is there. As a fallback, "Another admin" can be replaced with the username in the message.

Same here, we shouldn't display this message in a particular input field. The notice should be displayed in the SettingsForm component.

[jimmymadon]

Every module has the SettingsEdit component that returns a progress bar if the required state isn't resolved yet.

@eugene-manuilov I originally was going to do it that way - then realised that in the case of Analytics, we don't prevent the loading of all other elements/switches if we are only 'deciding' on how to render the Select fields. But in the end, I did for Analytics, I did put the loading logic in SettingsEdit as I didn't want multiple loading bars showing up within. So yeah, makes sense to move the search-console too! Thanks!

[jimmymadon]

An ℹ️ notice should be displayed below the inputs (see the design in Figma)

{module owner username} configured {module name} and you don’t have access to this {connection entity}. Contact them to share access or change the {connection entity}.

@aaemnnosttv Just to clarify, in Analytics, should this notice be displayed twice - below the UA fields and the GA4 fields or just below the UA fields?

[eugene-manuilov]

• In assets/js/modules/analytics/components/settings/SettingsEdit.js and assets/js/modules/search-console/components/settings/SettingsEdit.js::

@jimmymadon, we forgot to add the AdSense module.

[jimmymadon]

• In assets/js/modules/analytics/components/settings/SettingsEdit.js and assets/js/modules/search-console/components/settings/SettingsEdit.js::

@jimmymadon, we forgot to add the AdSense module.

@eugene-manuilov I have added a note to the IB based on Evan's reply to my comment. AdSense does not have any editable owned settings - so I don't think it is necessary to check for module access here?

[techanvil]

Hey @jimmymadon, this is looking good but please can you write a QA Brief for the issue?

[wpdarren]

QA Update: ⚠️

@jimmymadon an observation: if the current user does not have access to Analytics, the blue warning message appears twice in the settings. Once under the account and main property, then again under GA4. Is that expected? In addition there's a red text permission message too, I am assuming that is expected too? 🤔

[wpdarren]

QA Update: ✅

After conversation with @jimmymadon we decided to include the observation above as a potential 'known issue' for bug bash so it can be discussed further, as we are unsure of the definition here. I am therefore moving this to approved.

Verified:

• If the current user does not have access to the module, the inputs that control the module's connection are disabled.
• The user can still be able to change "secondary" settings changes even if they don't have access to the entity
• When disabled, raw values can be displayed as necessary as the same type of input.
• A notice is displayed below the inputs as per the figma designs.

{module owner username} configured {module name} and you don’t have access to this {connection entity}. Contact them to share access or change the {connection entity}.

• If the current user does have access to the module, the inputs for owned settings remain editable.
• If dashboardSharing is enabled and the user has changed any of the owned settings and is not the owner themselves,
  a new notice at the bottom of the edit view with the following content appears.

By clicking {submit button text}, you’re granting other users view-only access to data from {module name} via your Google account. You can always manage this later in the dashboard sharing settings.

• Settings edit interfaces remain in a loading state while the module access is being checked.

Screenshots

[jimmymadon]

@wpdarren

if the current user does not have access to Analytics, the blue warning message appears twice in the settings. Once under the account and main property, then again under GA4. Is that expected?

This is what I asked Evan here and for now we have decided to proceed with 2 notices if the current user does not have access to both, the UA and GA4 properties. Thinking about this now, I should've included adding/removing access at the "Property" level instead of the "Account" level as defined in the QAB to test this behaviour.

However, the red notice can be discussed further as a 'known issue'.