Skip to content

Commit

Permalink
fix: admin verification issues (#594)
Browse files Browse the repository at this point in the history
* Remove dangerous `RESET_PASSWORD_CODE` mutation

* Refactor mutation to query since there is no mutating occurring

* Add new query to get the verification status of a user

* Update frontend logic to match new backend endpoints

* Refactor front-end to fix refresh bug

* Refactor backend URL generation to pass the expected params

* Fix logic error in forgot-password component

* Fix failing tests

* PR feedback: unused function

* PR feedback: clarify logic
  • Loading branch information
jfdoming authored Oct 4, 2023
1 parent d572fb9 commit 13a7217
Show file tree
Hide file tree
Showing 20 changed files with 261 additions and 200 deletions.
7 changes: 6 additions & 1 deletion backend/graphql/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import gql from "graphql-tag";
import { applyMiddleware } from "graphql-middleware";
import { merge } from "lodash";

import { isAuthorizedByRole, isAuthorizedByUserId } from "../middlewares/auth";
import {
isAuthorizedByRole,
isAuthorizedByUserId,
isAuthorizedForEveryone,
} from "../middlewares/auth";
import authResolvers from "./resolvers/authResolvers";
import authType from "./types/authType";
import schoolResolvers from "./resolvers/schoolResolvers";
Expand Down Expand Up @@ -61,6 +65,7 @@ const buildSchema = async () => {

const graphQLMiddlewares = {
Query: {
userVerificationStatus: isAuthorizedForEveryone(),
tests: authorizedByAllRoles(),
schoolByTeacherId: isAuthorizedByUserId("teacherId"),
},
Expand Down
23 changes: 9 additions & 14 deletions backend/graphql/resolvers/authResolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ const cookieOptions: CookieOptions = {
};

const authResolvers = {
Query: {
verifyPasswordResetCode: async (
_parent: undefined,
{ oobCode }: { oobCode: string },
): Promise<string> => {
const email = await authService.verifyPasswordResetCode(oobCode);
return email;
},
},
Mutation: {
login: async (
_parent: undefined,
Expand Down Expand Up @@ -104,27 +113,13 @@ const authResolvers = {
await authService.resetPassword(email);
return true;
},
resetPasswordCode: async (
_parent: undefined,
{ email }: { email: string },
): Promise<string> => {
const oobCode = await authService.resetPasswordCode(email);
return oobCode;
},
verifyEmail: async (
_parent: undefined,
{ oobCode }: { oobCode: string },
): Promise<string> => {
const email = await authService.verifyEmail(oobCode);
return email;
},
verifyPasswordReset: async (
_parent: undefined,
{ oobCode }: { oobCode: string },
): Promise<string> => {
const email = await authService.verifyPasswordReset(oobCode);
return email;
},
confirmPasswordReset: async (
_parent: undefined,
{ newPassword, oobCode }: { newPassword: string; oobCode: string },
Expand Down
6 changes: 6 additions & 0 deletions backend/graphql/resolvers/userResolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ const authService: IAuthService = new AuthService(userService, emailService);

const userResolvers = {
Query: {
userVerificationStatus: async (
_parent: undefined,
{ id }: { id: string },
): Promise<UserDTO> => {
return userService.getUserById(id);
},
userByEmail: async (
_parent: undefined,
{ email }: { email: string },
Expand Down
6 changes: 4 additions & 2 deletions backend/graphql/types/authType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,17 @@ const authType = gql`
school: SchoolMetadataInput!
}
extend type Query {
verifyPasswordResetCode(oobCode: String!): String!
}
extend type Mutation {
login(email: String!, password: String!): AuthDTO!
registerTeacher(user: RegisterTeacherDTO!): AuthDTO!
refresh: String!
logout(userId: ID!): ID
resetPassword(email: String!): Boolean!
resetPasswordCode(email: String!): String!
verifyEmail(oobCode: String!): String!
verifyPasswordReset(oobCode: String!): String!
confirmPasswordReset(newPassword: String!, oobCode: String!): Boolean!
sendEmailVerificationLink(email: String!): Boolean!
}
Expand Down
9 changes: 9 additions & 0 deletions backend/graphql/types/userType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ const userType = gql`
grades: [GradeEnum]
currentlyTeachingJM: Boolean
class: [String]
isVerified: Boolean!
}
type UserVerificationStatusDTO {
id: ID!
email: String!
role: Role!
isVerified: Boolean!
}
type TeacherDTO {
Expand Down Expand Up @@ -50,6 +58,7 @@ const userType = gql`
}
extend type Query {
userVerificationStatus(id: ID!): UserVerificationStatusDTO!
userByEmail(email: String!): UserDTO!
usersByRole(role: String!): [UserDTO!]!
teachers: [TeacherDTO]
Expand Down
18 changes: 18 additions & 0 deletions backend/middlewares/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ export const getAccessToken = (req: Request): string | null => {
return null;
};

/* No-op authorization check which always resolves */
/* eslint-disable @typescript-eslint/no-explicit-any */
export const isAuthorizedForEveryone =
() =>
async (
resolve: (
parent: any,
args: { [key: string]: any },
context: ExpressContext,
info: GraphQLResolveInfo,
) => any,
parent: any,
args: { [key: string]: any },
context: ExpressContext,
info: GraphQLResolveInfo,
) =>
resolve(parent, args, context, info);

/* Determine if request is authorized based on accessToken validity and role of client */
/* eslint-disable @typescript-eslint/no-explicit-any */
/* eslint-disable @typescript-eslint/explicit-module-boundary-types */
Expand Down
67 changes: 45 additions & 22 deletions backend/services/implementations/authService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,24 @@ class AuthService implements IAuthService {
</span>`;
}

private getURLWithoutSearch(link: string): URL {
const url = new URL(link);
url.search = "";
return url;
}

private copyOobCodeToURL(link: URL, source: string, key: string): URL {
const sourceURL = new URL(source);
const oobCode = sourceURL.searchParams.get("oobCode");
if (!oobCode) {
const errorMessage = `Failed to extract oobCode from link ${link}`;
Logger.error(errorMessage);
throw new Error(errorMessage);
}
link.searchParams.append(key, oobCode);
return link;
}

async resetPassword(email: string): Promise<void> {
if (!this.emailService) {
const errorMessage =
Expand All @@ -139,9 +157,14 @@ class AuthService implements IAuthService {

try {
const user = await this.userService.getUserByEmail(email);
const resetLink = await firebaseAdmin
const firebaseResetLink = await firebaseAdmin
.auth()
.generatePasswordResetLink(email);
const resetLink = this.copyOobCodeToURL(
this.getURLWithoutSearch(firebaseResetLink),
firebaseResetLink,
"resetPasswordOobCode",
).toString();

const emailBody = this.getEmailTemplate(
"password-reset-header.png",
Expand All @@ -164,25 +187,6 @@ class AuthService implements IAuthService {
}
}

async resetPasswordCode(email: string): Promise<string> {
let oobCode: string;
try {
const resetLink = await firebaseAdmin
.auth()
.generatePasswordResetLink(email);
const regex = /(?<=&oobCode=)(.*)(?=&apiKey=)/gm;
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
[oobCode] = resetLink.match(regex)!;
} catch (error) {
Logger.error(
`Failed to generate password reset code for user with email ${email}`,
);
throw error;
}

return oobCode;
}

async sendEmailVerificationLink(email: string): Promise<void> {
if (!this.emailService) {
const errorMessage =
Expand All @@ -193,9 +197,28 @@ class AuthService implements IAuthService {

try {
const user = await this.userService.getUserByEmail(email);
const emailVerificationLink = await firebaseAdmin
const firebaseVerificationLink = await firebaseAdmin
.auth()
.generateEmailVerificationLink(email);
let resetLinkURL = this.copyOobCodeToURL(
this.getURLWithoutSearch(firebaseVerificationLink),
firebaseVerificationLink,
"verifyEmailOobCode",
);
if (user.role === "Admin") {
const firebaseResetLink = await firebaseAdmin
.auth()
.generatePasswordResetLink(email);

resetLinkURL = this.copyOobCodeToURL(
resetLinkURL,
firebaseResetLink,
"resetPasswordOobCode",
);
}
resetLinkURL.searchParams.append("userId", user.id);
const emailVerificationLink = resetLinkURL.toString();

const emailBody = this.getEmailTemplate(
"email-header.png",
user.firstName,
Expand Down Expand Up @@ -303,7 +326,7 @@ class AuthService implements IAuthService {
return res.email;
}

async verifyPasswordReset(oobCode: string): Promise<string> {
async verifyPasswordResetCode(oobCode: string): Promise<string> {
let res: ResetPasswordResponse;
try {
res = await FirebaseRestClient.verifyPasswordResetCode(oobCode);
Expand Down
6 changes: 6 additions & 0 deletions backend/services/implementations/userService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class UserService implements IUserService {
role: user.role,
grades: user.grades,
currentlyTeachingJM: user.currentlyTeachingJM,
isVerified: firebaseUser.emailVerified,
};
}

Expand Down Expand Up @@ -78,6 +79,7 @@ class UserService implements IUserService {
role: user.role,
grades: user.grades,
currentlyTeachingJM: user.currentlyTeachingJM,
isVerified: firebaseUser.emailVerified,
};
}

Expand Down Expand Up @@ -143,6 +145,7 @@ class UserService implements IUserService {
role: user.role,
grades: user.grades,
currentlyTeachingJM: user.currentlyTeachingJM,
isVerified: firebaseUser.emailVerified,
};
}),
);
Expand Down Expand Up @@ -204,6 +207,7 @@ class UserService implements IUserService {
role: newUser.role,
grades: newUser.grades,
currentlyTeachingJM: newUser.currentlyTeachingJM,
isVerified: firebaseUser.emailVerified,
};
}

Expand Down Expand Up @@ -272,6 +276,7 @@ class UserService implements IUserService {
role: user.role,
grades: user.grades,
currentlyTeachingJM: user.currentlyTeachingJM,
isVerified: updatedFirebaseUser.emailVerified,
};
}

Expand Down Expand Up @@ -362,6 +367,7 @@ class UserService implements IUserService {
role: user.role,
grades: user.grades,
currentlyTeachingJM: user.currentlyTeachingJM,
isVerified: firebaseUser.emailVerified,
};
}),
);
Expand Down
10 changes: 1 addition & 9 deletions backend/services/interfaces/authService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,6 @@ interface IAuthService {
*/
resetPassword(email: string): Promise<void>;

/**
* Generate a password reset code for the user with the given email
* @param email email of user requesting password reset
* @returns oobCode for password reset
* @throws Error if unable to generate code
*/
resetPasswordCode(email: string): Promise<string>;

/**
* Generate an email verification link for the user with the given email and send
* the link to that email address
Expand Down Expand Up @@ -94,7 +86,7 @@ interface IAuthService {
* @param oobCode email action code sent to the user's email for resetting the password
* @returns the user's email if the password reset code is valid, empty string otherwise
*/
verifyPasswordReset(oobCode: string): Promise<string>;
verifyPasswordResetCode(oobCode: string): Promise<string>;

/**
* Apply a password reset change to the account of the user with the given oobCode
Expand Down
2 changes: 2 additions & 0 deletions backend/testUtils/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export const mockAdmin: UserDTO = {
lastName: "One",
email: "[email protected]",
role: "Admin",
isVerified: true,
};

export const mockTeacher: UserDTO = {
Expand All @@ -18,6 +19,7 @@ export const mockTeacher: UserDTO = {
role: "Teacher",
grades: [Grade.KINDERGARTEN, Grade.GRADE_1, Grade.GRADE_2, Grade.GRADE_3],
currentlyTeachingJM: true,
isVerified: true,
};

export const testUsers = [mockAdmin, mockTeacher];
9 changes: 6 additions & 3 deletions backend/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,18 @@ export type UserDTO = {
role: Role;
grades?: Grade[];
currentlyTeachingJM?: boolean;
isVerified: boolean;
};

export type TeacherDTO = UserDTO & { school: string };

export type CreateUserDTO = Omit<UserDTO, "id"> & { password: string };
export type CreateUserDTO = Omit<UserDTO, "id" | "isVerified"> & {
password: string;
};

export type UpdateUserDTO = Omit<UserDTO, "id">;
export type UpdateUserDTO = Omit<UserDTO, "id" | "isVerified">;

export type RegisterUserDTO = Omit<CreateUserDTO, "role">;
export type RegisterUserDTO = Omit<CreateUserDTO, "role" | "isVerified">;

export interface SchoolMetadata {
name: string;
Expand Down
12 changes: 0 additions & 12 deletions frontend/src/APIClients/mutations/AuthMutations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,12 @@ export const RESET_PASSWORD = gql`
}
`;

export const RESET_PASSWORD_CODE = gql`
mutation ResetPasswordCode($email: String!) {
resetPasswordCode(email: $email)
}
`;

export const VERIFY_EMAIL = gql`
mutation VerifyEmail($oobCode: String!) {
verifyEmail(oobCode: $oobCode)
}
`;

export const VERIFY_PASSWORD_RESET = gql`
mutation VerifyPasswordReset($oobCode: String!) {
verifyPasswordReset(oobCode: $oobCode)
}
`;

export const CONFIRM_PASSWORD_RESET = gql`
mutation ConfirmPasswordReset($newPassword: String!, $oobCode: String!) {
confirmPasswordReset(newPassword: $newPassword, oobCode: $oobCode)
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/APIClients/queries/AuthQueries.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { gql } from "@apollo/client";

export const VERIFY_PASSWORD_RESET_CODE = gql`
query VerifyPasswordResetCode($oobCode: String!) {
verifyPasswordResetCode(oobCode: $oobCode)
}
`;
Loading

0 comments on commit 13a7217

Please sign in to comment.