fix: refactor institution handling in user authorization and mutation

apps/backend/example.development.env

@@ -52,6 +52,9 @@ TZ=Europe/Stockholm
 DATE_FORMAT=dd-MM-yyyy
 DATE_TIME_FORMAT=dd-MM-yyyy HH:mm
 
+# ROR API URL, used for fetching organization information based on ROR IDs
+# ROR_API_URL=https://api.ror.org/organizations
+
 # The email address of the initial user officer.
 # INITIAL_USER_OFFICER_EMAIL=
 
@@ -67,4 +70,6 @@ DATE_TIME_FORMAT=dd-MM-yyyy HH:mm
 # SINK_EMAIL=sink.email@example.com
 
 # Opentelemetry envs can be found in here https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/
-# OTEL_EXPORTER_OTLP_ENDPOINT="http://localhost:4318/v1/traces"
+# OTEL_EXPORTER_OTLP_ENDPOINT="http://localhost:4318/v1/traces"
+
+ROR_API_URL=https://api.ror.org/organizations

apps/backend/src/auth/OAuthAuthorization.spec.ts

@@ -41,9 +41,9 @@ describe('OAuthAuthorization', () => {
     country = 'testCountry',
   }: {
     user: User;
-    rorId?: string;
-    name?: string;
-    country?: string;
+    rorId?: string | null;
+    name?: string | null;
+    country?: string | null;
   }) =>
     (mockOpenIdClient.login = jest.fn().mockResolvedValue({
       userProfile: {
@@ -128,8 +128,8 @@ describe('OAuthAuthorization', () => {
 
     expect(mockAdminDataSource.createCountry).not.toHaveBeenCalled();
     expect(mockAdminDataSource.createInstitution).toHaveBeenCalledWith({
-      name: 'testName',
-      country: testCountry.countryId,
+      name: 'Unknown institution',
+      country: null,
       rorId: 'testRorId',
     });
     expect(
@@ -161,7 +161,8 @@ describe('OAuthAuthorization', () => {
       .spyOn(mockAdminDataSource, 'createInstitution')
       .mockResolvedValue({ id: expectedInstitutionId } as Institution);
 
-    mockOpenIdLoginResponse({ user: dummyUser });
+    // make dummyUser without ror_id
+    mockOpenIdLoginResponse({ user: dummyUser, rorId: null });
     await oauthAuthorization.externalTokenLogin('valid', '', null);
 
     expect(mockAdminDataSource.createCountry).toHaveBeenCalledWith(
@@ -170,7 +171,7 @@ describe('OAuthAuthorization', () => {
     expect(mockAdminDataSource.createInstitution).toHaveBeenCalledWith({
       name: 'testName',
       country: testCountry.countryId,
-      rorId: 'testRorId',
+      rorId: undefined,
     });
 
     expect(
@@ -202,6 +203,34 @@ describe('OAuthAuthorization', () => {
     ).toBe(dummyUser.institutionId);
   });
 
+  describe('getOrCreateUserInstitution', () => {
+    it('Should create new institution if institution name matches but country does not', async () => {
+      const existingInstitution = new Institution(1, 'Existing Inst', 1);
+      const newCountry = { country: 'New Country', countryId: 2 };
+
+      jest
+        .spyOn(mockAdminDataSource, 'getInstitutionByName')
+        .mockResolvedValue(existingInstitution);
+      jest
+        .spyOn(mockAdminDataSource, 'getCountryByName')
+        .mockResolvedValue(newCountry);
+      jest
+        .spyOn(mockAdminDataSource, 'createInstitution')
+        .mockResolvedValue({ id: 2 } as Institution);
+
+      await oauthAuthorization.getOrCreateUserInstitution({
+        name: existingInstitution.name,
+        country: newCountry.country,
+      });
+
+      expect(mockAdminDataSource.createInstitution).toHaveBeenCalledWith({
+        name: existingInstitution.name,
+        country: newCountry.countryId,
+        rorId: undefined,
+      });
+    });
+  });
+
   describe('upsertUser->create: INITIAL_USER_OFFICER_EMAIL', () => {
     it('should assign USER_OFFICER role if the email is the INITIAL_USER_OFFICER_EMAIL', async () => {
       process.env.INITIAL_USER_OFFICER_EMAIL = dummyUser.email;

apps/backend/src/auth/OAuthAuthorization.ts

@@ -6,22 +6,18 @@ import { OpenIdClient } from '@user-office-software/openid';
 import { ValidTokenSet } from '@user-office-software/openid/lib/model/ValidTokenSet';
 import { ValidUserInfo } from '@user-office-software/openid/lib/model/ValidUserInfo';
 import { GraphQLError } from 'graphql';
-import { UserinfoResponse } from 'openid-client';
 import { container } from 'tsyringe';
 
 import { Tokens } from '../config/Tokens';
 import { AdminDataSource } from '../datasources/AdminDataSource';
+import { Institution } from '../models/Institution';
 import { Rejection } from '../models/Rejection';
 import { SettingsId } from '../models/Settings';
 import { AuthJwtPayload, User, UserRole } from '../models/User';
+import { GetOrCreateInstitutionInput } from '../resolvers/mutations/UpsertUserMutation';
+import { getInstitutionFromRor } from '../services/RorApi';
 import { UserAuthorization } from './UserAuthorization';
 
-interface UserinfoResponseWithInstitution extends UserinfoResponse {
-  institution_ror_id?: string;
-  institution_name?: string;
-  institution_country?: string;
-}
-
 export class OAuthAuthorization extends UserAuthorization {
   private db = container.resolve<AdminDataSource>(Tokens.AdminDataSource);
 
@@ -85,38 +81,82 @@ export class OAuthAuthorization extends UserAuthorization {
     });
   }
 
-  public async getOrCreateUserInstitution(
-    userInfo: UserinfoResponseWithInstitution
-  ) {
-    if (!userInfo.institution_name || !userInfo.institution_country) {
-      return null;
+  private async getOrCreateInstitutionByRorId(
+    rorId: string
+  ): Promise<Institution | null> {
+    let institution = await this.adminDataSource.getInstitutionByRorId(rorId);
+    if (!institution) {
+      const rorData = await getInstitutionFromRor(rorId);
+      let name = 'Unknown institution';
+      let countryId: number | null = null;
+
+      if (rorData) {
+        name = rorData.name;
+        let country = await this.adminDataSource.getCountryByName(
+          rorData.country
+        );
+        if (!country) {
+          logger.logWarn('Country not found in database', {
+            countryName: rorData.country,
+          });
+          country = await this.adminDataSource.createCountry(rorData.country);
+        }
+        countryId = country.countryId;
+      }
+
+      institution = await this.adminDataSource.createInstitution({
+        name: name,
+        country: countryId,
+        rorId: rorId,
+      });
     }
 
-    let institution = userInfo.institution_ror_id
-      ? await this.adminDataSource.getInstitutionByRorId(
-          userInfo.institution_ror_id
-        )
-      : await this.adminDataSource.getInstitutionByName(
-          userInfo.institution_name
-        );
+    return institution;
+  }
+
+  private async getOrCreateInstitutionByManualInput(manualInput: {
+    name: string;
+    country: string;
+  }): Promise<Institution | null> {
+    const institutionName = manualInput.name;
+    const countryName = manualInput.country;
+
+    let country = await this.adminDataSource.getCountryByName(countryName);
+    if (!country) {
+      // create country if it does not exist
+      country = await this.adminDataSource.createCountry(countryName);
+    }
+
+    let institution =
+      await this.adminDataSource.getInstitutionByName(institutionName);
+
+    // If the institution exists but the country does not match, we should create a new institution
+    if (institution && institution.country !== country.countryId) {
+      institution = null;
+    }
 
     if (!institution) {
-      let institutionCountry = await this.adminDataSource.getCountryByName(
-        userInfo.institution_country
-      );
+      institution = await this.adminDataSource.createInstitution({
+        name: institutionName,
+        country: country.countryId,
+        rorId: undefined,
+      });
+    }
 
-      if (!institutionCountry) {
-        institutionCountry = await this.adminDataSource.createCountry(
-          userInfo.institution_country
-        );
+    return institution;
+  }
+
+  public async getOrCreateUserInstitution(input: GetOrCreateInstitutionInput) {
+    let institution: Institution | null = null;
+    if (typeof input === 'string') {
+      // ROR ID provided
+      if (!input || input.trim() === '') {
+        return null;
       }
-      const newInstitution = {
-        name: userInfo.institution_name,
-        country: institutionCountry.countryId,
-        rorId: userInfo.institution_ror_id,
-      };
-      institution =
-        await this.adminDataSource.createInstitution(newInstitution);
+      institution = await this.getOrCreateInstitutionByRorId(input);
+    } else if (input !== null && typeof input === 'object') {
+      // Manual institution details provided
+      institution = await this.getOrCreateInstitutionByManualInput(input);
     }
 
     return institution;
@@ -127,7 +167,17 @@ export class OAuthAuthorization extends UserAuthorization {
     tokenSet: ValidTokenSet
   ): Promise<User> {
     const client = await OpenIdClient.getInstance();
-    const institution = await this.getOrCreateUserInstitution(userInfo);
+    let institutionInput: GetOrCreateInstitutionInput = null;
+    if (userInfo.institution_ror_id) {
+      institutionInput = userInfo.institution_ror_id as string;
+    } else if (userInfo.institution_name && userInfo.institution_country) {
+      institutionInput = {
+        country: userInfo.institution_country as string,
+        name: userInfo.institution_name as string,
+      };
+    }
+
+    const institution = await this.getOrCreateUserInstitution(institutionInput);
     const userId = this.getUniqueId(userInfo);
     const userWithOAuthSubMatch =
       await this.userDataSource.getByOIDCSub(userId);

apps/backend/src/auth/StfcUserAuthorization.ts

@@ -22,6 +22,7 @@ import { Instrument } from '../models/Instrument';
 import { Rejection, rejection } from '../models/Rejection';
 import { Role, Roles } from '../models/Role';
 import { AuthJwtPayload, User, UserWithRole } from '../models/User';
+import { GetOrCreateInstitutionInput } from '../resolvers/mutations/UpsertUserMutation';
 import { Cache } from '../utils/Cache';
 import { StfcUserDataSource } from './../datasources/stfc/StfcUserDataSource';
 import { UserAuthorization } from './UserAuthorization';
@@ -349,11 +350,9 @@ export class StfcUserAuthorization extends UserAuthorization {
     return true;
   }
 
-  getOrCreateUserInstitution(userInfo: {
-    institution_ror_id?: string;
-    institution_name?: string;
-    institution_country?: string;
-  }): Promise<Institution | null> {
+  getOrCreateUserInstitution(
+    _input: GetOrCreateInstitutionInput
+  ): Promise<Institution | null> {
     throw new Error('Method not implemented.');
   }
 }

apps/backend/src/auth/UserAuthorization.ts

@@ -13,6 +13,7 @@ import { Institution } from '../models/Institution';
 import { Rejection } from '../models/Rejection';
 import { Role, Roles } from '../models/Role';
 import { AuthJwtPayload, User, UserWithRole } from '../models/User';
+import { GetOrCreateInstitutionInput } from '../resolvers/mutations/UpsertUserMutation';
 import { AdminDataSource } from './../datasources/AdminDataSource';
 
 export abstract class UserAuthorization {
@@ -198,11 +199,9 @@ export abstract class UserAuthorization {
     iss: string | null
   ): Promise<User | null>;
 
-  abstract getOrCreateUserInstitution(userInfo: {
-    institution_ror_id?: string;
-    institution_name?: string;
-    institution_country?: string;
-  }): Promise<Institution | null>;
+  abstract getOrCreateUserInstitution(
+    institution: GetOrCreateInstitutionInput
+  ): Promise<Institution | null>;
 
   abstract logout(token: AuthJwtPayload): Promise<string | Rejection>;