From bf87832a5b7babebe95e9e7771af7384bfcb897d Mon Sep 17 00:00:00 2001 From: Sebastian Sangervasi Date: Fri, 21 Oct 2022 15:55:43 -0700 Subject: [PATCH 1/5] ui/auth: Remove lingering code from unified merge Fidesops PR 1409 replaced the original credential storage with the redux-persist logic. The credential storage lines got reintroduced by a merge: https://github.com/ethyca/fidesops/pull/1409/files#diff-af758411de81fed87d14012809b227b11c67ce770c998fd5059ea0769fbb5bd4L59 Unclear if this has caused any problems. My guess is that it's just been adding an unused key to localStorage. --- clients/admin-ui/src/app/store.ts | 4 -- .../admin-ui/src/features/auth/auth.slice.ts | 38 ++++--------------- .../user-management/user-management.slice.ts | 4 -- clients/admin-ui/src/features/user/index.ts | 1 - .../admin-ui/src/features/user/user.slice.ts | 35 ----------------- 5 files changed, 7 insertions(+), 75 deletions(-) delete mode 100644 clients/admin-ui/src/features/user/index.ts delete mode 100644 clients/admin-ui/src/features/user/user.slice.ts diff --git a/clients/admin-ui/src/app/store.ts b/clients/admin-ui/src/app/store.ts index 92384a8737..aebdba9ca2 100644 --- a/clients/admin-ui/src/app/store.ts +++ b/clients/admin-ui/src/app/store.ts @@ -56,7 +56,6 @@ import { } from "~/features/organization"; import { reducer as systemReducer, systemApi } from "~/features/system"; import { reducer as taxonomyReducer, taxonomyApi } from "~/features/taxonomy"; -import { reducer as userReducer } from "~/features/user"; import { authApi, AuthState, reducer as authReducer } from "../features/auth"; @@ -90,7 +89,6 @@ const reducer = { [dataUseApi.reducerPath]: dataUseApi.reducer, [datasetApi.reducerPath]: datasetApi.reducer, [datastoreConnectionApi.reducerPath]: datastoreConnectionApi.reducer, - [datastoreConnectionApi.reducerPath]: datastoreConnectionApi.reducer, [organizationApi.reducerPath]: organizationApi.reducer, [plusApi.reducerPath]: plusApi.reducer, [privacyRequestApi.reducerPath]: privacyRequestApi.reducer, @@ -98,7 +96,6 @@ const reducer = { [systemApi.reducerPath]: systemApi.reducer, [taxonomyApi.reducerPath]: taxonomyApi.reducer, [userApi.reducerPath]: userApi.reducer, - [userApi.reducerPath]: userApi.reducer, auth: authReducer, configWizard: configWizardReducer, connectionType: connectionTypeReducer, @@ -111,7 +108,6 @@ const reducer = { subjectRequests: privacyRequestsReducer, system: systemReducer, taxonomy: taxonomyReducer, - user: userReducer, userManagement: userManagementReducer, }; diff --git a/clients/admin-ui/src/features/auth/auth.slice.ts b/clients/admin-ui/src/features/auth/auth.slice.ts index 3486f0b969..97a41bc79f 100644 --- a/clients/admin-ui/src/features/auth/auth.slice.ts +++ b/clients/admin-ui/src/features/auth/auth.slice.ts @@ -1,15 +1,12 @@ -import { - createListenerMiddleware, - createSlice, - PayloadAction, -} from "@reduxjs/toolkit"; +import { createSlice, PayloadAction } from "@reduxjs/toolkit"; import { createApi, fetchBaseQuery } from "@reduxjs/toolkit/query/react"; -import { addCommonHeaders } from "common/CommonHeaders"; -import { utf8ToB64 } from "common/utils"; -import { User } from "user-management/types"; -import type { RootState } from "../../app/store"; -import { BASE_URL, STORAGE_ROOT_KEY } from "../../constants"; +import type { RootState } from "~/app/store"; +import { BASE_URL } from "~/constants"; +import { addCommonHeaders } from "~/features/common/CommonHeaders"; +import { utf8ToB64 } from "~/features/common/utils"; +import { User } from "~/features/user-management/types"; + import { LoginRequest, LoginResponse, @@ -56,27 +53,6 @@ export const selectToken = (state: RootState) => selectAuth(state).token; export const { login, logout } = authSlice.actions; -export const credentialStorage = createListenerMiddleware(); -credentialStorage.startListening({ - actionCreator: login, - effect: (action, { getState }) => { - if (window && window.localStorage) { - localStorage.setItem( - STORAGE_ROOT_KEY, - JSON.stringify(selectAuth(getState() as RootState)) - ); - } - }, -}); -credentialStorage.startListening({ - actionCreator: logout, - effect: () => { - if (window && window.localStorage) { - localStorage.removeItem(STORAGE_ROOT_KEY); - } - }, -}); - // Auth API export const authApi = createApi({ reducerPath: "authApi", diff --git a/clients/admin-ui/src/features/user-management/user-management.slice.ts b/clients/admin-ui/src/features/user-management/user-management.slice.ts index 710dda113a..bf74152b28 100644 --- a/clients/admin-ui/src/features/user-management/user-management.slice.ts +++ b/clients/admin-ui/src/features/user-management/user-management.slice.ts @@ -37,10 +37,6 @@ export const userManagementSlice = createSlice({ name: "userManagement", initialState, reducers: { - assignToken: (state, action: PayloadAction) => ({ - ...state, - token: action.payload, - }), setUsernameSearch: (state, action: PayloadAction) => ({ ...state, page: initialState.page, diff --git a/clients/admin-ui/src/features/user/index.ts b/clients/admin-ui/src/features/user/index.ts deleted file mode 100644 index bc748a17c5..0000000000 --- a/clients/admin-ui/src/features/user/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from "./user.slice"; diff --git a/clients/admin-ui/src/features/user/user.slice.ts b/clients/admin-ui/src/features/user/user.slice.ts deleted file mode 100644 index 46773f5e29..0000000000 --- a/clients/admin-ui/src/features/user/user.slice.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { createSlice, PayloadAction } from "@reduxjs/toolkit"; -import { HYDRATE } from "next-redux-wrapper"; - -import type { RootState } from "../../app/store"; - -export interface State { - token: string | null; -} - -const initialState: State = { - token: null, -}; - -export const userSlice = createSlice({ - name: "user", - initialState, - reducers: { - assignToken: (state, action: PayloadAction) => ({ - ...state, - token: action.payload, - }), - }, - extraReducers: { - [HYDRATE]: (state, action) => ({ - ...state, - ...action.payload.user, - }), - }, -}); - -export const { assignToken } = userSlice.actions; - -export const selectUserToken = (state: RootState) => state.user.token; - -export const { reducer } = userSlice; From 5815a8957f80cc94f1c2ec93b0a0a5fb9770d894 Mon Sep 17 00:00:00 2001 From: Sebastian Sangervasi Date: Tue, 25 Oct 2022 11:38:18 -0700 Subject: [PATCH 2/5] ui/types: Add UserPermissions models --- clients/admin-ui/src/types/api/index.ts | 3 +++ .../src/types/api/models/UserPermissionsCreate.ts | 10 ++++++++++ .../src/types/api/models/UserPermissionsEdit.ts | 11 +++++++++++ .../src/types/api/models/UserPermissionsResponse.ts | 12 ++++++++++++ 4 files changed, 36 insertions(+) create mode 100644 clients/admin-ui/src/types/api/models/UserPermissionsCreate.ts create mode 100644 clients/admin-ui/src/types/api/models/UserPermissionsEdit.ts create mode 100644 clients/admin-ui/src/types/api/models/UserPermissionsResponse.ts diff --git a/clients/admin-ui/src/types/api/index.ts b/clients/admin-ui/src/types/api/index.ts index c41d6bd273..f890253bec 100644 --- a/clients/admin-ui/src/types/api/index.ts +++ b/clients/admin-ui/src/types/api/index.ts @@ -66,6 +66,9 @@ export type { UserCreateResponse } from "./models/UserCreateResponse"; export type { UserLogin } from "./models/UserLogin"; export type { UserLoginResponse } from "./models/UserLoginResponse"; export type { UserPasswordReset } from "./models/UserPasswordReset"; +export type { UserPermissionsCreate } from "./models/UserPermissionsCreate"; +export type { UserPermissionsEdit } from "./models/UserPermissionsEdit"; +export type { UserPermissionsResponse } from "./models/UserPermissionsResponse"; export type { UserResponse } from "./models/UserResponse"; export type { UserUpdate } from "./models/UserUpdate"; export type { ValidateRequest } from "./models/ValidateRequest"; diff --git a/clients/admin-ui/src/types/api/models/UserPermissionsCreate.ts b/clients/admin-ui/src/types/api/models/UserPermissionsCreate.ts new file mode 100644 index 0000000000..ab0c4017f8 --- /dev/null +++ b/clients/admin-ui/src/types/api/models/UserPermissionsCreate.ts @@ -0,0 +1,10 @@ +/* istanbul ignore file */ +/* tslint:disable */ +/* eslint-disable */ + +/** + * Data required to create a FidesUserPermissions record + */ +export type UserPermissionsCreate = { + scopes: Array; +}; diff --git a/clients/admin-ui/src/types/api/models/UserPermissionsEdit.ts b/clients/admin-ui/src/types/api/models/UserPermissionsEdit.ts new file mode 100644 index 0000000000..27121ce6d8 --- /dev/null +++ b/clients/admin-ui/src/types/api/models/UserPermissionsEdit.ts @@ -0,0 +1,11 @@ +/* istanbul ignore file */ +/* tslint:disable */ +/* eslint-disable */ + +/** + * Data required to edit a FidesUserPermissions record + */ +export type UserPermissionsEdit = { + scopes: Array; + id: string; +}; diff --git a/clients/admin-ui/src/types/api/models/UserPermissionsResponse.ts b/clients/admin-ui/src/types/api/models/UserPermissionsResponse.ts new file mode 100644 index 0000000000..e2b85b25f3 --- /dev/null +++ b/clients/admin-ui/src/types/api/models/UserPermissionsResponse.ts @@ -0,0 +1,12 @@ +/* istanbul ignore file */ +/* tslint:disable */ +/* eslint-disable */ + +/** + * Response after creating, editing, or retrieving a FidesUserPermissions record + */ +export type UserPermissionsResponse = { + scopes: Array; + id: string; + user_id: string; +}; From 2eed1ca15f4bf4c55ff732188b0905d3712f1e64 Mon Sep 17 00:00:00 2001 From: Sebastian Sangervasi Date: Tue, 25 Oct 2022 14:49:18 -0700 Subject: [PATCH 3/5] ui/user-management: Use API models where possible --- .../features/user-management/EditUserForm.tsx | 2 +- .../src/features/user-management/UserForm.tsx | 6 +- .../src/features/user-management/types.ts | 72 +++++++++---------- .../user-management/user-management.slice.ts | 44 +++++------- 4 files changed, 53 insertions(+), 71 deletions(-) diff --git a/clients/admin-ui/src/features/user-management/EditUserForm.tsx b/clients/admin-ui/src/features/user-management/EditUserForm.tsx index 3019f03838..401dd49f71 100644 --- a/clients/admin-ui/src/features/user-management/EditUserForm.tsx +++ b/clients/admin-ui/src/features/user-management/EditUserForm.tsx @@ -18,7 +18,7 @@ const useUserForm = (profile: User, permissions: UserPermissions) => { username: profile.username ?? "", first_name: profile.first_name ?? "", last_name: profile.last_name ?? "", - password: profile.password ?? "", + password: "", scopes: permissions.scopes ?? [], id: profile.id, }; diff --git a/clients/admin-ui/src/features/user-management/UserForm.tsx b/clients/admin-ui/src/features/user-management/UserForm.tsx index a7f8410869..2b06eb9e91 100644 --- a/clients/admin-ui/src/features/user-management/UserForm.tsx +++ b/clients/admin-ui/src/features/user-management/UserForm.tsx @@ -17,7 +17,7 @@ import * as Yup from "yup"; import { USER_MANAGEMENT_ROUTE, USER_PRIVILEGES } from "../../constants"; import { CustomTextInput } from "./form/inputs"; -import { User } from "./types"; +import { User, UserCreateResponse } from "./types"; import UpdatePasswordModal from "./UpdatePasswordModal"; import { useUpdateUserPermissionsMutation } from "./user-management.slice"; @@ -49,7 +49,7 @@ const ValidationSchema = Yup.object().shape({ interface Props { onSubmit: (values: FormValues) => Promise< | { - data: User; + data: User | UserCreateResponse; } | { error: FetchBaseQueryError | SerializedError; @@ -86,7 +86,7 @@ const UserForm = ({ // then issue a separate call to update their permissions const { data } = result; const userWithPrivileges = { - id: data.id, + user_id: data.id, scopes: [...new Set([...values.scopes, "privacy-request:read"])], }; const updateUserPermissionsResult = await updateUserPermissions( diff --git a/clients/admin-ui/src/features/user-management/types.ts b/clients/admin-ui/src/features/user-management/types.ts index ea20a50d6f..59dc777da7 100644 --- a/clients/admin-ui/src/features/user-management/types.ts +++ b/clients/admin-ui/src/features/user-management/types.ts @@ -1,19 +1,33 @@ -export interface User { - id: string; - first_name?: string; - last_name?: string; - username?: string; - password?: string; - created_at?: string; -} +import { + Page_UserResponse_, + UserCreate, + UserCreateResponse, + UserPasswordReset, + UserPermissionsCreate, + UserPermissionsEdit, + UserPermissionsResponse, + UserResponse, + UserUpdate, +} from "~/types/api"; + +// Now that we have generated API types, this file can mostly re-export those interfaces. +export type { + UserCreate, + UserCreateResponse, + UserPermissionsCreate, + UserPermissionsResponse, + UserResponse, + UserUpdate, +}; -export interface UserResponse { - id: string; -} +export interface UsersResponse extends Page_UserResponse_ {} -export interface UsersResponse { - items: User[]; - total: number; +export interface User extends UserResponse {} + +export interface UserPermissions extends UserPermissionsResponse {} + +export interface UserUpdateParams extends UserUpdate { + id: string; } export interface UsersListParams { @@ -22,37 +36,17 @@ export interface UsersListParams { username: string; } -export interface UserPasswordUpdate { +export interface UserPasswordResetParams extends UserPasswordReset { id: string; - old_password: string; - new_password: string; } -export interface UserPermissionsUpdate { - id: string | null; - scopes: string[]; -} - -export interface UserPermissionsResponse { - data: { - id: string; - }; - scope: string[]; +export interface UserPermissionsEditParams { + // This is the Id of the User, not the the Id field of the UserPermissions model. + user_id: string; + scopes: UserPermissionsEdit["scopes"]; } export interface UserPrivileges { privilege: string; scope: string; } - -export type CreateUserError = { - detail: { - msg: string; - }[]; -}; - -export type UserPermissions = { - id: string; - scopes: string[]; - user_id: string; -}; diff --git a/clients/admin-ui/src/features/user-management/user-management.slice.ts b/clients/admin-ui/src/features/user-management/user-management.slice.ts index bf74152b28..840015da4c 100644 --- a/clients/admin-ui/src/features/user-management/user-management.slice.ts +++ b/clients/admin-ui/src/features/user-management/user-management.slice.ts @@ -3,18 +3,21 @@ import { createApi, fetchBaseQuery } from "@reduxjs/toolkit/query/react"; import { addCommonHeaders } from "common/CommonHeaders"; import { utf8ToB64 } from "common/utils"; -import type { RootState } from "../../app/store"; -import { BASE_URL } from "../../constants"; +import type { RootState } from "~/app/store"; +import { BASE_URL } from "~/constants"; + import { selectToken } from "../auth"; import { User, - UserPasswordUpdate, + UserCreate, + UserCreateResponse, + UserPasswordResetParams, UserPermissions, - UserPermissionsResponse, - UserPermissionsUpdate, + UserPermissionsEditParams, UserResponse, UsersListParams, UsersResponse, + UserUpdateParams, } from "./types"; export interface UserManagementState { @@ -102,7 +105,7 @@ export const userApi = createApi({ query: (id) => ({ url: `user/${id}/permission` }), providesTags: ["User"], }), - createUser: build.mutation>({ + createUser: build.mutation({ query: (user) => ({ url: "user", method: "POST", @@ -110,18 +113,7 @@ export const userApi = createApi({ }), invalidatesTags: ["User"], }), - createUserPermissions: build.mutation< - UserPermissionsResponse, - Partial - >({ - query: (user) => ({ - url: `user/${user?.data?.id}/permission`, - method: "POST", - body: { scopes: user.scope }, - }), - invalidatesTags: ["User"], - }), - editUser: build.mutation & Pick>({ + editUser: build.mutation({ query: ({ id, ...patch }) => ({ url: `user/${id}`, method: "PUT", @@ -148,10 +140,7 @@ export const userApi = createApi({ } }, }), - updateUserPassword: build.mutation< - UserPasswordUpdate, - Partial & Pick - >({ + updateUserPassword: build.mutation({ query: ({ id, old_password, new_password }) => ({ url: `user/${id}/reset-password`, method: "POST", @@ -163,13 +152,13 @@ export const userApi = createApi({ invalidatesTags: ["User"], }), updateUserPermissions: build.mutation< - UserPermissionsUpdate, - Partial & Pick + UserPermissions, + UserPermissionsEditParams >({ - query: ({ id, scopes }) => ({ - url: `user/${id}/permission`, + query: ({ user_id, scopes }) => ({ + url: `user/${user_id}/permission`, method: "PUT", - body: { id, scopes }, + body: { id: user_id, scopes }, }), invalidatesTags: ["User"], }), @@ -192,6 +181,5 @@ export const { useDeleteUserMutation, useUpdateUserPasswordMutation, useUpdateUserPermissionsMutation, - useCreateUserPermissionsMutation, useGetUserPermissionsQuery, } = userApi; From cd5c97e02d9882ac160695f0a1d2408f9f4c1ede Mon Sep 17 00:00:00 2001 From: Sebastian Sangervasi Date: Tue, 25 Oct 2022 17:55:42 -0700 Subject: [PATCH 4/5] [1480] ui/auth: Query current user permissions to detect stale token --- clients/admin-ui/cypress/support/commands.ts | 16 ++++++----- clients/admin-ui/src/constants.ts | 7 +++++ .../src/features/auth/ProtectedRoute.tsx | 27 +++++++++++++------ 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/clients/admin-ui/cypress/support/commands.ts b/clients/admin-ui/cypress/support/commands.ts index 0be9973e73..e5f98ae771 100644 --- a/clients/admin-ui/cypress/support/commands.ts +++ b/clients/admin-ui/cypress/support/commands.ts @@ -1,6 +1,6 @@ /// -import { STORAGE_ROOT_KEY } from "~/constants"; +import { STORAGE_ROOT_KEY, USER_PRIVILEGES } from "~/constants"; Cypress.Commands.add("getByTestId", (selector, ...args) => cy.get(`[data-testid='${selector}']`, ...args) @@ -9,13 +9,17 @@ Cypress.Commands.add("getByTestId", (selector, ...args) => Cypress.Commands.add("login", () => { cy.fixture("login.json").then((body) => { const authState = { - user_data: body.user_data, + user: body.user_data, token: body.token_data.access_token, }; - window.localStorage.setItem( - STORAGE_ROOT_KEY, - JSON.stringify(authState) - ); + window.localStorage.setItem(STORAGE_ROOT_KEY, JSON.stringify(authState)); + cy.intercept("/api/v1/user/*/permission", { + body: { + id: body.user_data.id, + user_id: body.user_data.id, + scopes: USER_PRIVILEGES.map((up) => up.scope), + }, + }).as("getUserPermission"); }); }); diff --git a/clients/admin-ui/src/constants.ts b/clients/admin-ui/src/constants.ts index a19b532f95..a5845d17cd 100644 --- a/clients/admin-ui/src/constants.ts +++ b/clients/admin-ui/src/constants.ts @@ -106,6 +106,13 @@ export const USER_PRIVILEGES: UserPrivileges[] = [ }, ]; +/** + * Interval between re-fetching a logged-in user's permission to validate their auth token. + * Only applies to an active page -- token will always revalidate on page refresh. + * Ten minutes in milliseconds. + */ +export const VERIFY_AUTH_INTERVAL = 10 * 60 * 1000; + // API ROUTES export const INDEX_ROUTE = "/"; export const LOGIN_ROUTE = "/login"; diff --git a/clients/admin-ui/src/features/auth/ProtectedRoute.tsx b/clients/admin-ui/src/features/auth/ProtectedRoute.tsx index 8c6dddc948..6cfcd02044 100644 --- a/clients/admin-ui/src/features/auth/ProtectedRoute.tsx +++ b/clients/admin-ui/src/features/auth/ProtectedRoute.tsx @@ -1,20 +1,31 @@ import { useRouter } from "next/router"; -import { useSelector } from "react-redux"; +import { useDispatch, useSelector } from "react-redux"; -import { LOGIN_ROUTE } from "../../constants"; -import { selectToken } from "./auth.slice"; +import { LOGIN_ROUTE, VERIFY_AUTH_INTERVAL } from "~/constants"; +import { useGetUserPermissionsQuery } from "~/features/user-management"; + +import { logout, selectToken, selectUser } from "./auth.slice"; const useProtectedRoute = (redirectUrl: string) => { const router = useRouter(); + const dispatch = useDispatch(); const token = useSelector(selectToken); - - // TODO: check for token invalidation - if (!token && typeof window !== "undefined") { - router.push(redirectUrl); + const user = useSelector(selectUser); + const userId = user?.id; + const permissionsQuery = useGetUserPermissionsQuery(userId!, { + pollingInterval: VERIFY_AUTH_INTERVAL, + skip: !userId, + }); + + if (!token || !userId || permissionsQuery.isError) { + dispatch(logout()); + if (typeof window !== "undefined") { + router.push(redirectUrl); + } return false; } - return true; + return permissionsQuery.isSuccess; }; interface ProtectedRouteProps { From 3f32f70ca1728200df64c438087fb94f0d34d391 Mon Sep 17 00:00:00 2001 From: Sebastian Sangervasi Date: Tue, 25 Oct 2022 18:22:29 -0700 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c489fea960..03e756dd94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The types of changes are: ### Fixed * After editing a dataset, the table will stay on the previously selected collection instead of resetting to the first one. [#1511](https://github.com/ethyca/fides/pull/1511) +* Expired auth tokens will now log the user out automatically. [#1569](https://github.com/ethyca/fides/pull/1569) ## [1.9.2](https://github.com/ethyca/fides/compare/1.9.1...1.9.2)