diff --git a/.github/workflows/test-rules.yaml b/.github/workflows/test-rules.yaml index 247944b..6fac2e7 100644 --- a/.github/workflows/test-rules.yaml +++ b/.github/workflows/test-rules.yaml @@ -22,4 +22,4 @@ jobs: - name: Install dependencies run: npm ci - name: Test Rules - run: npm run tests + run: npm run tests -- --silent diff --git a/tf/actions/linkUserByEmail.js b/tf/actions/linkUserByEmail.js index 4177ead..fb7a71b 100644 --- a/tf/actions/linkUserByEmail.js +++ b/tf/actions/linkUserByEmail.js @@ -41,10 +41,12 @@ exports.onExecutePostLogin = async (event, api) => { // is case sensitive, we need to search for both situations. In the first search we search by "this" users email // which might be mixed case (or not). Our second search is for the lowercase equivalent but only if two searches // would be different. + // + // This code only ever cares about accounts with verified email addresses. const searchMultipleEmailCases = async () => { - let userAccountsFound = []; + const userAccountsFound = []; - // Push the + // Push the results for the email as specified. userAccountsFound.push( mgmtClient.usersByEmail.getByEmail({ email: event.user.email }) ); @@ -66,48 +68,15 @@ exports.onExecutePostLogin = async (event, api) => { return acc.concat(response.data); }, []); - return mergedDataProfiles; + return mergedDataProfiles.filter((u) => u.email_verified === true); }; - const linkAccount = async (otherProfile) => { - // sanity check if both accounts have LDAP as primary - // we should NOT link these accounts and simply allow the user to continue logging in. - if ( - event.user.user_id.startsWith("ad|Mozilla-LDAP") && - otherProfile.user_id.startsWith("ad|Mozilla-LDAP") - ) { - console.error( - `Error: both ${event.user.user_id} and ${otherProfile.user_id} are LDAP Primary accounts. Linking will not occur.` - ); - return; // Continue with user login without account linking - } - - // LDAP takes priority being the primary identity - // So we need to determine if one or neither are LDAP - // If both are non-primary, linking order doesn't matter - let primaryUser; - let secondaryUser; - - if (event.user.user_id.startsWith("ad|Mozilla-LDAP")) { - primaryUser = event.user; - secondaryUser = otherProfile; - } else { - primaryUser = otherProfile; - secondaryUser = event.user; - } - + const linkAccount = async (primaryUser, secondaryUser) => { // Link the secondary account into the primary account console.log( `Linking secondary identity ${secondaryUser.user_id} into primary identity ${primaryUser.user_id}` ); - // We no longer keep the user_metadata nor app_metadata from the secondary account - // that is being linked. If the primary account is LDAP, then its existing - // metadata should prevail. And in the case of both, primary and secondary being - // non-ldap, account priority does not matter and neither does the metadata of - // the secondary account. - - // Link the accounts try { await mgmtClient.users.link( { id: String(primaryUser.user_id) }, @@ -116,10 +85,6 @@ exports.onExecutePostLogin = async (event, api) => { user_id: secondaryUser.identities[0].user_id, } ); - - // Auth0 Action api object provides a method for updating the current - // authenticated user to the new user_id after account linking has taken place - api.authentication.setPrimaryUser(primaryUser.user_id); } catch (err) { console.error("An unknown error occurred while linking accounts:", err); throw err; @@ -131,10 +96,7 @@ exports.onExecutePostLogin = async (event, api) => { // Main try { // Search for multiple accounts of the same user to link - let userAccountList = await searchMultipleEmailCases(); - - // Ignore non-verified users - userAccountList = userAccountList.filter((u) => u.email_verified); + const userAccountList = await searchMultipleEmailCases(); if (userAccountList.length <= 1) { // The user logged in with an identity which is the only one Auth0 knows about @@ -143,25 +105,71 @@ exports.onExecutePostLogin = async (event, api) => { return; } - if (userAccountList.length === 2) { - // Auth0 is aware of 2 identities with the same email address which means - // that the user just logged in with a new identity that hasn't been linked - // into the other existing identity. Here we pass the other account to the - // linking function + // We found multiple user accounts. Let's break it down: + const userAccountListLdapList = userAccountList.filter((u) => + u.user_id.startsWith("ad|Mozilla-LDAP") + ); - await linkAccount( - userAccountList.filter((u) => u.user_id !== event.user.user_id)[0] + // There should never be more than one LDAP account found with the same name. + // If there is, we wouldn't know what to do in automation, so log it. + if (userAccountListLdapList.length > 1) { + const userstring = userAccountListLdapList + .map((u) => u.user_id) + .join(" "); + console.error( + `Error: ${userstring} are LDAP Primary accounts. Linking will not occur.` + ); + return; // Continue with user login without account linking + } + + // While it's unlikely we'll ever need to merge many accounts in one go, 2024-2026 proved + // we can absolutely get ourselves into such a situation, so let's fix that. Allow + // multiple merges into one main profile. + let mainProfile; + let mergeList; + if (userAccountListLdapList.length === 1) { + // There is only one LDAP profile, so merge the non-LDAP users into the LDAP user. + // This should be an obvious decision. + // We do not keep the user_metadata or app_metadata from the secondary account(s). + // The LDAP account's metadata should prevail. + mainProfile = userAccountListLdapList[0]; + mergeList = userAccountList.filter( + (u) => !u.user_id.startsWith("ad|Mozilla-LDAP") + ); + } else if (userAccountListLdapList.length === 0) { + // There is no LDAP profile, so merge into the other users into the current event's user. + // Think about this one a bit. Clearly if there was an LDAP user, we'd prefer that. + // + // But here, you have a kind of 'now what?' path. Do you: + // * merge an older user into the current-event user? You risk losing any metadata + // from an older user. + // * merge the current user into an older user? You basically prefer the first user + // identity that was ever found, and never move off of it. + // + // Honestly, there's no good choice here. Each has pitfalls. + // CAUTION / DEBT? + // The merge of two non-LDAPs could be a long-lurking bug: if both accounts have groups + // on PMO, that completely desyncs from PMO and then nothing good will happen. + mainProfile = event.user; + mergeList = userAccountList.filter( + (u) => u.user_id !== event.user.user_id ); } else { - // data.length is > 2 which, post November 2020 when all identities were - // force linked manually, shouldn't be possible - var error_message = - `Error linking account ${event.user.user_id} as there are ` + - `over 2 identities with the email address ${event.user.email} ` + - userAccountList.map((x) => x.user_id).join(); + // This is unnecessary but wards off any mistaken fallthroughs. + const error_message = "Impossible Default Case reached"; console.error(error_message); throw new Error(error_message); } + + for (const mergeProfile of mergeList) { + await linkAccount(mainProfile, mergeProfile); + } + + // Auth0 Action api object provides a method for updating the current + // authenticated user to the new user_id after account linking has taken place. + // This can only be called once per transaction, so we call it at the end + // once we've merged all profiles. + api.authentication.setPrimaryUser(mainProfile.user_id); } catch (err) { console.error("An error occurred while linking accounts:", err); return api.access.deny(err.message || String(err)); diff --git a/tf/tests/linkUserByEmail.test.js b/tf/tests/linkUserByEmail.test.js index e9e6ad8..94d7034 100644 --- a/tf/tests/linkUserByEmail.test.js +++ b/tf/tests/linkUserByEmail.test.js @@ -12,16 +12,6 @@ const { onExecutePostLogin } = require("../actions/linkUserByEmail.js"); // Mock auth0 module jest.mock("auth0"); -// Take all log enteries and combine them into a single array -const combineLog = (consoleLogs) => { - let combinedLog = []; - for (let i = 0; i < consoleLogs.length; i++) { - singleStr = consoleLogs[i].join(" "); - combinedLog.push(singleStr); - } - return combinedLog; -}; - beforeEach(() => { // Clone the event object to be used before each test _event = _.cloneDeep(eventObj); @@ -61,7 +51,7 @@ beforeEach(() => { // Mock sendUserTo api.redirect.sendUserTo.mockImplementation((uri) => { - _event.transaction["redirect_uri"] = uri; + _event.transaction.redirect_uri = uri; }); // Mock implementation of ManagementClient @@ -84,19 +74,11 @@ beforeEach(() => { // Mock the constructor of ManagementClient to return the mocked instance auth0Sdk.ManagementClient = jest.fn(() => mockManagementClient); - - // Spy on console - consoleLogSpy = jest.spyOn(console, "log").mockImplementation(() => {}); - consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); - consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); }); afterEach(() => { // Clean up after each test jest.clearAllMocks(); - consoleLogSpy.mockRestore(); - consoleWarnSpy.mockRestore(); - consoleErrorSpy.mockRestore(); }); test("Expect onExecutePostLogin to be defined", async () => { @@ -135,10 +117,10 @@ test("If one usersByEmail is found, expect no account linking", async () => { expect(api.authentication.setPrimaryUser).not.toHaveBeenCalled(); }); -test("If two usersByEmail is found, expect account linking", async () => { +test("If two usersByEmail are found, expect account linking when one is LDAP", async () => { // Define user profiles to return with usersByEmail.getByEmail users = [_ldapUser, _emailUser]; - _event.user = _ldapUser; + _event.user = _.cloneDeep(_ldapUser); // Mock implementation of ManagementClient.usersByEmail.getByEmail() mockManagementClient.usersByEmail.getByEmail.mockImplementation(getByEmail); @@ -146,19 +128,30 @@ test("If two usersByEmail is found, expect account linking", async () => { // Execute onExecutePostLogin await onExecutePostLogin(_event, api); - // Expect linking not to have been called + // Expect linking to have been called + expect(mockManagementClient.users.link).toHaveBeenCalled(); + expect(api.authentication.setPrimaryUser).toHaveBeenCalledTimes(1); + expect(_event.user.user_id).toEqual(_ldapUser.user_id); +}); + +test("If two usersByEmail are found, expect account linking even when neither are LDAP", async () => { + // Define user profiles to return with usersByEmail.getByEmail + users = [_emailUser, _firefoxaccountUser]; + _event.user = _.cloneDeep(_emailUser); + + // Mock implementation of ManagementClient.usersByEmail.getByEmail() + mockManagementClient.usersByEmail.getByEmail.mockImplementation(getByEmail); + + // Execute onExecutePostLogin + await onExecutePostLogin(_event, api); + + // Expect linking to have been called expect(mockManagementClient.users.link).toHaveBeenCalled(); - expect(api.authentication.setPrimaryUser).toHaveBeenCalled(); - - // Collect console logs and expect searchString to exist in logs - const combinedLogs = combineLog(consoleLogSpy.mock.calls); - const searchString = `Linking secondary identity ${_emailUser.user_id} into primary identity ${_event.user.user_id}`; - expect(combinedLogs.some((element) => element.includes(searchString))).toBe( - true - ); + expect(api.authentication.setPrimaryUser).toHaveBeenCalledTimes(1); + expect(_event.user.user_id).toEqual(_emailUser.user_id); }); -test("If more than 2 usersByEmail is found, expect error", async () => { +test("If more than 2 usersByEmail is found, expect account linking", async () => { // Define user profiles to return with usersByEmail.getByEmail users = [_ldapUser, _emailUser, _firefoxaccountUser]; _event.user = _ldapUser; @@ -169,16 +162,11 @@ test("If more than 2 usersByEmail is found, expect error", async () => { // Execute onExecutePostLogin await onExecutePostLogin(_event, api); - // Expect linking not to have been called - expect(mockManagementClient.users.link).not.toHaveBeenCalled(); - expect(api.authentication.setPrimaryUser).not.toHaveBeenCalled(); - - // Collect console logs and expect searchString to exist in logs - const combinedErrorLogs = combineLog(consoleErrorSpy.mock.calls); - const searchString = `Error linking account ${_event.user.user_id} as there are over 2 identities`; - expect( - combinedErrorLogs.some((element) => element.startsWith(searchString)) - ).toBe(true); + // Expect linking to have been called + expect(mockManagementClient.users.link).toHaveBeenCalledTimes(2); + expect(api.authentication.setPrimaryUser).toHaveBeenCalledTimes(1); + // The user_id should be the LDAP account. + expect(_event.user.user_id).toEqual(_ldapUser.user_id); }); test("Test for email of different case, expect account linking", async () => { @@ -195,7 +183,7 @@ test("Test for email of different case, expect account linking", async () => { // Expect linking not to have been called expect(mockManagementClient.users.link).toHaveBeenCalled(); - expect(api.authentication.setPrimaryUser).toHaveBeenCalled(); + expect(api.authentication.setPrimaryUser).toHaveBeenCalledTimes(1); }); test("Test for email of different case and where primary is swapped, expect account linking", async () => { @@ -212,7 +200,7 @@ test("Test for email of different case and where primary is swapped, expect acco // Expect linking not to have been called expect(mockManagementClient.users.link).toHaveBeenCalled(); - expect(api.authentication.setPrimaryUser).toHaveBeenCalled(); + expect(api.authentication.setPrimaryUser).toHaveBeenCalledTimes(1); // Expect Primary user to be set to LDAP user expect(_event.user.user_id).toEqual(_ldapUser.user_id); @@ -233,11 +221,4 @@ test("Test attempting to link 2 LDAP accounts, expect error", async () => { // Expect linking not to have been called expect(mockManagementClient.users.link).not.toHaveBeenCalled(); expect(api.authentication.setPrimaryUser).not.toHaveBeenCalled(); - - // Collect console logs and expect searchString to exist in logs - const combinedErrorLogs = combineLog(consoleErrorSpy.mock.calls); - const searchString = `Error: both ${_event.user.user_id} and ${_firefoxaccountUser.user_id} are LDAP Primary accounts. Linking will not occur.`; - expect( - combinedErrorLogs.some((element) => element.includes(searchString)) - ).toBe(true); });