Skip to content

Commit c46aabf

Browse files
MadeorskMadeorsk
authored
Merge pull request #10949 from marmelab/fix-logout-redirect-priorities
Fix `useLogout` does not redirect to the `checkAuth` call `redirectTo` property
2 parents 6bf3c34 + ceab060 commit c46aabf

File tree

4 files changed

+165
-73
lines changed

4 files changed

+165
-73
lines changed

‎packages/ra-core/src/auth/useLogout.spec.tsx‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import expect from 'expect';
77
import { useGetOne } from '../dataProvider';
88
import useLogout from './useLogout';
99
import { CoreAdminContext } from '../core/CoreAdminContext';
10+
import { Redirect } from './useLogout.stories';
1011

1112
import { TestMemoryRouter } from '../routing';
1213

@@ -53,4 +54,22 @@ describe('useLogout', () => {
5354
queryClient.getQueryData(['posts', 'getOne', { id: '1' }])
5455
).toBeUndefined();
5556
});
57+
it('should redirect to `/login` by default', async () => {
58+
render(<Redirect redirectTo="default" />);
59+
await screen.findByText('Page');
60+
fireEvent.click(screen.getByText('Logout'));
61+
await screen.findByText('Login');
62+
});
63+
it('should redirect to the url returned by the authProvider.logout call', async () => {
64+
render(<Redirect redirectTo="authProvider.logout" />);
65+
await screen.findByText('Page');
66+
fireEvent.click(screen.getByText('Logout'));
67+
await screen.findByText('Custom from authProvider.logout');
68+
});
69+
it('should redirect to the url returned by the caller', async () => {
70+
render(<Redirect redirectTo="caller" />);
71+
await screen.findByText('Page');
72+
fireEvent.click(screen.getByText('Logout'));
73+
await screen.findByText('Custom from useLogout caller');
74+
});
5675
});

‎packages/ra-core/src/auth/useLogout.stories.tsx‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import * as React from 'react';
2+
import { Route, Routes } from 'react-router';
3+
import { TestMemoryRouter } from '../routing/TestMemoryRouter';
4+
import { CoreAdminContext } from '../core/CoreAdminContext';
5+
import useLogout from './useLogout';
6+
7+
export default {
8+
title: 'ra-core/auth/useLogout',
9+
};
10+
11+
export const Redirect = ({
12+
redirectTo,
13+
}: {
14+
redirectTo: 'default' | 'authProvider.logout' | 'caller';
15+
}) => {
16+
const authProvider = {
17+
logout: () =>
18+
Promise.resolve(
19+
redirectTo === 'authProvider.logout'
20+
? '/logout_redirect'
21+
: undefined
22+
),
23+
} as any;
24+
25+
return (
26+
<TestMemoryRouter key={redirectTo}>
27+
<CoreAdminContext authProvider={authProvider}>
28+
<Routes>
29+
<Route path="/" element={<div>Page</div>} />
30+
<Route path="/login" element={<div>Login</div>} />
31+
<Route
32+
path="/logout_redirect"
33+
element={<div>Custom from authProvider.logout</div>}
34+
/>
35+
<Route
36+
path="/caller_redirect"
37+
element={<div>Custom from useLogout caller</div>}
38+
/>
39+
</Routes>
40+
41+
<LogoutButton
42+
redirectTo={
43+
redirectTo === 'caller' ? '/caller_redirect' : undefined
44+
}
45+
/>
46+
</CoreAdminContext>
47+
</TestMemoryRouter>
48+
);
49+
};
50+
51+
Redirect.args = {
52+
redirectTo: 'default',
53+
};
54+
55+
Redirect.argTypes = {
56+
redirectTo: {
57+
type: 'string',
58+
options: ['default', 'authProvider.logout', 'caller'],
59+
mapping: {
60+
default: undefined,
61+
'authProvider.logout': 'authProvider.logout',
62+
caller: 'caller',
63+
},
64+
control: {
65+
type: 'radio',
66+
},
67+
},
68+
};
69+
70+
const LogoutButton = ({ redirectTo }: { redirectTo?: string }) => {
71+
const logout = useLogout();
72+
return (
73+
<button onClick={() => logout(undefined, redirectTo)}>Logout</button>
74+
);
75+
};

‎packages/ra-core/src/auth/useLogout.ts‎

