chore: cleanup of update user by oidcsub (#1309)

Author: yoganandaness

apps/backend/src/datasources/UserDataSource.ts

@@ -4,10 +4,7 @@ import { Role, Roles } from '../models/Role';
 import { BasicUserDetails, User, UserRole } from '../models/User';
 import { AddUserRoleArgs } from '../resolvers/mutations/AddUserRoleMutation';
 import { CreateUserByEmailInviteArgs } from '../resolvers/mutations/CreateUserByEmailInviteMutation';
-import {
-  UpdateUserByOidcSubArgs,
-  UpdateUserByIdArgs,
-} from '../resolvers/mutations/UpdateUserMutation';
+import { UpdateUserByIdArgs } from '../resolvers/mutations/UpdateUserMutation';
 import { UsersArgs } from '../resolvers/queries/UsersQuery';
 
 export interface UserDataSource {
@@ -82,7 +79,6 @@ export interface UserDataSource {
     rorId?: string
   ): Promise<number>;
   update(user: UpdateUserByIdArgs): Promise<User>;
-  updateUserByOidcSub(args: UpdateUserByOidcSubArgs): Promise<User | null>;
   setUserRoles(id: number, roles: number[]): Promise<void>;
   setUserNotPlaceholder(id: number): Promise<User | null>;
   checkScientistToProposal(

apps/backend/src/datasources/mockups/UserDataSource.ts

@@ -9,10 +9,7 @@ import {
 } from '../../models/User';
 import { AddUserRoleArgs } from '../../resolvers/mutations/AddUserRoleMutation';
 import { CreateUserByEmailInviteArgs } from '../../resolvers/mutations/CreateUserByEmailInviteMutation';
-import {
-  UpdateUserByIdArgs,
-  UpdateUserByOidcSubArgs,
-} from '../../resolvers/mutations/UpdateUserMutation';
+import { UpdateUserByIdArgs } from '../../resolvers/mutations/UpdateUserMutation';
 import { UsersArgs } from '../../resolvers/queries/UsersQuery';
 import { UserDataSource } from '../UserDataSource';
 
@@ -400,17 +397,6 @@ export class UserDataSourceMock implements UserDataSource {
     return dummyUser;
   }
 
-  async updateUserByOidcSub(
-    args: UpdateUserByOidcSubArgs
-  ): Promise<User | null> {
-    if (dummyUser.oidcSub === args.oidcSub) {
-      return { ...dummyUser, ...args };
-    }
-
-    // User not found
-    return null;
-  }
-
   async me(id: number) {
     return dummyUser;
   }

apps/backend/src/datasources/postgres/UserDataSource.ts

@@ -14,10 +14,7 @@ import {
 } from '../../models/User';
 import { AddUserRoleArgs } from '../../resolvers/mutations/AddUserRoleMutation';
 import { CreateUserByEmailInviteArgs } from '../../resolvers/mutations/CreateUserByEmailInviteMutation';
-import {
-  UpdateUserByIdArgs,
-  UpdateUserByOidcSubArgs,
-} from '../../resolvers/mutations/UpdateUserMutation';
+import { UpdateUserByIdArgs } from '../../resolvers/mutations/UpdateUserMutation';
 import { UsersArgs } from '../../resolvers/queries/UsersQuery';
 import { UserDataSource } from '../UserDataSource';
 import database, { isUniqueConstraintError } from './database';
@@ -126,61 +123,6 @@ export default class PostgresUserDataSource implements UserDataSource {
     }
   }
 
-  async updateUserByOidcSub(
-    args: UpdateUserByOidcSubArgs
-  ): Promise<User | null> {
-    const {
-      firstname,
-      user_title,
-      lastname,
-      preferredname,
-      gender,
-      birthdate,
-      institutionId,
-      department,
-      position,
-      email,
-      telephone,
-      placeholder,
-      oauthRefreshToken,
-      oauthIssuer,
-    } = args;
-    try {
-      const [userRecord]: UserRecord[] = await database
-        .update({
-          firstname,
-          user_title,
-          lastname,
-          preferredname,
-          gender,
-          birthdate,
-          institution_id: institutionId,
-          department,
-          position,
-          email,
-          telephone,
-          placeholder,
-          oauth_refresh_token: oauthRefreshToken,
-          oauth_issuer: oauthIssuer,
-          updated_at: new Date(),
-        })
-        .from('users')
-        .where('oidc_sub', args.oidcSub)
-        .returning(['*']);
-
-      if (!userRecord) {
-        return null;
-      }
-
-      return createUserObject(userRecord);
-    } catch (error) {
-      if (isUniqueConstraintError(error)) {
-        throw new GraphQLError('User already exists');
-      }
-      throw new GraphQLError('Could not create user. Check your Inputs.');
-    }
-  }
-
   async createInviteUser(args: CreateUserByEmailInviteArgs): Promise<number> {
     const { firstname, lastname, email } = args;
 

apps/backend/src/datasources/stfc/StfcUserDataSource.ts

@@ -9,10 +9,7 @@ import { Role, Roles } from '../../models/Role';
 import { BasicUserDetails, User, UserRole } from '../../models/User';
 import { AddUserRoleArgs } from '../../resolvers/mutations/AddUserRoleMutation';
 import { CreateUserByEmailInviteArgs } from '../../resolvers/mutations/CreateUserByEmailInviteMutation';
-import {
-  UpdateUserByIdArgs,
-  UpdateUserByOidcSubArgs,
-} from '../../resolvers/mutations/UpdateUserMutation';
+import { UpdateUserByIdArgs } from '../../resolvers/mutations/UpdateUserMutation';
 import { UsersArgs } from '../../resolvers/queries/UsersQuery';
 import { Cache } from '../../utils/Cache';
 import PostgresUserDataSource from '../postgres/UserDataSource';
@@ -503,12 +500,6 @@ export class StfcUserDataSource implements UserDataSource {
     throw new Error('Method not implemented.');
   }
 
-  async updateUserByOidcSub(
-    args: UpdateUserByOidcSubArgs
-  ): Promise<User | null> {
-    throw new Error('Method not implemented.');
-  }
-
   async me(id: number) {
     return this.getUser(id);
   }

apps/backend/src/mutations/UserMutations.spec.ts

@@ -11,7 +11,7 @@ import {
   dummyUserOfficerWithRole,
 } from '../datasources/mockups/UserDataSource';
 import { EmailInviteResponse } from '../models/EmailInviteResponse';
-import { isRejection, Rejection } from '../models/Rejection';
+import { isRejection } from '../models/Rejection';
 import { AuthJwtPayload, User, UserRole } from '../models/User';
 import { verifyToken } from '../utils/jwt';
 import UserMutations from './UserMutations';
@@ -238,135 +238,6 @@ test('externalTokenLogin supplies a new JWT', async () => {
   expect(decoded.user.id).toBe(dummyUser.id);
 });
 
-// Tests for updateUserByOidcSub functionality
-describe('updateUserByOidcSub', () => {
-  test('A user can update their own profile by OIDC sub', async () => {
-    const result = await userMutations.updateUserByOidcSub(dummyUserWithRole, {
-      oidcSub: dummyUser.oidcSub as string,
-      firstname: 'UpdatedJane',
-      lastname: 'UpdatedDoe',
-      email: 'updated.jane@example.com',
-      id: dummyUser.id,
-    });
-
-    expect(result).toEqual({
-      ...dummyUser,
-      firstname: 'UpdatedJane',
-      lastname: 'UpdatedDoe',
-      email: 'updated.jane@example.com',
-    });
-  });
-
-  test('A user officer can update another user by OIDC sub', async () => {
-    const result = await userMutations.updateUserByOidcSub(
-      dummyUserOfficerWithRole,
-      {
-        oidcSub: dummyUser.oidcSub as string,
-        firstname: 'OfficerUpdatedJane',
-        department: 'Updated Department',
-        id: dummyUser.id,
-      }
-    );
-
-    expect(result).toEqual({
-      ...dummyUser,
-      firstname: 'OfficerUpdatedJane',
-      department: 'Updated Department',
-    });
-  });
-  test('A user cannot update another user by OIDC sub', async () => {
-    const result = await userMutations.updateUserByOidcSub(
-      dummyUserNotOnProposalWithRole,
-      {
-        oidcSub: dummyUser.oidcSub as string,
-        firstname: 'ShouldNotUpdate',
-        id: dummyUser.id,
-      }
-    );
-
-    expect(isRejection(result)).toBe(true);
-    expect((result as Rejection).reason).toBe(
-      'Can not update user because of insufficient permissions'
-    );
-  });
-
-  test('A not logged in user cannot update a user by OIDC sub', async () => {
-    const result = await userMutations.updateUserByOidcSub(null, {
-      oidcSub: dummyUser.oidcSub as string,
-      firstname: 'ShouldNotUpdate',
-      id: dummyUser.id,
-    });
-
-    expect(isRejection(result)).toBe(true);
-    expect((result as Rejection).reason).toBe('NOT_LOGGED_IN');
-  });
-
-  test('A user can update partial profile data by OIDC sub', async () => {
-    const result = await userMutations.updateUserByOidcSub(dummyUserWithRole, {
-      oidcSub: dummyUser.oidcSub as string,
-      telephone: '+1-555-9999',
-      position: 'Senior Architect',
-      id: dummyUser.id,
-    });
-
-    expect(result).toEqual({
-      ...dummyUser,
-      telephone: '+1-555-9999',
-      position: 'Senior Architect',
-    });
-  });
-
-  test('A user cannot update someone else profile even with their own OIDC sub when trying to update different user', async () => {
-    // Simulate user with different OIDC sub trying to update dummyUser
-    const userWithDifferentOidcSub = {
-      ...dummyUserNotOnProposalWithRole,
-      oidcSub: 'different-oidc-sub',
-    };
-
-    const result = await userMutations.updateUserByOidcSub(
-      userWithDifferentOidcSub,
-      {
-        oidcSub: dummyUser.oidcSub as string,
-        firstname: 'ShouldNotUpdate',
-        id: dummyUser.id,
-      }
-    );
-
-    expect(isRejection(result)).toBe(true);
-    expect((result as Rejection).reason).toBe(
-      'Can not update user because of insufficient permissions'
-    );
-  });
-
-  test('Empty update object should work', async () => {
-    const result = await userMutations.updateUserByOidcSub(dummyUserWithRole, {
-      oidcSub: dummyUser.oidcSub as string,
-      id: dummyUser.id,
-    });
-
-    expect(result).toEqual(dummyUser);
-  });
-
-  test('Update should preserve original user data for unspecified fields', async () => {
-    const result = await userMutations.updateUserByOidcSub(dummyUserWithRole, {
-      oidcSub: dummyUser.oidcSub as string,
-      firstname: 'OnlyFirstName',
-      id: dummyUser.id,
-    });
-
-    expect(result).toEqual({
-      ...dummyUser,
-      firstname: 'OnlyFirstName',
-    });
-
-    // Verify other fields remain unchanged
-    expect(isRejection(result)).toBe(false);
-    expect((result as typeof dummyUser).lastname).toBe(dummyUser.lastname);
-    expect((result as typeof dummyUser).email).toBe(dummyUser.email);
-    expect((result as typeof dummyUser).department).toBe(dummyUser.department);
-  });
-});
-
 describe('upsertUserByOidcSub', () => {
   test('A user can be created if OIDC sub does not exist', async () => {
     const newOidcSub = 'new-unique-oidc-sub';

apps/backend/src/mutations/UserMutations.ts

@@ -10,7 +10,6 @@ import {
 import * as bcrypt from 'bcryptjs';
 import { DateTime } from 'luxon';
 import { inject, injectable } from 'tsyringe';
-import { Args } from 'type-graphql';
 
 import { UserAuthorization } from '../auth/UserAuthorization';
 import { Tokens } from '../config/Tokens';
@@ -33,7 +32,6 @@ import { AddUserRoleArgs } from '../resolvers/mutations/AddUserRoleMutation';
 import { CreateUserByEmailInviteArgs } from '../resolvers/mutations/CreateUserByEmailInviteMutation';
 import {
   UpdateUserRolesArgs,
-  UpdateUserByOidcSubArgs,
   UpdateUserByIdArgs,
 } from '../resolvers/mutations/UpdateUserMutation';
 import { UpsertUserByOidcSubArgs } from '../resolvers/mutations/UpsertUserMutation';
@@ -245,49 +243,6 @@ export default class UserMutations {
       });
   }
 
-  @Authorized()
-  @EventBus(Event.USER_UPDATED)
-  async updateUserByOidcSub(
-    agent: UserWithRole | null,
-    @Args() args: UpdateUserByOidcSubArgs
-  ): Promise<User | Rejection> {
-    const isUpdatingOwnUser = agent?.oidcSub === args.oidcSub;
-    if (
-      !this.userAuth.isApiToken(agent) &&
-      !this.userAuth.isUserOfficer(agent) &&
-      !isUpdatingOwnUser
-    ) {
-      return rejection(
-        'Can not update user because of insufficient permissions',
-        {
-          args,
-          agent,
-          code: ApolloServerErrorCodeExtended.INSUFFICIENT_PERMISSIONS,
-        }
-      );
-    }
-
-    try {
-      const updatedUser = await this.dataSource.updateUserByOidcSub(args);
-
-      if (!updatedUser) {
-        return rejection(
-          'USER_NOT_FOUND',
-          { oidcSub: args.oidcSub },
-          new Error(`User with OIDC sub ${args.oidcSub} not found`)
-        );
-      }
-
-      return updatedUser;
-    } catch (error) {
-      return rejection(
-        'INTERNAL_ERROR',
-        { agent, args },
-        error instanceof Error ? error : new Error(String(error))
-      );
-    }
-  }
-
   @ValidateArgs(getTokenForUserValidationSchema)
   @Authorized()
   async getTokenForUser(

apps/backend/src/resolvers/mutations/UpdateUserMutation.ts

@@ -111,12 +111,4 @@ export class UpdateUserMutation {
   ) {
     return context.mutations.user.setUserNotPlaceholder(context.user, id);
   }
-
-  @Mutation(() => User)
-  updateUserByOidcSub(
-    @Args() input: UpdateUserByOidcSubArgs,
-    @Ctx() context: ResolverContext
-  ) {
-    return context.mutations.user.updateUserByOidcSub(context.user, input);
-  }
 }