Site-kit-wp: Deep links don't support secondary Google accounts

[aaemnnosttv]

Bug Description

Google allows for multiple accounts to be signed-in simultaneously.

When a user is logged-in to multiple accounts at once, links to Google services assume your "default user" if it is not otherwise defined in the url, usually as an authuser=x query parameter (where x is an integer starting at 0).

This causes our deep links (i.e. linking to a specific Analytics view or search console report) to fail if the Google user authenticated with Site Kit is not the user's default user.

The index to use for authuser is unknown to us, and does not appear to be exposed via any API. However, some services are known to support using the user's email address for this parameter instead.

Steps to reproduce

1. Sign out of all Google Accounts
2. Sign in to account A without access to a Search Console property ( this is considered the primary account )
3. Sign in to account B with access to a Search Console property
4. Set up Site Kit using account B
5. Go to the Site Kit dashboard
6. Click on the first link under "Top search queries for your site"
7. See error that your user does not have access to this property

---

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

Acceptance criteria

• Direct "deep" links to Google resources should always include the authuser query parameter which is set to the user's email address from their profile (user option googlesitekit_profile). All Google APIs we use primarily rely on the authuser parameter except:
  
  
  
  • AdSense, which however support a similar mechanism by including a URL segment like /u/0 (for example https://www.google.com/adsense/new/u/0/pub-1234567890/home) - this can also be /u/[email protected], in which case it will automatically handle the correct user.
  
  
  • PageSpeed Insights does not have any user association, so can be ignored entirely here.
  
  
• There should be a new selector getServiceURL( args = {} ), where args supports optional fields path (should be sanitized to start with forward slash) and query (should be a map that will be sanitized into a query string), on the following datastores (email in the references below would be the user's email from core/user store):
  
  
  
  • modules/analytics: Should return https://analytics.google.com/analytics/web/?authuser={email}#{path}{query}
  
  
  • modules/tagmanager: Should return https://tagmanager.google.com/?authuser={email}#{path}{query}
  
  
  • modules/optimize: Should return https://optimize.google.com/optimize/home/?authuser={email}#{path}{query}
  
  
  • modules/search-console: Should return https://search.google.com/search-console{path}{query} - query includes authuser={$email}
  
  
  • modules/adsense: Should return https://www.google.com/adsense/new/u/{email}{path}/{query}
  
  
  • modules/pagespeed-insights: Should return https://developers.google.com/speed/pagespeed/insights{path}{query}
  
  
  • If path or query are empty, they shouldn't be sanitized, i.e. the returned URL shouldn't include any single ? segment or so.
  
  

Implementation Brief

• Create a new selector for each of the 6 modules called getServiceURL following the AC for implementation details in regards to the parameters and validation of them.
  
  
  
  • The selector should be able to access the core/user datastore to retrieve the userEmail
    
    
    
    
    
    • Refer to the AC's for how each of the 6 URLs is formatted.
    
    
    
  
  
• assets/js/modules/search-console/dashboard/dashboard-widget-keyword-table.js
  
  
  
  • Convert class component to functional component but continue to pass component to withData HOC as there is not a datastore equivalent.
  
  
  • Generate the links using the getServiceURL selector passing the appropriate path and query items as the args parameter
  
  
• assets/js/modules/search-console/dashboard/dashboard-widget-popular-keyword-table.js
  
  
  
  • Convert class component to functional component but continue to pass component to withData HOC as there is not a datastore equivalent.
  
  
  • Generate the links using the getServiceURL selector passing the appropriate path and query items as the args parameter
  
  
• assets/js/modules/analytics/dashboard/dashboard-widget-top-pages-table.js
  
  
  
  • Convert class component to functional component but continue to pass component to withData HOC as there is not a datastore equivalent.
    
    *Generate the links using the getServiceURL selector passing the appropriate path and query items as the args parameter
  
  
  • Complete and merge #1789
  
  

QA Brief

Steps to reproduce issue:

1. Sign out of all Google Accounts
2. Sign in to account A without access to a Search Console property ( this is considered the primary account )
3. Sign in to account B with access to a Search Console property
4. Set up Site Kit using account B
5. Go to the Site Kit dashboard
6. Click on the first link under "Top search queries for your site"
7. See error that your user does not have access to this property
8. In the Analytics tab, click on any links in Top Content.
9. See error that your user does not have access to this property

After building this branch, none be sure to log out of all accounts and log in again. These errors should not occur.

Since the PR touches several areas, we should also thoroughly test a few other areas especially. For those, the focus should be on ensuring there's no breakage - there are no specific AC criteria that these need to fulfill:

• The AdSense setup flow (all different states, via QA Tester plugin).
• The Analytics provisioning flow (create new account).
• On the Site Kit dashboard and module pages: All "source" links and links on e.g. URL or page lists that point to a Google service frontend should be valid. We don't need to look in detail at the exact URLs here, but we should make sure there are no broken URLs.

Changelog entry

• Consistently enhance deep links to Google services to support users logged into multiple Google accounts in their browser.

[tofumatt]

The only place I can find us not using a user account email we're using a user ID: https://github.com/google/site-kit-wp/blob/c3c07b37452c70ac382e3eeb174acd0b24adc135/includes/Modules/AdSense.php#L429 + https://github.com/google/site-kit-wp/blob/c3c07b37452c70ac382e3eeb174acd0b24adc135/includes/Modules/AdSense.php#L579.

It looks like the last place we were using /u/0 was changed more recently than this issue: 4875cd0b920ea53cff3eb31951e1db94a873e7e0. I wasn't able to replicate the error listed here, so I'm curious to know if this is still a valid issue. Could someone else recreate it? Because I can't find any places we're using /u/0. I think this can be closed as fixed.

[felixarntz]

@tofumatt This issue is still relevant, it's not only about removing /u/0 usage. The AdSense account ID used in the URLs you referenced is correct, but it's also not really relevant for this issue (AdSense account != Google account), maybe that was unclear.

Essentially, all deep links that link to a Google service frontend should include a parameter to indicate the current Google user account, which we can do using the Google account email that is stored for every user in WordPress. This will ensure that users that have multiple Google accounts (and may be using another account as the primary one, or they could currently not be logged in with the correct account at all) land in the right place: For example:

• User has two Google accounts A and B. Account A is their primary account they're logged in with.
• User signs in to Site Kit with account B, and the new Search Console property set up can only be accessed by account B.
• User clicks external link to see more details in Search Console.
  
  
  
  • Right now, they would land in the UI for account A, which doesn't have access to the deep-linked property, so they run into an access error in the UI.
  
  
  • After this issue, they would land in the UI for account B (because the URL indicated that account), so they would see the expected UI.
  
  

Basically we will need to inspect all external links to Google service frontends in Site Kit to specify:
* either an authuser=EMAILADDRESS query parameter
* or a /u/EMAILADDRESS path segment (only for Google service frontends where the authuser parameter is not supported, we will need to test this)

[tofumatt]

This one has me a bit stumped, because I don't seem to have a collection of accounts with actual Search Console data to load and compare with. I tend to test things with one account, so I'm not able to properly define the IB here because I can't get an exhaustive list of outbound links to appear with the issue. Unassigning myself for now in the hopes that someone else has the right number of accounts to note all of the places the "default" account is used in outbound links.

[aaemnnosttv]

@ryanwelcher the IB looks good to me for the most part, just a few points to clarify

• By useData I think you mean withData?
• I think the "Close issue as this is fixed" bullet can probably be removed 😄

While this would fix the problem, although I feel like we should see about making this a little easier going forward by introducing a new selector to streamline this a bit, so we wouldn't need to do it manually in each instance. Something like getServiceURL or getDeepURL as a registry selector that constructs the module's service URL including the authuser part. It would only need to take the path and maybe queryArgs as optional parameters.

Thoughts @felixarntz ?

[felixarntz]

@aaemnnosttv @ryanwelcher I've updated the ACs based on the suggestion to introduce store selectors (named them getServiceBaseURL) for the base URLs, which makes this more future-proof and sets the correct baseline for other modules we have too.

I've double-checked how all of our module services deal with the identifying parameter and provided the details in the ACs on what these selectors should return. While they are 6 selectors, implementing them should be trivial, so it probably wouldn't bring up the estimate here too much. The IB of course needs to be amended due to these changes though.

[ryanwelcher]

@felixarntz as per slack, we've discovered that using the /u/{email} approach for search-console only works if the user is logged into more than one account. As discussed, I'll update the AC's to indicate thatsearch-console should be passed the authuser query param which works in either case.

Adsense functions as expected so no change is required.

[felixarntz]

@ryanwelcher IB ✅

Except the selector name should be getServiceBaseURL (not "based") :)

[aaemnnosttv]

@felixarntz would it not be more accurate to name getServiceURL since it will give you a full URL back to something other than the base given additional args? It would only return the base URL by default, but most of the time we would be calling it to generate a more specific URL. Similar to home_url( $path ) or site_url( $path ), this is essentially service_url( $args ) with more flexibility for the building up the URL 🙂

[felixarntz]

@aaemnnosttv That's a fair point. I was thinking though that getServiceURL could be the more commonly used one where we e.g. link to the right GA view etc. Like deep links to GA would pretty much always include the active account/property/view segment, deep links to SC would pretty much always include the resource_id query parameter for the SC property, etc.

If we renamed the current getServiceBaseURL to getServiceURL, what would be good names for those more specific, but at the same time more commonly used selectors for service URLs? I'm not opposed to doing that, but we should think about what that would mean for those selectors.

[ryanwelcher]

Diving into this further and it looks like there will be some additional refactoring needed for some of the table-based components.

For example, in SearchConsoleDashboardWidgetKeywordTable, data is passed as a prop and is looped to create the links. This will violate the rules of hooks as we can't guarantee that the data prop is the same length and as such could cause the component to error out.

I think we should update the getDataTableFromData component to wrap other components to build the rows rather than build them internally using data.

With these additional changes, we may go over slightly on the estimate.

[felixarntz]

@ryanwelcher For such cases, how about we build the base URL without the query argument just once before the loop, and then in the loop add the extra query arg via the addQueryArgs?

[ryanwelcher]

@felixarntz that is possible, yes. However, it would be nice to clean up the approach in getDataTableFromData while we're in this section of code to make it more readable and use the new selector as it was intended.

[felixarntz]

@ryanwelcher That's a fair point, could you open a separate issue for that and add to Prioritization though? I think for this issue specifically we can easily get away with the simpler change, and I'd say it's still using the new selector for its primary purpose.

[ryanwelcher]

@felixarntz sounds great. I'll move forward with your suggestion, thanks!

[aaemnnosttv]

@felixarntz

I was thinking though that getServiceURL could be the more commonly used one where we e.g. link to the right GA view etc.

I think these selectors might use something like getServiceDashboardURL if generic, or something more specific to Analytics could be getServiceReportHomeURL?

For Search Console, maybe getServicePropertyURL or getServiceResourceURL?

[felixarntz]

@aaemnnosttv I was originally thinking it would be great to have a generic name like getServiceDashboardURL or getServiceAccountURL (although the identifying information isn't technically always called an account) or similar, but it may just have to be required to do this more specifically per module, since the URLs work so differently.

Let's go with getServiceURL for this one here like you suggested, and figure out the exact other behavior later.

@ryanwelcher See above, let's use getServiceURL instead of getServiceBaseURL for all selectors here.

[ryanwelcher]

let's use getServiceURL instead of getServiceBaseURL for all selectors here.

I've updated the AC and IB to reflect the change.

[aaemnnosttv]

@ryanwelcher you still have a few references to getServiceBasedURL 😄

[felixarntz]

There's an issue introduced by this one when Analytics returns 0 data:

[tofumatt]

QA ✅