Lines changed: 56 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -60,77 +60,72 @@ const useLogout = (): Logout => {
6060
const logout: Logout = useCallback(
6161
(
6262
params = {},
63-
redirectTo = loginUrl,
63+
redirectFromCaller,
6464
redirectToCurrentLocationAfterLogin = true
6565
) => {
6666
if (authProvider) {
67-
return authProvider
68-
.logout(params)
69-
.then(redirectToFromProvider => {
70-
if (
71-
redirectToFromProvider === false ||
72-
redirectTo === false
73-
) {
74-
resetStore();
75-
queryClient.clear();
76-
// do not redirect
77-
return;
78-
}
67+
return authProvider.logout(params).then(redirectFromLogout => {
68+
if (
69+
redirectFromLogout === false ||
70+
redirectFromCaller === false
71+
) {
72+
resetStore();
73+
queryClient.clear();
74+
// do not redirect
75+
return;
76+
}
7977

80-
const finalRedirectTo =
81-
redirectToFromProvider || redirectTo;
78+
const finalRedirectTo =
79+
redirectFromCaller || redirectFromLogout || loginUrl;
8280

83-
if (finalRedirectTo?.startsWith('http')) {
84-
// absolute link (e.g. https://my.oidc.server/login)
85-
resetStore();
86-
queryClient.clear();
87-
window.location.href = finalRedirectTo;
88-
return finalRedirectTo;
89-
}
81+
if (finalRedirectTo?.startsWith('http')) {
82+
// absolute link (e.g. https://my.oidc.server/login)
83+
resetStore();
84+
queryClient.clear();
85+
window.location.href = finalRedirectTo;
86+
return finalRedirectTo;
87+
}
9088

91-
// redirectTo is an internal location that may contain a query string, e.g. '/login?foo=bar'
92-
// we must split it to pass a structured location to navigate()
93-
const redirectToParts = finalRedirectTo.split('?');
94-
const newLocation: Partial<Path> = {
95-
pathname: redirectToParts[0],
96-
};
97-
let newLocationOptions = {};
89+
// redirectTo is an internal location that may contain a query string, e.g. '/login?foo=bar'
90+
// we must split it to pass a structured location to navigate()
91+
const redirectToParts = finalRedirectTo.split('?');
92+
const newLocation: Partial<Path> = {
93+
pathname: redirectToParts[0],
94+
};
95+
let newLocationOptions = {};
9896

99-
if (
100-
redirectToCurrentLocationAfterLogin &&
101-
locationRef.current &&
102-
locationRef.current.pathname
103-
) {
104-
newLocationOptions = {
105-
state: {
106-
nextPathname: locationRef.current.pathname,
107-
nextSearch: locationRef.current.search,
108-
},
109-
};
110-
}
111-
if (redirectToParts[1]) {
112-
newLocation.search = redirectToParts[1];
113-
}
97+
if (
98+
redirectToCurrentLocationAfterLogin &&
99+
locationRef.current &&
100+
locationRef.current.pathname
101+
) {
102+
newLocationOptions = {
103+
state: {
104+
nextPathname: locationRef.current.pathname,
105+
nextSearch: locationRef.current.search,
106+
},
107+
};
108+
}
109+
if (redirectToParts[1]) {
110+
newLocation.search = redirectToParts[1];
111+
}
114112

115-
// We need to navigate and reset the store after a litte delay to avoid a race condition
116-
// between the store reset and the navigation.
117-
//
118-
// This would only happen when the `authProvider.getPermissions` method returns
119-
// a resolved promise with no delay: If the store was reset before the navigation,
120-
// the `usePermissions` query would reset, causing the `CoreAdminRoutes` component to
121-
// rerender the `LogoutOnMount` component leading to an infinite loop.
122-
setTimeout(() => {
123-
navigateRef.current(
124-
newLocation,
125-
newLocationOptions
126-
);
113+
// We need to navigate and reset the store after a litte delay to avoid a race condition
114+
// between the store reset and the navigation.
115+
//
116+
// This would only happen when the `authProvider.getPermissions` method returns
117+
// a resolved promise with no delay: If the store was reset before the navigation,
118+
// the `usePermissions` query would reset, causing the `CoreAdminRoutes` component to
119+
// rerender the `LogoutOnMount` component leading to an infinite loop.
120+
setTimeout(() => {
121+
navigateRef.current(newLocation, newLocationOptions);
127122

128-
resetStore();
129-
queryClient.clear();
130-
}, 0);
123+
resetStore();
124+
queryClient.clear();
125+
}, 0);
131126

132-
return redirectToFromProvider;
133-
});
127+
return redirectFromLogout;
128+
});
134129
} else {
135130
navigateRef.current(
136131
{

‎packages/ra-core/src/auth/useLogoutIfAccessDenied.ts‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,15 @@ const useLogoutIfAccessDenied = (): LogoutIfAccessDenied => {
5656
);
5757

5858
const logoutIfAccessDenied = useCallback<LogoutIfAccessDenied>(
59-
async (error?: any) => {
59+
async (errorFromCheckAuth?: any) => {
6060
if (!authProvider) {
6161
return logoutIfAccessDeniedWithoutProvider();
6262
}
6363
try {
64-
await authProvider.checkError(error);
64+
await authProvider.checkError(errorFromCheckAuth);
6565
return false;
66-
} catch (e: any) {
67-
const logoutUser = e?.logoutUser ?? true;
66+
} catch (errorFromCheckError: any) {
67+
const logoutUser = errorFromCheckError?.logoutUser ?? true;
6868
// manual debounce
6969
if (timer) {
7070
return true; // side effects already triggered in this tick, exit
@@ -74,15 +74,18 @@ const useLogoutIfAccessDenied = (): LogoutIfAccessDenied => {
7474
}, 0);
7575

7676
const redirectTo =
77-
e && e.redirectTo != null
78-
? e.redirectTo
79-
: error && error.redirectTo
80-
? error.redirectTo
77+
errorFromCheckError &&
78+
errorFromCheckError.redirectTo != null
79+
? errorFromCheckError.redirectTo
80+
: errorFromCheckAuth && errorFromCheckAuth.redirectTo
81+
? errorFromCheckAuth.redirectTo
8182
: undefined;
8283

8384
const shouldNotify = !(
84-
(e && e.message === false) ||
85-
(error && error.message === false) ||
85+
(errorFromCheckError &&
86+
errorFromCheckError.message === false) ||
87+
(errorFromCheckAuth &&
88+
errorFromCheckAuth.message === false) ||
8689
redirectTo?.startsWith('http')
8790
);
8891
if (shouldNotify) {
@@ -92,15 +95,15 @@ const useLogoutIfAccessDenied = (): LogoutIfAccessDenied => {
9295
if (logoutUser) {
9396
notify(
9497
getErrorMessage(
95-
e,
98+
errorFromCheckError,
9699
'ra.notification.logged_out'
97100
),
98101
{ type: 'error' }
99102
);
100103
} else {
101104
notify(
102105
getErrorMessage(
103-
e,
106+
errorFromCheckError,
104107
'ra.notification.not_authorized'
105108
),
106109
{ type: 'error' }

0 commit comments

Comments
 (0)