site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Restrict editing GTM settings to users with access

Open aaemnnosttv opened this issue 2 years ago • 7 comments

Feature Description

In https://github.com/google/site-kit-wp/issues/4825 we updated the settings edit views for shareable modules to restrict editing of owned settings to users with access to the service entity.

This issue builds on that to apply the same restrictions to Tag Manager as a step to make this behavior less specific to dashboard sharing but modules with owned settings.


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

Acceptance criteria

The AC below are largely based on #4825 as this issue is mostly the same but for Tag Manager. Accordingly, the approach should mostly follow the existing implementation for the other modules where it is already applied.

  • Module settings edit view for Tag Manager should be updated based on the current user's access to the module (see #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 similar to the notices currently in place in Analytics

        {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
  • Settings edit interfaces should remain in a loading state while the module access is being checked to avoid settings becoming disabled after being displayed
  • Selectors for resources related to accounts/containers should only be invoked if the user has access to avoid duplicate messaging (see #5429)

Implementation Brief

Implementing logic for when the user does not have access

Note: The following implementation is very similar to the implementation within search console's corresponding components: <SettingsEdit>, <SettingsForm> and <PropertySelect>. So use these as a reference for code snippets to action the following steps.

  • In assets/js/modules/tagmanager/components/settings/SettingsEdit.js:

    • Fetch the loggedInUserID using CORE_USER's getID() selector.
    • Fetch the moduleOwnerID using MODULES_TAGMANAGER's getOwnerID() selector.
    • If the logged in user is not the module owner, then check if the hasModuleAccess( slug ) selector from CORE_MODULES. Pass this value down to the <SettingsForm> component as a new prop.
    • Check if any of the above 3 selectors are still loading (using their respective store's hasFinishedResolution() selector) and show the <ProgressBar> component if that is the case.
  • In assets/js/modules/tagmanager/components/settings/SettingsForm.js,

    • If the new hasModuleAccess prop is false, display a <SettingsNotice> with type=TYPE_INFO and a WarningIcon (with the module owners name) exactly the same as the one in search console's SettingsForm. Replace "Search Console property" with "Tag Manager account". Display this notice after the googlesitekit-setup-module__inputs div and above the <ContainerNames> component.
    • Pass the hasModuleAccess prop further down to the AccountSelect, WebContainerSelect and AMPContainerSelect components (default the prop to true in all these components as well).
  • In assets/js/modules/tagmanager/components/common/AccountSelect.js, WebContainerSelect and AMPContainerSelect:

    • If the new hasModuleAccess prop is false, then render a simpler <Select> with just the corresponding accountID / containerID / ampContainerID as the option and value, along with setting the disabled prop. This should be similar to how a simple <Select> is implemented in search-console's <PropertySelect>.
    • Also, within WebContainerSelect and AMPContainerSelect, wrap calls to select() the getWebContainers() and getAMPContainers() selectors and return null if the hasModuleAccess prop is false.

StoryBook coverage

  • Ensure the above states are added to Storybook in module-tagmanager-settings.stories.js.

Test Coverage

  • Add coverage to existing JS (RTL) tests for the above components when hasModuleAccess is false (or undefined).

QA Brief

Changelog entry

  • Only allow users with Tag Manager access to edit Tag Manager settings in the UI.

aaemnnosttv avatar Jul 01 '22 18:07 aaemnnosttv

We would like to get this into Sprint 81, as the Support team are waiting on this to release some documentation that has been written cc @eclarke1

FlicHollis avatar Jul 20 '22 10:07 FlicHollis

Hey @jimmymadon, the IB is looking good. A couple of minor typo-looking points -

  • In the 2nd point, "Tagmanager Account" should be "Tag Manager account".
  • In the 3rd point, "If the new hasModuleAccess prop is true" should read "If the new hasModuleAccess prop is false".

Also, in keeping with #5429, I would suggest:

  • Add default prop value of true for the new hasModuleAccess props.
  • Return null instead of empty array for the hasModuleAccess === false cases under point 3.
  • Add tests for the hasModuleAccess === false versions of the components mentioned in point 3.

Estimate-wise, similarly to #5429 I think this could arguably still be an 11 but just as happy to bump it up to err on the side of caution...

techanvil avatar Aug 08 '22 12:08 techanvil

Thanks @techanvil - have updated the IB.

jimmymadon avatar Aug 08 '22 15:08 jimmymadon

Thanks @jimmymadon!

IB :white_check_mark:

techanvil avatar Aug 08 '22 16:08 techanvil

QA Update: ⚠️

@hussain-t according to the QAB:

Ensure Admin1 configured Tag Manager and you don’t have access to this Tag Manager account. Contact them to share access or change the Tag Manager account. warning message was displayed.

When I follow the steps in the QAB, and I get to editing the GTM settings, I do not see the message above. This would make sense though since I have approved the GTM invitation and have access to this account. I am wondering if the QAB steps are incorrect, or am I missing something obvious here? 🤔

This is what I see when editing GTM for admin2:

image

wpdarren avatar Aug 23 '22 03:08 wpdarren

@wpdarren the steps mentioned in the QAB are correct. I was able to reproduce it. This is the screenshot from my secondary admin dashboard:

Screenshot 2022-08-23 at 10 23 40 AM

Could you confirm your user access permissions are the same as mine below?

Screenshot 2022-08-23 at 10 27 33 AM

I'm happy to jump on a call to walk you through it.

hussain-t avatar Aug 23 '22 05:08 hussain-t

QA Update: ✅

@hussain-t let's put this down to user error! 😄

Verified:

  • The warning message is displayed Admin1 configured Tag Manager and you don’t have access to this Tag Manager account. Contact them to share access or change the Tag Manager account.

image

  • Also checked the stories in the QAB, and all look as expected for the different scenarios.

wpdarren avatar Aug 24 '22 02:08 wpdarren