site-kit-wp
site-kit-wp copied to clipboard
Restrict editing GTM settings to users with access
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
- The user should still be able to change "secondary" settings (i.e. non-owned settings, e.g.
- If the current user does have access to the module
- The inputs for owned settings should remain editable as usual
- 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
- 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
usingCORE_USER
'sgetID()
selector. - Fetch the
moduleOwnerID
usingMODULES_TAGMANAGER
'sgetOwnerID()
selector. - If the logged in user is not the module owner, then check if the
hasModuleAccess( slug )
selector fromCORE_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.
- Fetch the
-
In
assets/js/modules/tagmanager/components/settings/SettingsForm.js
,- If the new
hasModuleAccess
prop is false, display a<SettingsNotice>
with type=TYPE_INFO
and aWarningIcon
(with the module owners name) exactly the same as the one in search console'sSettingsForm
. Replace "Search Console property" with "Tag Manager account". Display this notice after thegooglesitekit-setup-module__inputs
div and above the<ContainerNames>
component. - Pass the
hasModuleAccess
prop further down to theAccountSelect
,WebContainerSelect
andAMPContainerSelect
components (default the prop totrue
in all these components as well).
- If the new
-
In
assets/js/modules/tagmanager/components/common/AccountSelect.js
,WebContainerSelect
andAMPContainerSelect
:- If the new
hasModuleAccess
prop is false, then render a simpler<Select>
with just the correspondingaccountID
/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
andAMPContainerSelect
, wrap calls toselect()
thegetWebContainers()
andgetAMPContainers()
selectors and returnnull
if thehasModuleAccess
prop isfalse
.
- If the new
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
- Go to Site Kit GTM settings.
- Setup a new account using the (default) Google account for
Admin1
. - Create another user with an
Administrator
role. - Go to the GTM console and add another Google account (user) for the newly created GTM account.
- Accept the GTM invitation from the secondary Google account.
- Login to WP admin using the second admin.
- Setup Site Kit using the (secondary) Google account, which was added to the GTM account.
- Go to Site Kit GTM settings and click edit.
- 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. - For code reviews, ensure the following stories are as expected:
- Default - Edit with all settings w/o module access
- Primary AMP - Edit with all settings w/o module access
- Secondary AMP - Edit with all settings w/o module access
- For QA, ensure the following stories are as expected:
- Default - Edit with all settings w/o module access
- Primary AMP - Edit with all settings w/o module access
- Secondary AMP - Edit with all settings w/o module access
Changelog entry
- Only allow users with Tag Manager access to edit Tag Manager settings in the UI.
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
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 newhasModuleAccess
prop is false".
Also, in keeping with #5429, I would suggest:
- Add default prop value of
true
for the newhasModuleAccess
props. - Return
null
instead of empty array for thehasModuleAccess === 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...
Thanks @techanvil - have updated the IB.
Thanks @jimmymadon!
IB :white_check_mark:
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:
@wpdarren the steps mentioned in the QAB are correct. I was able to reproduce it. This is the screenshot from my secondary admin dashboard:
Could you confirm your user access permissions are the same as mine below?
I'm happy to jump on a call to walk you through it.
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.
- Also checked the stories in the QAB, and all look as expected for the different scenarios.