Skip to content

Commit

Permalink
fix: unexpected error when handling sign-in callback in React and Vue…
Browse files Browse the repository at this point in the history
… SDKs
  • Loading branch information
charIeszhao committed Jul 6, 2023
1 parent 3b0cd4a commit 856a973
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 39 deletions.
6 changes: 6 additions & 0 deletions .changeset/curvy-ways-confess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@logto/react": patch
"@logto/vue": patch
---

Fix potential infinite loop when handling sign-in callback in React and Vue SDKs
50 changes: 21 additions & 29 deletions packages/react/src/hooks/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,17 @@ import LogtoClient from '@logto/browser';
import { renderHook, act, waitFor } from '@testing-library/react';
import { type ReactNode } from 'react';

import { LogtoContext, type LogtoContextProps } from '../context.js';
import { LogtoProvider } from '../provider.js';

import { useHandleSignInCallback, useLogto } from './index.js';

const isAuthenticated = jest.fn(() => false);
const isSignInRedirected = jest.fn(() => false);
const isAuthenticated = jest.fn(async () => false);
const isSignInRedirected = jest.fn(async () => false);
const handleSignInCallback = jest.fn().mockResolvedValue(undefined);
const getAccessToken = jest.fn(() => {
throw new Error('not authenticated');
});

const notImplemented = () => {
throw new Error('Not implemented');
};

jest.mock('@logto/browser', () => {
return jest.fn().mockImplementation(() => {
return {
Expand All @@ -40,6 +35,10 @@ const createHookWrapper =
<LogtoProvider config={{ endpoint, appId }}>{children}</LogtoProvider>;

describe('useLogto', () => {
afterEach(() => {
handleSignInCallback.mockRestore();
});

it('should throw without using context provider', () => {
expect(() => renderHook(useLogto)).toThrow();
});
Expand Down Expand Up @@ -73,41 +72,34 @@ describe('useLogto', () => {
});

it('should not call `handleSignInCallback` when it is not in callback url', async () => {
await act(async () => {
renderHook(useHandleSignInCallback, {
wrapper: createHookWrapper(),
});
const { result } = renderHook(useHandleSignInCallback, {
wrapper: createHookWrapper(),
});

await waitFor(() => {
// LogtoClient is initialized
expect(result.current.isLoading).toBe(false);
expect(result.current.isAuthenticated).toBe(false);
});

expect(handleSignInCallback).not.toHaveBeenCalled();
});

it('should not call `handleSignInCallback` when it is authenticated', async () => {
const mockClient = new LogtoClient({ endpoint, appId });
const mockContext: LogtoContextProps = {
logtoClient: mockClient,
isAuthenticated: true, // Mock this to true by default
loadingCount: 1,
setIsAuthenticated: notImplemented,
setLoadingCount: notImplemented,
setError: notImplemented,
};

isSignInRedirected.mockReturnValueOnce(true);
isAuthenticated.mockReturnValueOnce(true);
isSignInRedirected.mockResolvedValueOnce(true);
isAuthenticated.mockResolvedValueOnce(true);

await act(async () => {
renderHook(useHandleSignInCallback, {
wrapper: ({ children }: { children?: ReactNode }) => (
<LogtoContext.Provider value={mockContext}>{children}</LogtoContext.Provider>
),
wrapper: createHookWrapper(),
});
});

expect(handleSignInCallback).not.toHaveBeenCalled();
});

it('should call `handleSignInCallback` when navigated back to predefined callback url', async () => {
isSignInRedirected.mockReturnValueOnce(true);
isSignInRedirected.mockResolvedValueOnce(true);

const { result } = renderHook(useHandleSignInCallback, {
wrapper: createHookWrapper(),
Expand All @@ -116,17 +108,17 @@ describe('useLogto', () => {
expect(result.current.isAuthenticated).toBe(true);
});
expect(handleSignInCallback).toHaveBeenCalledTimes(1);
handleSignInCallback.mockRestore();
});

it('should call `handleSignInCallback` only once even if it fails internally', async () => {
isSignInRedirected.mockReturnValueOnce(true);
isSignInRedirected.mockResolvedValueOnce(true);
handleSignInCallback.mockRejectedValueOnce(new Error('Oops'));

const { result } = renderHook(useHandleSignInCallback, {
wrapper: createHookWrapper(),
});
await waitFor(() => {
expect(result.current.isLoading).toBe(false);
expect(result.current.isAuthenticated).toBe(false);
});
expect(handleSignInCallback).toHaveBeenCalledTimes(1);
Expand Down
16 changes: 12 additions & 4 deletions packages/react/src/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,19 @@ const useHandleSignInCallback = (callback?: () => void) => {
);

useEffect(() => {
const currentPageUrl = window.location.href;

if (!isAuthenticated && logtoClient?.isSignInRedirected(currentPageUrl)) {
void handleSignInCallback(currentPageUrl);
if (!logtoClient) {
return;
}

(async () => {
const currentPageUrl = window.location.href;
const isAuthenticated = await logtoClient.isAuthenticated();
const isRedirected = await logtoClient.isSignInRedirected(currentPageUrl);

if (!isAuthenticated && isRedirected) {
void handleSignInCallback(currentPageUrl);
}
})();
}, [handleSignInCallback, isAuthenticated, logtoClient]);

return {
Expand Down
6 changes: 3 additions & 3 deletions packages/vue/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { useLogto, useHandleSignInCallback, createLogto } from './index.js';
import { createPluginMethods } from './plugin.js';

const isAuthenticated = jest.fn(async () => false);
const isSignInRedirected = jest.fn(() => false);
const isSignInRedirected = jest.fn(async () => false);
const handleSignInCallback = jest.fn().mockResolvedValue(undefined);
const mockedFetchUserInfo = jest.fn().mockResolvedValue({ sub: 'foo' });
const getAccessToken = jest.fn(() => {
Expand Down Expand Up @@ -153,7 +153,7 @@ describe('useHandleSignInCallback', () => {
});

it('should call `handleSignInCallback` if current url is callback url', async () => {
isSignInRedirected.mockImplementationOnce(() => true);
isSignInRedirected.mockResolvedValueOnce(true);
const { signIn } = useLogto();
useHandleSignInCallback();

Expand All @@ -164,7 +164,7 @@ describe('useHandleSignInCallback', () => {

it('should call `handleSignInCallback` only once even if it fails internally', async () => {
expect(handleSignInCallback).toHaveBeenCalledTimes(0);
isSignInRedirected.mockImplementationOnce(() => true);
isSignInRedirected.mockResolvedValueOnce(true);
handleSignInCallback.mockRejectedValueOnce(new Error('Oops'));
const { signIn } = useLogto();
useHandleSignInCallback();
Expand Down
10 changes: 8 additions & 2 deletions packages/vue/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,16 @@ export const useHandleSignInCallback = (callback?: () => void) => {
const { isAuthenticated, isLoading, logtoClient, error } = context;
const { handleSignInCallback } = createPluginMethods(context);

watchEffect(() => {
watchEffect(async () => {
if (!logtoClient.value) {
return;
}

const currentPageUrl = window.location.href;
const isAuthenticated = await logtoClient.value.isAuthenticated();
const isRedirected = await logtoClient.value.isSignInRedirected(currentPageUrl);

if (!isAuthenticated.value && logtoClient.value?.isSignInRedirected(currentPageUrl)) {
if (!isAuthenticated && isRedirected) {
void handleSignInCallback(currentPageUrl, callback);
}
});
Expand Down
2 changes: 1 addition & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 856a973

Please sign in to comment.