diff --git a/.gitignore b/.gitignore index 6e4f3ce9a5..aea1036b67 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,5 @@ dist-ssr *.sln *.sw? .tool-versions + +tmp \ No newline at end of file diff --git a/docs/APIDOCUMENTATION.md b/docs/APIDOCUMENTATION.md index 50a10cf11f..ed330d3445 100644 --- a/docs/APIDOCUMENTATION.md +++ b/docs/APIDOCUMENTATION.md @@ -132,6 +132,10 @@ > | `memberGuid` | required | string | The specific member guid | > | `clientLocale` | optional | string | The locale for the widget | +##### Notes + +> This callback is also used during OAuth flows to synchronize member data when the backend returns a different `inbound_member_guid` than the one used to start the flow (e.g., during non-OAuth to OAuth migrations). When this happens, the widget will fetch the new member record and update its internal state to use the new GUID. + ##### Responses > | http code | content-type | response | @@ -619,7 +623,7 @@ xee > | name | type | data type | description | > | ------------ | -------- | ------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------- | -> | `memberGuid` | optional | string | | +> | `memberGuid` | optional | string | The GUID of the member to update. If provided, the widget will initiate an OAuth update flow for this member. | > | `config` | required | [`ClientConfigType`](../typings/connectProps.d.ts#L19) | The connect widget uses the config to set the initial state and behavior of the widget. [More details](./CLIENT_CONFIG.md) | ##### Responses diff --git a/docs/USER_FEATURES.md b/docs/USER_FEATURES.md index 649018a58a..f9977c878b 100644 --- a/docs/USER_FEATURES.md +++ b/docs/USER_FEATURES.md @@ -27,6 +27,16 @@ const userFeatures = [ | `CONNECT_COMBO_JOBS` | When enabled, the Connect widget will create COMBINATION jobs instead of individual jobs (aggregate, verification, reward, etc). | 
{
 feature_name: 'CONNECT_COMBO_JOBS',
 guid: 'FTR-123', 
 is_enabled: true 
 }
