Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDS-2245: login strict mode issues #1273

Merged
merged 4 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
#### Added

- [Component] What is added?
- [Login] Added a utility function to detect login callback error that could be ignored.

#### Changed

Expand All @@ -26,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- [Component] What bugs/typos are fixed?
- [Footer] Fix Koros height issue (Calm type was of wrong height)
- [Login] Fixed initialization failure in React strict mode when external modules are used.

### Core

Expand Down Expand Up @@ -99,6 +101,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- [Component] What bugs/typos are fixed?

### Hds-js

#### Added

- [login] Added a utility function to detect login callback error that could be ignored.

## [3.7.0] - April, 11, 2024

### React
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import React from 'react';
import { render } from '@testing-library/react';
import { render, waitFor } from '@testing-library/react';
import { disableFetchMocks, enableFetchMocks } from 'jest-fetch-mock';

import { getDefaultOidcClientTestProps } from '../testUtils/oidcClientTestUtil';
import { LoginContextProvider } from './LoginContext';
import { LoginCallbackHandler } from './LoginCallbackHandler';
import { isHandlingLoginCallbackError } from './LoginCallbackHandler.util';
import { Beacon, ConnectedModule } from '../beacon/beacon';
import { OidcClient, OidcClientState, oidcClientStates } from '../client';
import { getLastMockCallArgs } from '../../../utils/testHelpers';
Expand All @@ -19,13 +20,18 @@ loginProps.userManagerSettings.automaticSilentRenew = false;
const onError = jest.fn();
const onSuccess = jest.fn();

const getLastError = () => getLastMockCallArgs(onError)[0];
const getReturnedUser = () => getLastMockCallArgs(onSuccess)[0];

beforeEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.runOnlyPendingTimers();
jest.useRealTimers();
jest.restoreAllMocks();
onError.mockReset();
onSuccess.mockReset();
sessionStorage.clear();
});

