-
Notifications
You must be signed in to change notification settings - Fork 937
chore(site): refactor AuthProvider to not use authXService #10184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
10f3c10
f577c4d
fd84833
4749547
6d0e3ab
06b9748
de37d44
cc18ecd
c6bee93
4aec6a5
b588271
ed42cbb
e6574e9
e29e3c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { AuthorizationRequest } from "api/typesGenerated"; | ||
import * as API from "api/api"; | ||
|
||
export const AUTHORIZATION_KEY = "authorization"; | ||
|
||
export const getAuthorizationKey = (req: AuthorizationRequest) => | ||
[AUTHORIZATION_KEY, req] as const; | ||
|
||
export const checkAuthorization = (req: AuthorizationRequest) => { | ||
return { | ||
queryKey: getAuthorizationKey(req), | ||
queryFn: () => API.checkAuthorization(req), | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,15 @@ | ||
import { QueryClient, QueryOptions } from "react-query"; | ||
import * as API from "api/api"; | ||
import { | ||
AuthorizationRequest, | ||
GetUsersResponse, | ||
UpdateUserPasswordRequest, | ||
UpdateUserProfileRequest, | ||
User, | ||
UsersRequest, | ||
} from "api/typesGenerated"; | ||
import { getMetadataAsJSON } from "utils/metadata"; | ||
import { getAuthorizationKey } from "./authCheck"; | ||
|
||
export const users = (req: UsersRequest): QueryOptions<GetUsersResponse> => { | ||
return { | ||
|
@@ -83,3 +88,76 @@ export const authMethods = () => { | |
queryFn: API.getAuthMethods, | ||
}; | ||
}; | ||
|
||
export const me = () => { | ||
return { | ||
queryKey: ["me"], | ||
queryFn: async () => | ||
getMetadataAsJSON<User>("user") ?? API.getAuthenticatedUser(), | ||
}; | ||
}; | ||
|
||
export const hasFirstUser = () => { | ||
return { | ||
queryKey: ["hasFirstUser"], | ||
queryFn: API.hasFirstUser, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this need to be wrapped in an arrow function, for mocking purposes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is okay for now since it is using MSW on the related tests... this is something we should talk about and get some "consensus" about what to use to mock API requests. |
||
}; | ||
}; | ||
|
||
export const login = ( | ||
authorization: AuthorizationRequest, | ||
queryClient: QueryClient, | ||
) => { | ||
return { | ||
mutationFn: async (credentials: { email: string; password: string }) => | ||
loginFn({ ...credentials, authorization }), | ||
onSuccess: async (data: Awaited<ReturnType<typeof loginFn>>) => { | ||
queryClient.setQueryData(["me"], data.user); | ||
queryClient.setQueryData( | ||
getAuthorizationKey(authorization), | ||
data.permissions, | ||
); | ||
}, | ||
}; | ||
}; | ||
|
||
const loginFn = async ({ | ||
email, | ||
password, | ||
authorization, | ||
}: { | ||
email: string; | ||
password: string; | ||
authorization: AuthorizationRequest; | ||
}) => { | ||
await API.login(email, password); | ||
const [user, permissions] = await Promise.all([ | ||
API.getAuthenticatedUser(), | ||
API.checkAuthorization(authorization), | ||
]); | ||
return { | ||
user, | ||
permissions, | ||
}; | ||
}; | ||
|
||
export const logout = (queryClient: QueryClient) => { | ||
return { | ||
mutationFn: API.logout, | ||
onSuccess: () => { | ||
queryClient.removeQueries(); | ||
}, | ||
}; | ||
}; | ||
|
||
export const updateProfile = () => { | ||
return { | ||
mutationFn: ({ | ||
userId, | ||
req, | ||
}: { | ||
userId: string; | ||
req: UpdateUserProfileRequest; | ||
}) => API.updateProfile(userId, req), | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,133 @@ | ||
import { useActor, useInterpret } from "@xstate/react"; | ||
import { createContext, FC, PropsWithChildren, useContext } from "react"; | ||
import { authMachine } from "xServices/auth/authXService"; | ||
import { ActorRefFrom } from "xstate"; | ||
import { checkAuthorization } from "api/queries/authCheck"; | ||
import { | ||
authMethods, | ||
hasFirstUser, | ||
login, | ||
logout, | ||
me, | ||
updateProfile as updateProfileOptions, | ||
} from "api/queries/users"; | ||
import { | ||
AuthMethods, | ||
UpdateUserProfileRequest, | ||
User, | ||
} from "api/typesGenerated"; | ||
import { | ||
createContext, | ||
FC, | ||
PropsWithChildren, | ||
useCallback, | ||
useContext, | ||
} from "react"; | ||
import { useMutation, useQuery, useQueryClient } from "react-query"; | ||
import { permissionsToCheck, Permissions } from "./permissions"; | ||
import { displaySuccess } from "components/GlobalSnackbar/utils"; | ||
import { FullScreenLoader } from "components/Loader/FullScreenLoader"; | ||
import { isApiError } from "api/errors"; | ||
|
||
interface AuthContextValue { | ||
authService: ActorRefFrom<typeof authMachine>; | ||
} | ||
type AuthContextValue = { | ||
isSignedOut: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if some of the connected properties on the type ( status: "signingIn" | "signedIn" | "signingOut" | "signedOut" Logically, it shouldn't be possible for a user to be be in multiple of these states at once, but it's still possible at the type level right now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm.... I think this is a good one. |
||
isSigningOut: boolean; | ||
isConfiguringTheFirstUser: boolean; | ||
isSignedIn: boolean; | ||
isSigningIn: boolean; | ||
isUpdatingProfile: boolean; | ||
user: User | undefined; | ||
permissions: Permissions | undefined; | ||
authMethods: AuthMethods | undefined; | ||
signInError: unknown; | ||
updateProfileError: unknown; | ||
signOut: () => void; | ||
signIn: (email: string, password: string) => Promise<void>; | ||
updateProfile: (data: UpdateUserProfileRequest) => void; | ||
}; | ||
|
||
const AuthContext = createContext<AuthContextValue | undefined>(undefined); | ||
|
||
export const AuthProvider: FC<PropsWithChildren> = ({ children }) => { | ||
const authService = useInterpret(authMachine); | ||
const meOptions = me(); | ||
const userQuery = useQuery(meOptions); | ||
const authMethodsQuery = useQuery(authMethods()); | ||
const hasFirstUserQuery = useQuery(hasFirstUser()); | ||
const permissionsQuery = useQuery({ | ||
...checkAuthorization({ checks: permissionsToCheck }), | ||
enabled: userQuery.data !== undefined, | ||
}); | ||
|
||
const queryClient = useQueryClient(); | ||
const loginMutation = useMutation( | ||
login({ checks: permissionsToCheck }, queryClient), | ||
); | ||
const logoutMutation = useMutation(logout(queryClient)); | ||
const updateProfileMutation = useMutation({ | ||
...updateProfileOptions(), | ||
onSuccess: (user) => { | ||
queryClient.setQueryData(meOptions.queryKey, user); | ||
displaySuccess("Updated settings."); | ||
}, | ||
}); | ||
|
||
const isSignedOut = | ||
userQuery.isError && | ||
isApiError(userQuery.error) && | ||
userQuery.error.response.status === 401; | ||
const isSigningOut = logoutMutation.isLoading; | ||
const isLoading = | ||
authMethodsQuery.isLoading || | ||
userQuery.isLoading || | ||
hasFirstUserQuery.isLoading || | ||
(userQuery.isSuccess && permissionsQuery.isLoading); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is overkill, but I'm wondering if userQuery.isSuccess && permissionsQuery.isLoading could be swapped out for permissionsQuery.isFetching && permissionsQuery.isLoading just to help insulate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm... not sure, for me the way it is now it is better to understand than the one you proposed, and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's fair. This might be partly misunderstanding the API on my part – I need to review what happens if a query succeeds on an initial fetch, but errors out on a refetch |
||
const isConfiguringTheFirstUser = !hasFirstUserQuery.data; | ||
const isSignedIn = userQuery.isSuccess && userQuery.data !== undefined; | ||
const isSigningIn = loginMutation.isLoading; | ||
const isUpdatingProfile = updateProfileMutation.isLoading; | ||
|
||
const signOut = useCallback(() => { | ||
logoutMutation.mutate(); | ||
}, [logoutMutation]); | ||
|
||
const signIn = async (email: string, password: string) => { | ||
await loginMutation.mutateAsync({ email, password }); | ||
}; | ||
|
||
const updateProfile = (req: UpdateUserProfileRequest) => { | ||
updateProfileMutation.mutate({ userId: userQuery.data!.id, req }); | ||
}; | ||
|
||
if (isLoading) { | ||
return <FullScreenLoader />; | ||
} | ||
|
||
return ( | ||
<AuthContext.Provider value={{ authService }}> | ||
<AuthContext.Provider | ||
value={{ | ||
isSignedOut, | ||
isSigningOut, | ||
isConfiguringTheFirstUser, | ||
isSignedIn, | ||
isSigningIn, | ||
isUpdatingProfile, | ||
signOut, | ||
signIn, | ||
updateProfile, | ||
user: userQuery.data, | ||
permissions: permissionsQuery.data as Permissions | undefined, | ||
Parkreiner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
authMethods: authMethodsQuery.data, | ||
signInError: loginMutation.error, | ||
updateProfileError: updateProfileMutation.error, | ||
}} | ||
> | ||
{children} | ||
</AuthContext.Provider> | ||
); | ||
}; | ||
|
||
type UseAuthReturnType = ReturnType< | ||
typeof useActor<AuthContextValue["authService"]> | ||
>; | ||
|
||
export const useAuth = (): UseAuthReturnType => { | ||
export const useAuth = () => { | ||
const context = useContext(AuthContext); | ||
|
||
if (!context) { | ||
throw new Error("useAuth should be used inside of <AuthProvider />"); | ||
} | ||
|
||
const auth = useActor(context.authService); | ||
|
||
return auth; | ||
return context; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
export const checks = { | ||
readAllUsers: "readAllUsers", | ||
updateUsers: "updateUsers", | ||
createUser: "createUser", | ||
createTemplates: "createTemplates", | ||
updateTemplates: "updateTemplates", | ||
deleteTemplates: "deleteTemplates", | ||
viewAuditLog: "viewAuditLog", | ||
viewDeploymentValues: "viewDeploymentValues", | ||
createGroup: "createGroup", | ||
viewUpdateCheck: "viewUpdateCheck", | ||
viewExternalAuthConfig: "viewExternalAuthConfig", | ||
viewDeploymentStats: "viewDeploymentStats", | ||
editWorkspaceProxies: "editWorkspaceProxies", | ||
} as const; | ||
|
||
export const permissionsToCheck = { | ||
[checks.readAllUsers]: { | ||
object: { | ||
resource_type: "user", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.updateUsers]: { | ||
object: { | ||
resource_type: "user", | ||
}, | ||
action: "update", | ||
}, | ||
[checks.createUser]: { | ||
object: { | ||
resource_type: "user", | ||
}, | ||
action: "create", | ||
}, | ||
[checks.createTemplates]: { | ||
object: { | ||
resource_type: "template", | ||
}, | ||
action: "update", | ||
}, | ||
[checks.updateTemplates]: { | ||
object: { | ||
resource_type: "template", | ||
}, | ||
action: "update", | ||
}, | ||
[checks.deleteTemplates]: { | ||
object: { | ||
resource_type: "template", | ||
}, | ||
action: "delete", | ||
}, | ||
[checks.viewAuditLog]: { | ||
object: { | ||
resource_type: "audit_log", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.viewDeploymentValues]: { | ||
object: { | ||
resource_type: "deployment_config", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.createGroup]: { | ||
object: { | ||
resource_type: "group", | ||
}, | ||
action: "create", | ||
}, | ||
[checks.viewUpdateCheck]: { | ||
object: { | ||
resource_type: "deployment_config", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.viewExternalAuthConfig]: { | ||
object: { | ||
resource_type: "deployment_config", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.viewDeploymentStats]: { | ||
object: { | ||
resource_type: "deployment_stats", | ||
}, | ||
action: "read", | ||
}, | ||
[checks.editWorkspaceProxies]: { | ||
object: { | ||
resource_type: "workspace_proxy", | ||
}, | ||
action: "create", | ||
}, | ||
} as const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just because this is such a long object, I feel like it could help to add two type-checking helpers. Something like: type PermissionConfig = {
object: {
resource_type: string; // Or something more specific, if it's already a type
},
action: "read" | "create" | "update" | "delete"
} export const permissionsToCheck = {
// Same content
} as const satisfies Record<keyof typeof checks, PermissionConfig>; I'm just worried that if somebody has a typo when updating the object, it might take a while for them to catch it otherwise There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, I'm wondering if we could get rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I liked both suggestions. |
||
|
||
export type Permissions = Record<keyof typeof permissionsToCheck, boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an issue, but something I've been wondering about: does the server have sanitization for the data we're sending it from the client? With TS being structurally-typed, the object we serialize for the request could have extra properties on it, and TypeScript wouldn't necessarily complain, because the overall "shape" would be what it's looking for. I'm worried that could cause two issues at some point:
I don't think 2 is worth worrying about too much, but the server is definitely a worry for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, the server is doing some parsing on the params so we don't need to worry about it for now.