Dashboard: only use sites the user is a member of for the omnibar and recent sites - #112479
Conversation
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~71 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~71 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~71 bytes added 📈 [gzipped]) DetailsReact components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
0f4da45 to
6e408ca
Compare
6e408ca to
f270ec5
Compare
f270ec5 to
83eb8d3
Compare
| const omnibarSite = [ routeSite, originSite, fallbackSite ].find( | ||
| ( site ) => site && isMemberOfSite( site ) | ||
| ); | ||
| const omnibarSiteId = omnibarSite?.ID ?? user?.primary_blog; |
There was a problem hiding this comment.
Is it possible that the user's primary blog is a blog they no longer have access to? Does the user need to be validated as a member as well?
There was a problem hiding this comment.
The primary blog is configured through Preferences > Account Defaults, where you can select any of your sites. However, if you choose a site that you don’t own as your primary blog, you may be removed from that site. That said, the likelihood of this happening is very low.
83eb8d3 to
bbaf07f
Compare
|
I'm not against running the two queries simultaneously, but I'm always hesitant to do so when we need to resolve a winner. Membership feels like an edge case that shouldn't introduce delays for regular users. It may be better to just catch and move on. Here's a quick prototype pr: #112495 (Disclaimer, AI generated) Thoughts? |
| onToggleNotifications, | ||
| }: Props ) { | ||
| const user = userProp ?? emptyUser; | ||
| const siteId = user.primary_blog ?? null; |
There was a problem hiding this comment.
I'm surprised then how come it's working until now ??
| import { AUTH_QUERY_KEY } from '../auth'; | ||
| import type { Site, User } from '@automattic/api-core'; | ||
|
|
||
| function isMemberOfSite( site: Site ) { |
There was a problem hiding this comment.
Nit: maybe add this as canViewSite() in client/dashboard/sites/features.ts so that it's paired nicely with canManageSite() in that file?
There was a problem hiding this comment.
but can't any one view the site 🤔
There was a problem hiding this comment.
Hmm... that's right... maybe isMemberOfSite() is clearer after all 😅
There was a problem hiding this comment.
yes, but it doesn't seem to match functions in this file... so I just inlined it 🙈
Validate site membership before persisting a site as the most recent site or setting it as the omnibar site, so a crafted or inaccessible `origin_site_id` can no longer be recorded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bbaf07f to
9befd26
Compare
fushar
left a comment
There was a problem hiding this comment.
Tested the cases. Works, thanks!
Proposed Changes
origin_site_idURL param, then a fallback (last omnibar site → most recent → primary blog) — but only accept a candidate the user is actually a member of.recentSitespreference.Why are these changes being made?
The dashboard set and recorded whatever site id it resolved — including one taken straight from the untrusted
origin_site_idURL param — with no check that the site belongs to the user. Classic Calypso filtered recent sites against the user's own sites; the dashboard path dropped that guard, so a crafted or stale id could be persisted and then fail to load. Membership is now the gate (any role, not just admin), matching the classic behavior.Testing Instructions
?origin_site_id=<id of a site you do not belong to>and confirm it is not set as the omnibar site or added to recents, the param is stripped from the URL, and the omnibar still initializes from a valid fallback.origin_site_idand confirm the omnibar still initializes (no hang).Pre-merge Checklist
Considerations
Membership is detected via the presence of the site's
capabilitiesproperty fromGET /sites/:id(a localisMemberOfSitehelper). This includes all member roles — broader than an admin-onlymanage_optionscheck — and reliably excludes private sites the user cannot access, since that lookup errors. One edge remains unverified: whether the endpoint returnscapabilitiesfor a public site the user has no role on. If it does, such a site could still be recorded; impact is limited to the user's own preference.