Expand All @@ -40,6 +46,7 @@ afterAll(() => {
describe('LoginCallbackHandler', () => {
const notificationText = 'Logging in...';
let beacon: Beacon;
const handleError = new Error('Handlecallback failed');
const renderComponent = (state: OidcClientState, returnUser: boolean) => {
const helperModule: ConnectedModule = {
namespace: 'helper',
Expand All @@ -51,7 +58,7 @@ describe('LoginCallbackHandler', () => {
jest.spyOn(oidcClient, 'getState').mockReturnValue(state);
jest.spyOn(oidcClient, 'getUser').mockReturnValue(validUser);
if (!returnUser) {
jest.spyOn(oidcClient, 'handleCallback').mockRejectedValue(new Error('failed'));
jest.spyOn(oidcClient, 'handleCallback').mockRejectedValue(handleError);
} else {
jest.spyOn(oidcClient, 'handleCallback').mockResolvedValue(validUser);
}
Expand All @@ -72,21 +79,45 @@ describe('LoginCallbackHandler', () => {

it('calls onSuccess with user object, if user already exists', async () => {
renderComponent(oidcClientStates.VALID_SESSION, true);
expect(onSuccess).toHaveBeenCalledTimes(1);
expect(isValidUser(getLastMockCallArgs(onSuccess)[0])).toBeTruthy();
await waitFor(() => {
expect(onSuccess).toHaveBeenCalledTimes(1);
});
expect(isValidUser(getReturnedUser())).toBeTruthy();
});
it(`calls onError, if oidc client state is other than ${oidcClientStates.NO_SESSION} or ${oidcClientStates.VALID_SESSION} `, async () => {
renderComponent(oidcClientStates.LOGGING_IN, true);
expect(onError).toHaveBeenCalledTimes(1);
await waitFor(() => {
expect(onError).toHaveBeenCalledTimes(1);
});
const error = getLastError() as OidcClientError;
expect(isHandlingLoginCallbackError(error)).toBeFalsy();
expect(error.isInvalidUserError).toBeFalsy();
expect(error.isRenewalError).toBeFalsy();
expect(error.isSignInError).toBeTruthy();
});
it(`isHandlingLoginCallbackError returns true when error.type === SIGNING_ERROR and error.message is certain.`, async () => {
renderComponent(oidcClientStates.HANDLING_LOGIN_CALLBACK, false);
await waitFor(() => {
expect(onError).toHaveBeenCalledTimes(1);
});
const error = getLastError() as OidcClientError;
expect(isHandlingLoginCallbackError(error)).toBeTruthy();
expect(error.isInvalidUserError).toBeFalsy();
expect(error.isRenewalError).toBeFalsy();
expect(error.isSignInError).toBeTruthy();
});
it('calls onSuccess when no session exists and handleCallback succeeds', async () => {
renderComponent(oidcClientStates.NO_SESSION, true);
expect(onSuccess).toHaveBeenCalledTimes(1);
expect(isValidUser(getLastMockCallArgs(onSuccess)[0])).toBeTruthy();
await waitFor(() => {
expect(onSuccess).toHaveBeenCalledTimes(1);
});
expect(isValidUser(getReturnedUser())).toBeTruthy();
});
it('calls onError when no session exists and handleCallback fails', async () => {
renderComponent(oidcClientStates.NO_SESSION, false);
expect(onError).toHaveBeenCalledTimes(1);
expect((getLastMockCallArgs(onError)[0] as OidcClientError).isSignInError).toBeTruthy();
await waitFor(() => {
expect(onError).toHaveBeenCalledTimes(1);
});
expect(getLastError()).toBe(handleError);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { User } from 'oidc-client-ts';
import { useOidcClient } from '../client/hooks';
import { OidcClientError, oidcClientErrors } from '../client/oidcClientError';
import { oidcClientStates } from '../client';
import { createCallbackStateErrorMessage } from './LoginCallbackHandler.util';

type LoginCallbackHandlerProps = {
children: React.ReactNode | React.ReactNode[] | null;
Expand Down Expand Up @@ -34,12 +35,7 @@ export function LoginCallbackHandler({
} else if (getState() === oidcClientStates.VALID_SESSION) {
onSuccess(getUser() as User);
} else {
onError(
new OidcClientError(
`Current state (${getState()}) cannot be handled by a callback`,
oidcClientErrors.SIGNIN_ERROR,
),
);
onError(new OidcClientError(createCallbackStateErrorMessage(getState()), oidcClientErrors.SIGNIN_ERROR));
}
// eslint-disable-next-line react/jsx-no-useless-fragment
return <>{children}</>;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { OidcClientError } from '../client/oidcClientError';
import { OidcClientState, oidcClientStates } from '../client';

export function createCallbackStateErrorMessage(state: OidcClientState) {
return `Current state (${state}) cannot be handled by a callback`;
}

export function isHandlingLoginCallbackError(error: OidcClientError): boolean {
return (
error.isSignInError && error.message === createCallbackStateErrorMessage(oidcClientStates.HANDLING_LOGIN_CALLBACK)
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const LoginProvider = ({
apiTokensClientSettings,
sessionPollerSettings,
debug,
modules = [],
modules,
children,
}: React.PropsWithChildren<LoginProviderProps>) => {
const loginProps: OidcClientProps = {
Expand All @@ -35,14 +35,15 @@ export const LoginProvider = ({
debug,
};

const mods = modules || [];
if (sessionPollerSettings) {
modules.push(createSessionPoller(sessionPollerSettings));
mods.push(createSessionPoller(sessionPollerSettings));
}
if (apiTokensClientSettings) {
modules.push(createApiTokenClient(apiTokensClientSettings));
mods.push(createApiTokenClient(apiTokensClientSettings));
}
return (
<LoginContextProvider loginProps={loginProps} modules={modules}>
<LoginContextProvider loginProps={loginProps} modules={mods}>
{children}
</LoginContextProvider>
);
Expand Down
3 changes: 3 additions & 0 deletions packages/react/src/components/login/index.vanilla-js.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export * from './apiTokensClient/apiTokensClientError';
export * from './client/oidcClientError';
export * from './sessionPoller/sessionPollerError';

// utils
export { isHandlingLoginCallbackError } from './components/LoginCallbackHandler.util';

// events
export { apiTokensClientEvents } from './apiTokensClient/index';
export { oidcClientEvents } from './client/index';
Expand Down
2 changes: 2 additions & 0 deletions site/src/docs/components/login/api.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ Button text is passed and rendered as a child.
| `onSuccess` | **Required**. The function is called when a response is handled successfully. The function is called with a <OidcClientTsLink>User</OidcClientTsLink> object. |
| [Table 3: Properties of the LoginCallbackHandler component] |

In some scenarios, when the component is rendered multiple times, usually in development mode with strict mode enabled, the `onError` callback is called with an error that can usually be ignored. The error indicates a login callback is already being handled. The error can be checked with the exported function `isHandlingLoginCallbackError(error)`.

#### LoginProvider

| Property | Description |
Expand Down
2 changes: 1 addition & 1 deletion site/src/docs/components/login/usage.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ return (

</PlaygroundPreview>

All component properties are listed on the <ApiPageAnchorLink anchor="logincallbackhandler">API page</ApiPageAnchorLink>.
All component properties are listed on the <ApiPageAnchorLink anchor="logincallbackhandler">API page</ApiPageAnchorLink>. See also a note about `isHandlingLoginCallbackError(error)`.

#### LoginProvider

Expand Down
Loading