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

Shared requests should not be attempted for recoverable modules #5383

Closed
aaemnnosttv opened this issue Jun 16, 2022 · 6 comments
Closed

Shared requests should not be attempted for recoverable modules #5383

aaemnnosttv opened this issue Jun 16, 2022 · 6 comments
Labels
P0 High priority PHP Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jun 16, 2022

Bug Description

If a request for module data which is shared by its owner happens to be for a module which is also recoverable, it should not be attempted although this is currently what's happening.

Steps to reproduce

  1. Connect with Admin A
  2. Share Search Console with Admins
  3. Disconnect Admin A
  4. Connect with Admin B
  5. See data errors about lack of granted scopes, even though the current user has the scopes

Screenshots

image


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

Acceptance criteria

  • Module data requests for recoverable modules should not qualify to be executed as a shared request

Implementation Brief

  • Define a new public Module::is_recoverable method, similar to is_shareable
  • Return a boolean from a new filter googlesitekit_is_module_recoverable which defaults to false and passes through its slug as the only extra parameter
  • Update Modules to hook into this filter and run the callback through its is_module_recoverable method
  • Update the condition in Module::get_oauth_client_for_datapoint to additionally require the module is not recoverable (using the new method) in order to return the owner oauth client
    • Additionally, ensure the owner's client satisfies the base scopes (using validate_base_scopes) as a final check before returning, otherwise falling back to the default client
  • Finish and merge Update oauth client qualification to consider module recoverability and scopes #5384

Test Coverage

  • Add test for added is_recoverable method

QA Brief

  • Follow steps to reproduce above before and after to ensure the request works as expected when the current admin has the granted scopes if the module is recoverable

Changelog entry

  • Do not attempt to make requests for module data where the module is shared and also recoverable.
@felixarntz
Copy link
Member

IB ✅

@techanvil techanvil assigned techanvil and unassigned techanvil Jun 21, 2022
@mohitwp mohitwp self-assigned this Jun 21, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 22, 2022

QA Update ❌

@aaemnnosttv @techanvil

Verified

  • Reproduced error successfully on main branch.
  • Now, data errors about lack of granted scopes is not showing on the development branch.
  • User is getting option to recover.

ISSUE

  • After clicking on recover button when Admin 2 opens Dashboard sharing modal then it shows that Admin 2 don't have manage view access.
  • After refreshing the dashboard, modal gets update and admin 2 gets access.
  • Expected : Manage access must get update immediately after recovery of account.

Before Refreshing the Dashboard :
image

After refreshing the Dashboard:
image

Recording.114.mp4

@mohitwp mohitwp assigned aaemnnosttv and unassigned mohitwp Jun 22, 2022
@aaemnnosttv
Copy link
Collaborator Author

@mohitwp this issue would have no effect on the sharing settings interface it only changes how requests are executed on the backend.

@techanvil did something change around module recovery where the owner is no longer being updated?

@aaemnnosttv aaemnnosttv assigned mohitwp and unassigned aaemnnosttv Jun 22, 2022
@techanvil
Copy link
Collaborator

@techanvil did something change around module recovery where the owner is no longer being updated?

Nothing that I am aware of, I just tried recovering a module locally and the owner ID updated OK.

@aaemnnosttv
Copy link
Collaborator Author

After taking a closer look here, the problem is that while the module's owner is properly updated, the recovering user's capabilities are not updated to reflect this. This is why the settings show properly after a page refresh but not immediately after recovering the module.

I've opened #5416 to address this. @mohitwp as mentioned before this is a related bug but caused by this issue. Thanks for raising it 👍

@mohitwp
Copy link
Collaborator

mohitwp commented Jun 23, 2022

Thank you @aaemnnosttv

QA Update ✅

Verified

  • Reproduced error successfully on main branch.
  • Now, data errors about lack of granted scopes is not showing on the development branch.
  • User is getting option to recover.
  • Issue mentioned here will get resolve by
    #5416
Recording.114.mp4

@mohitwp mohitwp removed their assignment Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority PHP Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants