Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor capabilities as they apply to dashboard sharing settings #5229

Closed
aaemnnosttv opened this issue May 18, 2022 · 3 comments
Closed

Refactor capabilities as they apply to dashboard sharing settings #5229

aaemnnosttv opened this issue May 18, 2022 · 3 comments
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented May 18, 2022

Feature Description

In #4481, capability checks were added to restrict which parts of the module sharing settings can be modified. This logic, while largely correct should be moved out of the module sharing settings class itself and live either in the controller or somewhere in between. Secondarily, the capability checks should be extended such that only users who can DELEGATE_MODULE_SHARING_MANAGEMENT can modify the management value for a module's sharing settings.


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

Acceptance criteria

  • No capability checks should be performed within the Module_Sharing_Settings class
    • These should instead happen within the REST endpoint handler for POST:sharing-settings as the only place where these are managed from Site Kit.
  • The application of these capability checks should be done in a way that can be easily applied elsewhere without needing to duplicate/reimplement the logic
  • Specifically the two capability checks should work as follows:
    • If the user does not have the ability to MANAGE_MODULE_SHARING_OPTIONS for a module, no change should be made to that module's sharedRoles value
    • If the user does not have the ability to DELEGATE_MODULE_SHARING_MANAGEMENT for a module, no change should be made to that module's management value

Implementation Brief

  • Introduce a new Collection_Key_Cap_Filter utility class which is designed to filter a specific key of each item in a given collection based on a capability
    • The array key of each item in the array will be passed to the capability check as an argument for meta capability checks
  • Apply the new key-cap filter to the incoming sharing settings in the POST:sharing-settings REST route definition before merging with the current sharing settings
    • One filter will be for the sharedRoles key, requiring the MANAGE_MODULE_SHARING_OPTIONS capability
    • One filter will be for the management key, requiring the DELEGATE_MODULE_SHARING_MANAGEMENT capability
    • Finally, merge the filtered result with the current sharing settings
  • Update the implementation of Module_Sharing_Settings::merge to match Module_Settings::merge
  • Finish and merge Move module sharing settings capability filtering to REST controller #5259

Test Coverage

  • As a generic utility class, simple tests can be added for Collection_Key_Cap_Filter
  • Current permission-related tests for Module_Sharing_Settings::merge should be moved to the REST_Dashboard_Sharing_ControllerTest, but only where current coverage is lacking

QA Brief

QA

  • Create an admin user, say "admin1" and setup SiteKit but do not setup analytics. This makes "admin1" the owner of search-console.
  • Create another admin user, say "admin2" and setup analytics making them the owner of analytics.
  • Within the Dashboard Sharing UI, verify that both, "who can view" and "who can manage access" fields are editable by the owner of the module and setting changes take effect.
  • Also verify that an admin can change the "who can view" field if the owner of that module has set the "who can manage access" setting to All Admins. Otherwise, no other admin should be able to change this setting.
  • Finally, verify that only the owner of a module can change the "who can manage access" setting. No other user can change this setting.
  • QA:Eng
    • Test the core/modules/data/sharing-settings with a API client or browser console with valid and invalid data and make sure AC is met.

Changelog entry

  • Ensure only users with the DELEGATE_MODULE_SHARING_MANAGEMENT permission can modify the management value for a module's sharing settings.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Enhancement Improvement of an existing feature labels May 18, 2022
@aaemnnosttv aaemnnosttv self-assigned this May 18, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment May 25, 2022
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 May 25, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil May 26, 2022
@wpdarren wpdarren added the QA: Eng Requires specialized QA by an engineer label May 27, 2022
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Jun 6, 2022
@jimmymadon jimmymadon self-assigned this Jun 7, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 8, 2022
@jimmymadon jimmymadon removed the QA: Eng Requires specialized QA by an engineer label Jun 8, 2022
@jimmymadon jimmymadon removed their assignment Jun 8, 2022
@tofumatt tofumatt assigned tofumatt and jimmymadon and unassigned tofumatt Jun 8, 2022
@jimmymadon jimmymadon assigned tofumatt and unassigned jimmymadon Jun 9, 2022
@tofumatt tofumatt removed their assignment Jun 13, 2022
@wpdarren wpdarren self-assigned this Jun 14, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jun 14, 2022

QA Update: ✅

Verified:

  • Within the Dashboard Sharing UI, both, "who can view" and "who can manage access" fields are editable by the owner of the module and setting changes take effect.
  • An admin can change the "who can view" field if the owner of that module has set the "who can manage access" setting to All Admins. Otherwise, no other admin can change this setting.
  • Only the owner of a module can change the "who can manage access" setting. No other user can change this setting.
Screenshots

image
image
image
image

Added QA:Eng due to the tests in QAB required.

@wpdarren wpdarren removed their assignment Jun 14, 2022
@wpdarren wpdarren added the QA: Eng Requires specialized QA by an engineer label Jun 14, 2022
@jimmymadon jimmymadon self-assigned this Jun 14, 2022
@jimmymadon
Copy link
Collaborator

QA: Eng ✅

Tested with the QAB of #4481.

Verified failed requests when feature flag is off, for GET request and POST request without body params. Screenshot 2022-06-16 at 16 42 45 Screenshot 2022-06-16 at 16 45 06 Screenshot 2022-06-16 at 16 46 09
Verified successful adding and editing of settings along with changing owner of PSI when settings are changed with another admin. Screenshot 2022-06-16 at 16 44 16 Screenshot 2022-06-16 at 16 51 38 Screenshot 2022-06-16 at 16 49 13

@jimmymadon jimmymadon removed their assignment Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants