Skip to content

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Oct 11, 2023
Merged

chore(site): refactor AuthProvider to not use authXService #10184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
10f3c10
Move xstate transitions to provider
BrunoQuaresma Oct 10, 2023
f577c4d
Centrlize auth logic in the provider
BrunoQuaresma Oct 10, 2023
fd84833
Remove actor
BrunoQuaresma Oct 10, 2023
4749547
Remove auth xservice
BrunoQuaresma Oct 10, 2023
6d0e3ab
Add loader while AuthProvider is loading
BrunoQuaresma Oct 10, 2023
06b9748
Simplify and fix a few computed states
BrunoQuaresma Oct 10, 2023
de37d44
Add a few replaces
BrunoQuaresma Oct 10, 2023
cc18ecd
Fix logout
BrunoQuaresma Oct 10, 2023
c6bee93
Remove unused import
BrunoQuaresma Oct 10, 2023
4aec6a5
Fix RequireAuth test
BrunoQuaresma Oct 10, 2023
b588271
Fix wait loader
BrunoQuaresma Oct 10, 2023
ed42cbb
Fix tests
BrunoQuaresma Oct 10, 2023
e6574e9
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
BrunoQuaresma Oct 11, 2023
e29e3c7
Wrap signout with callback
BrunoQuaresma Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions site/src/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,19 +120,9 @@ export const logout = async (): Promise<void> => {
await axios.post("/api/v2/users/logout");
};

export const getAuthenticatedUser = async (): Promise<
TypesGen.User | undefined
> => {
try {
const response = await axios.get<TypesGen.User>("/api/v2/users/me");
return response.data;
} catch (error) {
if (axios.isAxiosError(error) && error.response?.status === 401) {
return undefined;
}

throw error;
}
export const getAuthenticatedUser = async () => {
const response = await axios.get<TypesGen.User>("/api/v2/users/me");
return response.data;
};

export const getAuthMethods = async (): Promise<TypesGen.AuthMethods> => {
Expand Down
14 changes: 14 additions & 0 deletions site/src/api/queries/authCheck.ts
View file
Open in desktop
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),
Copy link
Member

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:

  1. Our request sends extra properties that the server doesn't care about. Not a big deal if the server is defensive about this
  2. RQ hashes objects deterministically, but if extra properties are on the object that we don't know about, that would affect the hash result, and could cause queries to run more often than we expect

I don't think 2 is worth worrying about too much, but the server is definitely a worry for me

Copy link
Collaborator Author

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.

};
};
78 changes: 78 additions & 0 deletions site/src/api/queries/users.ts
View file
Open in desktop
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 {
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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),
};
};
131 changes: 114 additions & 17 deletions site/src/components/AuthProvider/AuthProvider.tsx
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if some of the connected properties on the type (isSignedOut, isSigningOut, isSignedIn, isSigningIn) can be combined into a single status. Something like this:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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 permissionsQuery from any state changes in userQuery

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 permissionsQuery should not be changed.

Copy link
Member

@Parkreiner Parkreiner Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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,
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;
};
98 changes: 98 additions & 0 deletions site/src/components/AuthProvider/permissions.tsx
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I'm wondering if we could get rid of the checks object, in favor of making permissionsToCheck the main source of truth. From what I can tell, the object is being used in this file to define the keys, but the only other place is in is the updateCheckXService file, to make sure the correct key is being used with the permissions object

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked both suggestions.


export type Permissions = Record<keyof typeof permissionsToCheck, boolean>;
Loading