Browse Source

fix(pageFilters): Validate project IDs & env names (#57977)

Check if project IDs & envs from local storage/query params are valid
when initializing page filters.

**Before ——** if there's an invalid project ID (e.g. project was
deleted), page filters will initialize with an error
<img width="1065" alt="Screenshot 2023-10-12 at 8 26 13 AM"
src="https://github.com/getsentry/sentry/assets/44172267/e890b330-30b6-45c3-9fb4-4609eb0ab674">

**After ——** during initialization, invalid projects are removed from
local storage/query params. The page loads successfully.
<img width="1065" alt="Screenshot 2023-10-12 at 8 26 55 AM"
src="https://github.com/getsentry/sentry/assets/44172267/84f3ad50-eb59-4f19-9c5c-3dfa9bf365d9">
Vu Luong 1 year ago
parent
commit
04f2507010

+ 30 - 15
static/app/actionCreators/pageFilters.spec.tsx

@@ -1,5 +1,4 @@
-import {Organization} from 'sentry-fixture/organization';
-
+import {initializeOrg} from 'sentry-test/initializeOrg';
 import {act} from 'sentry-test/reactTestingLibrary';
 
 import {
@@ -16,9 +15,14 @@ import localStorage from 'sentry/utils/localStorage';
 
 jest.mock('sentry/utils/localStorage');
 
-describe('PageFilters ActionCreators', function () {
-  const organization = Organization();
+const {organization} = initializeOrg({
+  projects: [
+    {id: '1', slug: 'project-1', environments: ['prod', 'staging']},
+    {id: '2', slug: 'project-2', environments: ['prod', 'stage']},
+  ],
+});
 
+describe('PageFilters ActionCreators', function () {
   beforeEach(function () {
     jest.spyOn(PageFiltersStore, 'updateProjects');
     jest.spyOn(PageFiltersStore, 'onInitializeUrlState').mockImplementation();
@@ -53,7 +57,8 @@ describe('PageFilters ActionCreators', function () {
         organization,
         queryParams: {},
         router,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
       });
 
@@ -84,7 +89,8 @@ describe('PageFilters ActionCreators', function () {
         organization,
         queryParams: {},
         skipLoadLastUsed: true,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         router,
       });
@@ -107,7 +113,8 @@ describe('PageFilters ActionCreators', function () {
         queryParams: {},
         shouldPersist: false,
         router,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
       });
 
@@ -143,7 +150,8 @@ describe('PageFilters ActionCreators', function () {
         queryParams: {
           project: '1',
         },
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         router,
       });
@@ -167,7 +175,8 @@ describe('PageFilters ActionCreators', function () {
         queryParams: {
           project: '1',
         },
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         defaultSelection: {
           datetime: {
@@ -200,7 +209,8 @@ describe('PageFilters ActionCreators', function () {
           statsPeriod: '1h',
           project: '1',
         },
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         defaultSelection: {
           datetime: {
@@ -232,7 +242,8 @@ describe('PageFilters ActionCreators', function () {
           end: '2020-04-21T00:53:38',
           project: '1',
         },
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         defaultSelection: {
           datetime: {
@@ -263,7 +274,8 @@ describe('PageFilters ActionCreators', function () {
         queryParams: {
           project: '1',
         },
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         router,
       });
@@ -313,7 +325,8 @@ describe('PageFilters ActionCreators', function () {
         organization,
         queryParams: {},
         router,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
       });
 
@@ -344,7 +357,8 @@ describe('PageFilters ActionCreators', function () {
         organization,
         queryParams: {},
         router,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
       });
 
@@ -377,7 +391,8 @@ describe('PageFilters ActionCreators', function () {
         organization,
         queryParams: {},
         router,
-        memberProjects: [],
+        memberProjects: organization.projects,
+        nonMemberProjects: [],
         shouldEnforceSingleProject: false,
         storageNamespace: 'starfish',
       });

+ 59 - 10
static/app/actionCreators/pageFilters.tsx

@@ -115,6 +115,7 @@ function mergeDatetime(
 
 export type InitializeUrlStateParams = {
   memberProjects: Project[];
+  nonMemberProjects: Project[];
   organization: Organization;
   queryParams: Location['query'];
   router: InjectedRouter;
@@ -160,6 +161,7 @@ export function initializeUrlState({
   queryParams,
   router,
   memberProjects,
+  nonMemberProjects,
   skipLoadLastUsed,
   skipLoadLastUsedEnvironment,
   shouldPersist = true,
@@ -199,15 +201,54 @@ export function initializeUrlState({
   const hasProjectOrEnvironmentInUrl =
     Object.keys(pick(queryParams, [URL_PARAM.PROJECT, URL_PARAM.ENVIRONMENT])).length > 0;
 
+  // We should only check and update the desync state if the site has just been loaded
+  // (not counting route changes). To check this, we can use the `isReady` state: if it's
+  // false, then the site was just loaded. Once it's true, `isReady` stays true
+  // through route changes.
+  let shouldCheckDesyncedURLState = !PageFiltersStore.getState().isReady;
+
+  /**
+   * Check to make sure that the project ID exists in the projects list. Invalid project
+   * IDs (project was deleted/moved to another org) can still exist in local storage or
+   * shared links.
+   */
+  function validateProjectId(projectId: number) {
+    return (
+      !!memberProjects?.find(mp => String(mp.id) === String(projectId)) ||
+      !!nonMemberProjects?.find(nmp => String(nmp.id) === String(projectId))
+    );
+  }
+
+  /**
+   * Check to make sure that the environment exists. Invalid environments (due to being
+   * hidden) can still exist in local storage or shared links.
+   */
+  function validateEnvironment(env: string) {
+    return (
+      !!memberProjects?.find(mp => mp.environments.includes(env)) ||
+      !!nonMemberProjects?.find(nmp => nmp.environments.includes(env))
+    );
+  }
+
   if (hasProjectOrEnvironmentInUrl) {
-    pageFilters.projects = parsed.project || [];
-    pageFilters.environments = parsed.environment || [];
+    pageFilters.projects = parsed.project?.filter(validateProjectId) || [];
+    pageFilters.environments = parsed.environment?.filter(validateEnvironment) || [];
+
+    if (
+      pageFilters.projects.length < (parsed.project?.length ?? 0) ||
+      pageFilters.environments.length < (parsed.environment?.length ?? 0)
+    ) {
+      // don't check desync state since we're going to remove invalid projects/envs from
+      // the URL query
+      shouldCheckDesyncedURLState = false;
+    }
   }
 
   const storedPageFilters = skipLoadLastUsed
     ? null
     : getPageFilterStorage(orgSlug, storageNamespace);
   let shouldUsePinnedDatetime = false;
+  let shouldUpdateLocalStorage = false;
 
   // We may want to restore some page filters from local storage. In the new
   // world when they are pinned, and in the old world as long as
@@ -216,7 +257,12 @@ export function initializeUrlState({
     const {state: storedState, pinnedFilters} = storedPageFilters;
 
     if (!hasProjectOrEnvironmentInUrl && pinnedFilters.has('projects')) {
-      pageFilters.projects = storedState.project ?? [];
+      pageFilters.projects = storedState.project?.filter(validateProjectId) ?? [];
+
+      if (pageFilters.projects.length < (storedState.project?.length ?? 0)) {
+        shouldUpdateLocalStorage = true; // update storage to remove invalid projects
+        shouldCheckDesyncedURLState = false;
+      }
     }
 
     if (
@@ -224,7 +270,13 @@ export function initializeUrlState({
       !hasProjectOrEnvironmentInUrl &&
       pinnedFilters.has('environments')
     ) {
-      pageFilters.environments = storedState.environment ?? [];
+      pageFilters.environments =
+        storedState.environment?.filter(validateEnvironment) ?? [];
+
+      if (pageFilters.environments.length < (storedState.environment?.length ?? 0)) {
+        shouldUpdateLocalStorage = true; // update storage to remove invalid environments
+        shouldCheckDesyncedURLState = false;
+      }
     }
 
     if (!hasDatetimeInUrl && pinnedFilters.has('datetime')) {
@@ -276,13 +328,10 @@ export function initializeUrlState({
     ? new Set<PinnedPageFilter>(['projects', 'environments', 'datetime'])
     : storedPageFilters?.pinnedFilters ?? new Set();
 
-  // We should only check and update the desync state if the site has just been loaded
-  // (not counting route changes). To check this, we can use the `isReady` state: if it's
-  // false, then the site was just loaded. Once it's true, `isReady` stays true
-  // through route changes.
-  const shouldCheckDesyncedURLState = !PageFiltersStore.getState().isReady;
-
   PageFiltersStore.onInitializeUrlState(pageFilters, pinnedFilters, shouldPersist);
+  if (shouldUpdateLocalStorage) {
+    setPageFiltersStorage(organization.slug, new Set(['projects', 'environments']));
+  }
 
   if (shouldCheckDesyncedURLState) {
     checkDesyncedUrlState(router, shouldForceProject);

+ 1 - 0
static/app/components/organizations/datePageFilter.spec.tsx

@@ -142,6 +142,7 @@ describe('DatePageFilter', function () {
     PageFiltersStore.reset();
     initializeUrlState({
       memberProjects: [],
+      nonMemberProjects: [],
       organization: desyncOrganization,
       queryParams: {statsPeriod: '14d'},
       router: desyncRouter,

+ 2 - 1
static/app/components/organizations/environmentPageFilter/index.spec.tsx

@@ -159,7 +159,8 @@ describe('EnvironmentPageFilter', function () {
 
     PageFiltersStore.reset();
     initializeUrlState({
-      memberProjects: [],
+      memberProjects: organization.projects,
+      nonMemberProjects: [],
       organization: desyncOrganization,
       queryParams: {project: ['1'], environment: 'staging'},
       router: desyncRouter,

+ 6 - 1
static/app/components/organizations/pageFilters/container.spec.tsx

@@ -34,6 +34,10 @@ describe('PageFiltersContainer', function () {
   const {organization, router, routerContext} = initializeOrg({
     organization: {features: ['global-views']},
     projects: [
+      {
+        id: '1',
+        slug: 'project-1',
+      },
       {
         id: '2',
         slug: 'project-2',
@@ -352,6 +356,7 @@ describe('PageFiltersContainer', function () {
       organization: {
         features: ['global-views'],
       },
+      projects: [TestStubs.Project({id: 1}), TestStubs.Project({id: 2})],
       router: {
         // we need this to be set to make sure org in context is same as
         // current org in URL
@@ -915,7 +920,7 @@ describe('PageFiltersContainer', function () {
         // forceProject generally starts undefined
         const {rerender} = renderForGlobalView(
           {shouldForceProject: true},
-          changeQuery(initialData.routerContext, {project: 321})
+          changeQuery(initialData.routerContext, {project: 2})
         );
 
         rerender({forceProject: initialData.projects[1]});

+ 11 - 5
static/app/components/organizations/pageFilters/container.tsx

@@ -26,7 +26,11 @@ import {extractSelectionParameters} from './utils';
 
 type InitializeUrlStateProps = Omit<
   InitializeUrlStateParams,
-  'memberProjects' | 'queryParams' | 'router' | 'shouldEnforceSingleProject'
+  | 'memberProjects'
+  | 'nonMemberProjects'
+  | 'queryParams'
+  | 'router'
+  | 'shouldEnforceSingleProject'
 >;
 
 interface Props extends InitializeUrlStateProps {
@@ -95,6 +99,9 @@ function Container({
   const memberProjects = user.isSuperuser
     ? specifiedProjects
     : specifiedProjects.filter(project => project.isMember);
+  const nonMemberProjects = user.isSuperuser
+    ? []
+    : specifiedProjects.filter(project => !project.isMember);
 
   const doInitialization = () => {
     initializeUrlState({
@@ -104,6 +111,7 @@ function Container({
       skipLoadLastUsed,
       skipLoadLastUsedEnvironment,
       memberProjects,
+      nonMemberProjects,
       defaultSelection,
       forceProject,
       shouldForceProject,
@@ -122,15 +130,13 @@ function Container({
   //
   // This happens when we mount the container.
   useEffect(() => {
-    // We can initialize before ProjectsStore is fully loaded if we don't need to
-    // enforce single project.
-    if (!projectsLoaded && (shouldForceProject || enforceSingleProject)) {
+    if (!projectsLoaded) {
       return;
     }
 
     doInitialization();
     // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [projectsLoaded, shouldForceProject, enforceSingleProject]);
+  }, [projectsLoaded]);
 
   // Update store persistence when `disablePersistence` changes
   useEffect(() => updatePersistence(!disablePersistence), [disablePersistence]);

+ 3 - 2
static/app/components/organizations/projectPageFilter/index.spec.tsx

@@ -231,9 +231,10 @@ describe('ProjectPageFilter', function () {
 
     PageFiltersStore.reset();
     initializeUrlState({
-      memberProjects: [],
+      memberProjects: organization.projects.filter(p => p.isMember),
+      nonMemberProjects: organization.projects.filter(p => !p.isMember),
       organization: desyncOrganization,
-      queryParams: {project: '2'},
+      queryParams: {project: ['2']},
       router: desyncRouter,
       shouldEnforceSingleProject: false,
     });

+ 5 - 0
static/app/views/dashboards/widgetBuilder/buildSteps/visualizationStep.spec.tsx

@@ -4,6 +4,7 @@ import {initializeOrg} from 'sentry-test/initializeOrg';
 import {act, render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
 
 import {DEFAULT_DEBOUNCE_DURATION} from 'sentry/constants';
+import ProjectsStore from 'sentry/stores/projectsStore';
 import {Organization} from 'sentry/types';
 import {DashboardWidgetSource} from 'sentry/views/dashboards/types';
 import WidgetBuilder from 'sentry/views/dashboards/widgetBuilder';
@@ -87,6 +88,10 @@ describe('VisualizationStep', function () {
     },
   });
 
+  beforeEach(function () {
+    ProjectsStore.loadInitialData(organization.projects);
+  });
+
   it('debounce works as expected and requests are not triggered often', async function () {
     const {eventsMock} = mockRequests(organization.slug);
 

+ 4 - 1
static/app/views/dashboards/widgetBuilder/widgetBuilder.spec.tsx

@@ -16,6 +16,7 @@ import {
 } from 'sentry-test/reactTestingLibrary';
 
 import * as modals from 'sentry/actionCreators/modal';
+import ProjectsStore from 'sentry/stores/projectsStore';
 import TagStore from 'sentry/stores/tagStore';
 import {TOP_N} from 'sentry/utils/discover/types';
 import {
@@ -74,6 +75,8 @@ function renderTestComponent({
     },
   });
 
+  ProjectsStore.loadInitialData(organization.projects);
+
   render(
     <WidgetBuilder
       route={{}}
@@ -1068,7 +1071,7 @@ describe('WidgetBuilder', function () {
 
     expect(await screen.findByTestId('page-filter-timerange-selector')).toBeDisabled();
     expect(screen.getByTestId('page-filter-environment-selector')).toBeDisabled();
-    expect(screen.getByTestId('page-filter-project-selector-loading')).toBeDisabled();
+    expect(screen.getByTestId('page-filter-project-selector')).toBeDisabled();
     expect(mockReleases).toHaveBeenCalled();
 
     expect(screen.getByRole('button', {name: /all releases/i})).toBeDisabled();

+ 3 - 0
static/app/views/dashboards/widgetBuilder/widgetBuilderDataset.spec.tsx

@@ -13,6 +13,7 @@ import {
   within,
 } from 'sentry-test/reactTestingLibrary';
 
+import ProjectsStore from 'sentry/stores/projectsStore';
 import TagStore from 'sentry/stores/tagStore';
 import {
   DashboardDetails,
@@ -71,6 +72,8 @@ function renderTestComponent({
     },
   });
 
+  ProjectsStore.loadInitialData(organization.projects);
+
   render(
     <WidgetBuilder
       route={{}}

Some files were not shown because too many files changed in this diff