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

Site Kit can still show data after revoking access

Open techanvil opened this issue 2 years ago • 6 comments

Bug Description

When revoking access via User menu > Manage sites > Remove access, if the user doesn't press the Back button, data is still available in Site Kit.

Steps to reproduce

  1. With Site Kit connected, navigate to User menu > Manage sites > Remove access.
  2. Press the Remove access button for the site.
  3. Instead of pressing the Back button on the page, navigate back to Site Kit via the browser's back button or address bar.
  4. Data is still visible on the page despite access being revoked.

Screenshots

Actual

image

Expected

image

Additional Context

  • PHP Version: any
  • OS: any
  • Browser: any
  • Plugin Version: 1.72.0
  • Device: any

Thanks to @jamesozzie for his comment below, I can confirm that data is no longer visible once the access token has expired/refreshed and the browser cache (session storage) has been cleared. Instead the "Error: Looks like your site is not allowed access to Google account data" error message is visible as expected (see screenshots above).


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

techanvil avatar Apr 26 '22 11:04 techanvil

@techanvil I did some testing on this recently and it may be related to caching. See the notes within the QA brief in another issue (https://github.com/google/site-kit-wp/issues/1646)

jamesozzie avatar Apr 26 '22 11:04 jamesozzie

@techanvil I did some testing on this recently and it may be related to caching. See the notes within the QA brief in another issue (#1646)

Thanks @jamesozzie, that's very helpful to know and I can confirm that with a refreshed access token and having cleared session storage, data is no longer visible and an error is shown instead, as expected. I've made a note of this on the description. Cheers

techanvil avatar Apr 26 '22 12:04 techanvil

@techanvil this is expected as the SK service does not invalidate any access tokens when removing access – it's essentially removing your permission to delegate access tokens to the site. So any access token the site still has will continue to work until it expires (max 1hr) after which the token refresh will fail because the service no longer has permission to delegate tokens to it.

aaemnnosttv avatar Aug 02 '22 15:08 aaemnnosttv

Hi @aaemnnosttv, thanks for clarifying that. It's good to know this is expected behaviour on a technical level.

It still feels a bit confusing to me from a user perspective, pressing Remove access and then navigating back via the browser and seeing the site still active. Especially when then comparing that to pressing the Back CTA on the page which navigates back and disconnects.

Not sure if it's worth adding some clarification about this to the descriptive text on the page, or maybe Back could be relabelled, or if that would simply over-complicate the user-facing text for the sake of what is probably a fairly niche edge case...

techanvil avatar Aug 08 '22 15:08 techanvil

@techanvil I hear you, I'm just not sure what could actually be done to accomplish the same thing if a user were to navigate back rather than use the button. I feel like we're probably limited to solving this on the service side with some more clarification around what to expect.

Even the Back button will only disconnect the current site you came from (if you revoked it). Any other sites that had their access removed would still have the delay if the user already had an active token for the site.

Short of storing tokens per-user, per-site and revoking them on this action, I don't think it's possible, but even then the data may still show for some time due to the browser cache if the user navigated back in their history. AFAIK the service doesn't store tokens at all, correct @felixarntz ?

aaemnnosttv avatar Aug 09 '22 03:08 aaemnnosttv

@aaemnnosttv yup I can see it being problematic to get the behavioural parity between the button vs browser navigation... Probably solvable but only by modding the service and adding extra requests to validate the token(s) which would be overkill for such a niche item.

Really I'm just wondering if it matters enough to surface to users as a piece of info...

techanvil avatar Aug 09 '22 16:08 techanvil

@techanvil While this is a valid concern, we recently discussed this and decided to rather change language on the service side to e.g. clarify that cached data may still show up in the plugin dashboard for up to 1 hour. While we could in principle address this somehow in Site Kit, I think it's not worth the effort, and updating the messaging should be sufficient. cc @aaemnnosttv

felixarntz avatar Sep 21 '22 17:09 felixarntz