| + +## OAuth Member Synchronization + +When updating a member via OAuth, it is possible for the backend to return a different member GUID (`inbound_member_guid`) than the one used to initiate the flow. This commonly occurs during migrations from non-OAuth to OAuth connections, or when a user signs in with a different set of credentials at the same institution. + +The Connect Widget handles this synchronization automatically by: +1. Detecting the GUID change upon successful completion of the OAuth flow. +2. Fetching the new member's full record using the `loadMemberByGuid` callback. +3. Updating the internal Redux state to reflect the new `currentMemberGuid` and including the new member record in the list of active members. +4. Seamlessly transitioning the user to the `Connecting` step with the synchronized member data.
[<-- Back to README](../README.md#props) diff --git a/src/ConnectWidget.tsx b/src/ConnectWidget.tsx index 97acf4bf03..13a6b193cd 100644 --- a/src/ConnectWidget.tsx +++ b/src/ConnectWidget.tsx @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import React, { createContext, useEffect } from 'react' -import { Provider } from 'react-redux' +import { Provider, useDispatch } from 'react-redux' import Store from 'src/redux/Store' import Connect from 'src/Connect' @@ -19,11 +19,7 @@ interface PostMessageContextType { export const PostMessageContext = createContext({ onPostMessage: () => {} }) -function setupLocalizedContent(localizedContent: Record) { - Store.dispatch(setLocalizedContent(localizedContent)) -} - -export const ConnectWidget = ({ +export const ConnectWidgetWithoutReduxProvider = ({ onPostMessage = () => {}, onAnalyticPageview = () => {}, postMessageEventOverrides, @@ -33,24 +29,30 @@ export const ConnectWidget = ({ }: any) => { initGettextLocaleData(props.language) + const dispatch = useDispatch() + useEffect(() => { - setupLocalizedContent(props?.language?.localizedContent || {}) + dispatch(setLocalizedContent(props?.language?.localizedContent || {})) }, []) return ( - - - - - - {showTooSmallDialog && } - - - - - - + + + + + {showTooSmallDialog && } + + + + + ) } +export const ConnectWidget = (props: any) => ( + + + +) + export default ConnectWidget diff --git a/src/__tests__/ConnectWidget-test.tsx b/src/__tests__/ConnectWidget-test.tsx index 6bb41a5ad5..9c36b31e2f 100644 --- a/src/__tests__/ConnectWidget-test.tsx +++ b/src/__tests__/ConnectWidget-test.tsx @@ -1,13 +1,15 @@ import React from 'react' -import { describe, it, expect, vi } from 'vitest' +import { describe, it, expect, vi, beforeEach } from 'vitest' import { Subject } from 'rxjs' import { act } from '@testing-library/react' +import userEvent from '@testing-library/user-event' -import { ConnectWidget } from '../ConnectWidget' -import { render, screen, waitFor } from 'src/utilities/testingLibrary' +import { ConnectWidgetWithoutReduxProvider } from '../ConnectWidget' +import { render, screen, waitFor, createTestReduxStore } from 'src/utilities/testingLibrary' import { apiValue as apiValueMock } from 'src/const/apiProviderMock' -import { member, JOB_DATA } from 'src/services/mockedData' +import { member, JOB_DATA, OAUTH_STATE, initialState, masterData } from 'src/services/mockedData' import { ReadableStatuses } from 'src/const/Statuses' +import { STEPS } from 'src/const/Connect' // Mock react-confetti to avoid Canvas issues in JSDOM vi.mock('react-confetti', () => ({ @@ -22,6 +24,13 @@ describe('ConnectWidget', () => { language: { locale: 'en', localizedContent: {} }, } + let activeStore = createTestReduxStore() + + beforeEach(() => { + // Reset the active store before each test + activeStore = createTestReduxStore() + }) + it('renders the real Connect widget and handles WebSocket messages correctly', async () => { const webSocketMessages$ = new Subject() const mockWS = { @@ -45,14 +54,14 @@ describe('ConnectWidget', () => { const clientConfig = { mode: 'aggregation', current_member_guid: 'MBR-123' } render( - , - { apiValue: mockApiValue }, + { apiValue: mockApiValue, store: activeStore }, ) // The widget should enter the Connecting state @@ -79,4 +88,84 @@ describe('ConnectWidget', () => { ) }) }) + + it('handles OAuth migration flow where member GUID changes', async () => { + const oldMember = { + ...member.member, + guid: 'MBR-OLD', + is_oauth: true, + connection_status: ReadableStatuses.DENIED, + } + const newMember = { ...member.member, guid: 'MBR-NEW', is_oauth: true, name: 'New Member' } + + const loadOAuthStates = vi + .fn() + .mockResolvedValue([{ ...OAUTH_STATE.oauth_state, guid: 'OAS-123', auth_status: 1 }]) + const loadOAuthState = vi.fn().mockResolvedValue({ + ...OAUTH_STATE.oauth_state, + guid: 'OAS-123', + auth_status: 2, + inbound_member_guid: 'MBR-NEW', + }) + const loadMemberByGuid = vi.fn().mockImplementation((guid) => { + if (guid === 'MBR-OLD') return Promise.resolve(oldMember) + if (guid === 'MBR-NEW') return Promise.resolve(newMember) + return Promise.resolve({}) + }) + + const mockApiValue = { + ...apiValueMock, + loadOAuthStates, + loadOAuthState, + loadMemberByGuid, + } + + const clientConfig = { mode: 'aggregation', current_member_guid: 'MBR-OLD' } + + const preloadedState = { + profiles: initialState.profiles, + connect: { + ...initialState.connect, + members: [oldMember], + currentMemberGuid: 'MBR-OLD', + location: [{ step: STEPS.ENTER_CREDENTIALS }], // Start at OAuth Step + }, + } + + // Create a new store with preloaded state + activeStore = createTestReduxStore(preloadedState) + + render( + , + { + apiValue: mockApiValue, + store: activeStore, + }, + ) + + // Verify we are on OAuth Step + expect(await screen.findByText(/Log in at/i)).toBeInTheDocument() + + // Click continue to start waiting for OAuth + const loginButton = await screen.findByTestId('continue-button') + await userEvent.click(loginButton) + + // Now it should be in WaitingForOAuth, then finish polling, then fetch new member, then go to Connecting + expect(await screen.findByText(/Waiting for permission/i)).toBeInTheDocument() + + // Verify it transitions to Connecting with the NEW GUID + await waitFor( + () => { + expect(screen.getByText(/Connecting to/i)).toBeInTheDocument() + const state = activeStore.getState() + expect(state.connect.currentMemberGuid).toBe('MBR-NEW') + expect(state.connect.members).toContainEqual(newMember) + }, + { timeout: 15000 }, + ) + }, 35000) }) diff --git a/src/const/apiProviderMock.ts b/src/const/apiProviderMock.ts index 8b6d1e12e0..fae532e437 100644 --- a/src/const/apiProviderMock.ts +++ b/src/const/apiProviderMock.ts @@ -22,6 +22,7 @@ export const apiValue: ApiContextTypes = { loadInstitutionByGuid: () => Promise.resolve(institutionData.institution), oAuthStart: () => Promise.resolve(), updateMFA: () => Promise.resolve(member.member), + loadMemberByGuid: () => Promise.resolve(member.member), loadJob: () => Promise.resolve(JOB_DATA), runJob: () => Promise.resolve(member.member), loadOAuthStates: () => Promise.resolve([OAUTH_STATE.oauth_state]), diff --git a/src/redux/actions/Connect.js b/src/redux/actions/Connect.js index e772d1aae8..f8a436b222 100644 --- a/src/redux/actions/Connect.js +++ b/src/redux/actions/Connect.js @@ -102,6 +102,11 @@ export const handleOAuthSuccess = (memberGuid) => ({ payload: memberGuid, }) +export const updateMemberSuccess = (member) => ({ + type: ActionTypes.UPDATE_MEMBER_SUCCESS, + payload: { item: member }, +}) + export const deleteMemberSuccess = (memberGuid) => ({ type: ActionTypes.DELETE_MEMBER_SUCCESS, payload: { memberGuid }, diff --git a/src/utilities/testingLibrary.tsx b/src/utilities/testingLibrary.tsx index 3927686f7a..382ba554e7 100644 --- a/src/utilities/testingLibrary.tsx +++ b/src/utilities/testingLibrary.tsx @@ -86,6 +86,7 @@ const renderWithUser = ( ...options, }), user: userEvent.setup(), + store, } } diff --git a/src/views/oauth/OAuthStep.js b/src/views/oauth/OAuthStep.js index 24ef42d581..4c4107e2c3 100644 --- a/src/views/oauth/OAuthStep.js +++ b/src/views/oauth/OAuthStep.js @@ -2,7 +2,7 @@ import React, { useEffect, useState, useRef, useImperativeHandle, useContext } f import PropTypes from 'prop-types' import { useSelector, useDispatch } from 'react-redux' import { defer, of } from 'rxjs' -import { mergeMap, map, pluck } from 'rxjs/operators' +import { mergeMap, map } from 'rxjs/operators' import { useApi } from 'src/context/ApiContext' import { ReadableStatuses } from 'src/const/Statuses' @@ -123,22 +123,19 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => { let member$ /** - * WARNING: don't change this area without data to back up your changes + * NOTE: We are re-enabling the use of existing member GUIDs for OAuth flows (Update flow). * - * There has been a flip-flop of problems in this area, so this note is being written as a warning. - * Using existing OAuth members causes problems, because if a new set of credentials is used for - * an existing member, our system ends up in a bad state, where the old member gets mangled up with - * the new credentials. + * Historically, this caused "member combination" issues. We now mitigate this by + * detecting if the backend returns a different inbound_member_guid in WaitingForOAuth.js. + * If a change is detected, we fetch the new member record and update the Redux state + * accordingly, ensuring session integrity even if the GUID migrates during the flow. * - * We tried to reduce the amount of members created by re-using existing oauth members, but that caused - * a regression of a client reported bug, so we had to move this back to always creating new members, - * or using existing pending oauth members. - * - * Previous code attempt that was used to reduce member creation, but reintroduced the bug: - * if (member && member.is_oauth && api.getOAuthWindowURI) { - * member$ = of(member) + * The backend will ultimately decide when to send us back the same member guid, or a new one */ - if (pendingOauthMember) { + if (member?.is_oauth && api.getOAuthWindowURI) { + // If there is an existing member, don't create a new one, use that one (restores update flow) + member$ = of(member) + } else if (pendingOauthMember) { // If there is a pending oauth member, don't create a new one, use that one member$ = of(pendingOauthMember) } else { @@ -155,7 +152,7 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => { config, ), ) - .pipe(pluck('member')) + .pipe(map((resp) => resp.member)) .subscribe( (member) => { sendAnalyticsEvent(AnalyticEvents.OAUTH_PENDING_MEMBER_CREATED, { @@ -216,9 +213,14 @@ export const OAuthStep = React.forwardRef((props, navigationRef) => { setIsWaitingForOAuth(false) } - function handleOAuthSuccess(memberGuid) { + function handleOAuthSuccess(memberGuid, member = null) { closeOAuthWindow() - dispatch(connectActions.handleOAuthSuccess(memberGuid)) + + if (member) { + dispatch(connectActions.updateMemberSuccess(member)) + } else { + dispatch(connectActions.handleOAuthSuccess(memberGuid)) + } } function handleOAuthError(memberGuid, errorReason = null) { diff --git a/src/views/oauth/WaitingForOAuth.js b/src/views/oauth/WaitingForOAuth.js index 80ae02cc48..8020775aa4 100644 --- a/src/views/oauth/WaitingForOAuth.js +++ b/src/views/oauth/WaitingForOAuth.js @@ -1,7 +1,7 @@ import React, { useState, useEffect } from 'react' import PropTypes from 'prop-types' import { of, defer } from 'rxjs' -import { map, mergeMap, delay, pluck } from 'rxjs/operators' +import { map, mergeMap, first, filter, catchError } from 'rxjs/operators' import { Text } from '@mxenabled/mxui' import { useTokens } from '@kyper/tokenprovider' @@ -39,6 +39,8 @@ export const WaitingForOAuth = ({ const getNextDelay = getDelay() const { api } = useApi() + const clientLocale = document.querySelector('html')?.getAttribute('lang') || 'en' + useEffect(() => { /** * This gets the most recent PENDING oauth state for the member and polls that @@ -55,26 +57,55 @@ export const WaitingForOAuth = ({ * the oauth state created and know which oauth state to retreive ahead of time. */ const oauthStateCompleted$ = of(member).pipe( - delay(1500), mergeMap(() => defer(() => api.loadOAuthStates({ outbound_member_guid: member.guid, auth_status: OauthState.AuthStatus.PENDING, }), + ).pipe( + map((states) => states?.[0]), + catchError(() => of(null)), ), ), - pluck(0), // get the first response. Should be sorted by newest first + filter((latestState) => !!latestState), mergeMap((latestState) => pollOauthState(latestState.guid, api)), - map((pollingState) => { + mergeMap((pollingState) => { const oauthState = pollingState.currentResponse + const memberGuid = oauthState.inbound_member_guid + + if ( + oauthState.auth_status === OauthState.AuthStatus.SUCCESS && + memberGuid !== member.guid + ) { + /** + * If the inbound member guid is different from our current member guid, + * we need to fetch the new member's record so that we can sync it into + * our redux state. + */ + return defer(() => api.loadMemberByGuid(memberGuid, clientLocale)).pipe( + map((fetchedMember) => ({ + error: false, + memberGuid, + member: fetchedMember, + })), + catchError(() => + of({ + error: true, + errorReason: __('Failed to synchronize member data'), + memberGuid, + }), + ), + ) + } - return { + return of({ error: oauthState.auth_status === OauthState.AuthStatus.ERRORED, errorReason: OauthState.ReadableErrorReason[oauthState.error_reason], - memberGuid: oauthState.inbound_member_guid, - } + memberGuid, + }) }), + first(), ) /** @@ -84,7 +115,7 @@ export const WaitingForOAuth = ({ const sub$ = oauthStateCompleted$.subscribe( (resp) => { if (!resp.error) { - onOAuthSuccess(resp.memberGuid) + onOAuthSuccess(resp.memberGuid, resp.member) } else { onOAuthError(resp.memberGuid, resp.errorReason) } diff --git a/src/views/oauth/__tests__/OAuthStep-test.tsx b/src/views/oauth/__tests__/OAuthStep-test.tsx index 2caf4e9c10..c70460582c 100644 --- a/src/views/oauth/__tests__/OAuthStep-test.tsx +++ b/src/views/oauth/__tests__/OAuthStep-test.tsx @@ -3,6 +3,7 @@ import { render, screen, waitFor } from 'src/utilities/testingLibrary' import { OAuthStep } from 'src/views/oauth/OAuthStep' import { apiValue } from 'src/const/apiProviderMock' import { ApiProvider } from 'src/context/ApiContext' +import { OAUTH_STATE } from 'src/services/mockedData' describe('OauthStep view', () => { describe('Ensure OAuthDefault is rendered', () => { @@ -21,6 +22,7 @@ describe('OauthStep view', () => { connect: { members: [], currentMemberGuid: null, + location: [{ step: 'SEARCH' }], }, }, }, @@ -32,16 +34,81 @@ describe('OauthStep view', () => { await user.click(loginButton) const tryAgainButton = await screen.findByRole('button', { name: 'Try again' }) expect(tryAgainButton).toBeInTheDocument() - waitFor( + await waitFor( async () => { expect(tryAgainButton).not.toBeDisabled() await user.click(tryAgainButton) }, - { timeout: 2500 }, + { timeout: 5000 }, ) - waitFor(() => { - expect(loginButton).toBeInTheDocument() + await screen.findByTestId('continue-button') + }) + + it('should prioritize existing member for OAuth URI generation when currentMemberGuid is present', async () => { + const getOAuthWindowURISpy = vi.spyOn(apiValue, 'getOAuthWindowURI') + const addMemberSpy = vi.spyOn(apiValue, 'addMember') + + const existingMember = { + guid: 'MBR-EXISTING', + institution_guid: 'INS-123', + is_oauth: true, + } + + render( + + + , + { + preloadedState: { + connect: { + members: [existingMember], + currentMemberGuid: 'MBR-EXISTING', + location: [{ step: 'SEARCH' }], + }, + }, + }, + ) + + await waitFor(() => { + expect(getOAuthWindowURISpy).toHaveBeenCalledWith('MBR-EXISTING', expect.anything()) }) + expect(addMemberSpy).not.toHaveBeenCalled() }) + + it('should update Redux state when handleOAuthSuccess is called with a member object from WaitingForOAuth', async () => { + const loadOAuthStates = () => + Promise.resolve([{ ...OAUTH_STATE.oauth_state, guid: 'OAS-123', auth_status: 1 }]) + const loadOAuthState = () => + Promise.resolve({ + ...OAUTH_STATE.oauth_state, + guid: 'OAS-123', + auth_status: 2, + inbound_member_guid: 'MBR-NEW', + }) + const loadMemberByGuid = () => Promise.resolve({ guid: 'MBR-NEW', name: 'New Member' }) + + const { user, store } = render(, { + apiValue: { ...apiValue, loadOAuthStates, loadOAuthState, loadMemberByGuid }, + preloadedState: { + connect: { + members: [], + currentMemberGuid: null, + location: [{ step: 'SEARCH' }], + }, + }, + }) + + const loginButton = await screen.findByTestId('continue-button') + await user.click(loginButton) + + await waitFor( + () => { + const state = store.getState() + expect(state.connect.currentMemberGuid).toBe('MBR-NEW') + expect(state.connect.members).toContainEqual({ guid: 'MBR-NEW', name: 'New Member' }) + }, + { timeout: 30000 }, + ) + }, 35000) }) }) diff --git a/src/views/oauth/__tests__/WaitingForOAuth-test.tsx b/src/views/oauth/__tests__/WaitingForOAuth-test.tsx index 1e72a4e691..5c09f908bb 100644 --- a/src/views/oauth/__tests__/WaitingForOAuth-test.tsx +++ b/src/views/oauth/__tests__/WaitingForOAuth-test.tsx @@ -69,5 +69,31 @@ describe('WaitingForOAuth view', () => { { timeout: 3000 }, ) }) + + it('should call api.loadMemberByGuid and onOAuthSuccess with member if the inbound_member_guid differs from the current member guid', async () => { + const loadOAuthState = () => + Promise.resolve({ + ...OAUTH_STATE.oauth_state, + auth_status: 2, + inbound_member_guid: 'MBR-NEW', + }) + const loadMemberByGuidSpy = vi + .spyOn(apiValue, 'loadMemberByGuid') + .mockResolvedValue({ guid: 'MBR-NEW' }) + + render( + + + , + ) + + await waitFor( + async () => { + expect(loadMemberByGuidSpy).toHaveBeenCalledWith('MBR-NEW', expect.anything()) + expect(defaultProps.onOAuthSuccess).toHaveBeenCalledWith('MBR-NEW', { guid: 'MBR-NEW' }) + }, + { timeout: 3000 }, + ) + }) }) })