Skip to content

Commit 8cba994

Browse files
djhidjhi
authored
Merge pull request #6144 from marmelab/fix-multiple-notifications-on-checkError
Fix useLogoutIfAccessDenied may notify more than once
2 parents 59f1943 + 49ff2a4 commit 8cba994

File tree

3 files changed

+110
-61
lines changed

3 files changed

+110
-61
lines changed

‎examples/simple/src/dataProvider.tsx

+10
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@ const dataProvider = fakeRestProvider(data, true);
88
const uploadCapableDataProvider = addUploadFeature(dataProvider);
99
const sometimesFailsDataProvider = new Proxy(uploadCapableDataProvider, {
1010
get: (target, name) => (resource, params) => {
11+
// set session_ended=true in localStorage to trigger an API auth error
12+
if (localStorage.getItem('session_ended')) {
13+
const error = new Error('Session ended') as ResponseError;
14+
error.status = 403;
15+
return Promise.reject(error);
16+
}
1117
// add rejection by type or resource here for tests, e.g.
1218
// if (name === 'delete' && resource === 'posts') {
1319
// return Promise.reject(new Error('deletion error'));
@@ -33,4 +39,8 @@ const delayedDataProvider = new Proxy(sometimesFailsDataProvider, {
3339
),
3440
});
3541

42+
interface ResponseError extends Error {
43+
status?: number;
44+
}
45+
3646
export default cacheDataProviderProxy(delayedDataProvider);

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

+62-30
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,27 @@ import useLogout from './useLogout';
99
import useNotify from '../sideEffect/useNotify';
1010
import { AuthProvider } from '../types';
1111

12-
jest.mock('./useLogout');
13-
jest.mock('../sideEffect/useNotify');
12+
let loggedIn = true;
1413

15-
const logout = jest.fn();
16-
useLogout.mockImplementation(() => logout);
17-
const notify = jest.fn();
18-
useNotify.mockImplementation(() => notify);
14+
const authProvider: AuthProvider = {
15+
login: () => {
16+
loggedIn = true;
17+
return Promise.resolve();
18+
},
19+
logout: jest.fn(() => {
20+
loggedIn = false;
21+
return Promise.resolve();
22+
}),
23+
checkAuth: () =>
24+
loggedIn ? Promise.resolve() : Promise.reject('bad method'),
25+
checkError: params => {
26+
if (params instanceof Error && params.message === 'denied') {
27+
return Promise.reject(new Error('logout'));
28+
}
29+
return Promise.resolve();
30+
},
31+
getPermissions: () => Promise.reject('bad method'),
32+
};
1933

2034
const TestComponent = ({
2135
error,
@@ -32,29 +46,24 @@ const TestComponent = ({
3246
return <div>{loggedOut ? '' : 'logged in'}</div>;
3347
};
3448

35-
let loggedIn = true;
49+
jest.mock('./useLogout');
50+
jest.mock('../sideEffect/useNotify');
3651

37-
const authProvider: AuthProvider = {
38-
login: () => Promise.reject('bad method'),
39-
logout: () => {
40-
loggedIn = false;
41-
return Promise.resolve();
42-
},
43-
checkAuth: () =>
44-
loggedIn ? Promise.resolve() : Promise.reject('bad method'),
45-
checkError: params => {
46-
if (params instanceof Error && params.message === 'denied') {
47-
return Promise.reject(new Error('logout'));
48-
}
49-
return Promise.resolve();
50-
},
51-
getPermissions: () => Promise.reject('bad method'),
52-
};
52+
//@ts-expect-error
53+
useLogout.mockImplementation(() => {
54+
const logout = () => authProvider.logout(null);
55+
return logout;
56+
});
57+
const notify = jest.fn();
58+
//@ts-expect-error
59+
useNotify.mockImplementation(() => notify);
5360

5461
describe('useLogoutIfAccessDenied', () => {
5562
afterEach(() => {
56-
logout.mockClear();
63+
//@ts-expect-error
64+
authProvider.logout.mockClear();
5765
notify.mockClear();
66+
authProvider.login('');
5867
});
5968

6069
it('should not logout if passed no error', async () => {
@@ -64,7 +73,7 @@ describe('useLogoutIfAccessDenied', () => {
6473
</AuthContext.Provider>
6574
);
6675
await waitFor(() => {
67-
expect(logout).toHaveBeenCalledTimes(0);
76+
expect(authProvider.logout).toHaveBeenCalledTimes(0);
6877
expect(notify).toHaveBeenCalledTimes(0);
6978
expect(queryByText('logged in')).not.toBeNull();
7079
});
@@ -77,7 +86,7 @@ describe('useLogoutIfAccessDenied', () => {
7786
</AuthContext.Provider>
7887
);
7988
await waitFor(() => {
80-
expect(logout).toHaveBeenCalledTimes(0);
89+
expect(authProvider.logout).toHaveBeenCalledTimes(0);
8190
expect(notify).toHaveBeenCalledTimes(0);
8291
expect(queryByText('logged in')).not.toBeNull();
8392
});
@@ -90,7 +99,7 @@ describe('useLogoutIfAccessDenied', () => {
9099
</AuthContext.Provider>
91100
);
92101
await waitFor(() => {
93-
expect(logout).toHaveBeenCalledTimes(1);
102+
expect(authProvider.logout).toHaveBeenCalledTimes(1);
94103
expect(notify).toHaveBeenCalledTimes(1);
95104
expect(queryByText('logged in')).toBeNull();
96105
});
@@ -104,7 +113,30 @@ describe('useLogoutIfAccessDenied', () => {
104113
</AuthContext.Provider>
105114
);
106115
await waitFor(() => {
107-
expect(logout).toHaveBeenCalledTimes(1);
116+
expect(authProvider.logout).toHaveBeenCalledTimes(1);
117+
expect(notify).toHaveBeenCalledTimes(1);
118+
expect(queryByText('logged in')).toBeNull();
119+
});
120+
});
121+
122+
it('should not send multiple notifications if the errors arrive with a delay', async () => {
123+
let index = 0;
124+
const delayedAuthProvider = {
125+
...authProvider,
126+
checkError: () =>
127+
new Promise<void>((resolve, reject) => {
128+
setTimeout(() => reject(new Error('foo')), index * 100);
129+
index++; // answers immediately first, then after 100ms the second time
130+
}),
131+
};
132+
const { queryByText } = render(
133+
<AuthContext.Provider value={delayedAuthProvider}>
134+
<TestComponent />
135+
<TestComponent />
136+
</AuthContext.Provider>
137+
);
138+
await waitFor(() => {
139+
expect(authProvider.logout).toHaveBeenCalledTimes(2); /// two logouts, but only one notification
108140
expect(notify).toHaveBeenCalledTimes(1);
109141
expect(queryByText('logged in')).toBeNull();
110142
});
@@ -120,7 +152,7 @@ describe('useLogoutIfAccessDenied', () => {
120152
</AuthContext.Provider>
121153
);
122154
await waitFor(() => {
123-
expect(logout).toHaveBeenCalledTimes(1);
155+
expect(authProvider.logout).toHaveBeenCalledTimes(1);
124156
expect(notify).toHaveBeenCalledTimes(0);
125157
expect(queryByText('logged in')).toBeNull();
126158
});
@@ -140,7 +172,7 @@ describe('useLogoutIfAccessDenied', () => {
140172
</AuthContext.Provider>
141173
);
142174
await waitFor(() => {
143-
expect(logout).toHaveBeenCalledTimes(1);
175+
expect(authProvider.logout).toHaveBeenCalledTimes(1);
144176
expect(notify).toHaveBeenCalledTimes(0);
145177
expect(queryByText('logged in')).toBeNull();
146178
});

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

+38-31
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import useAuthProvider from './useAuthProvider';
44
import useLogout from './useLogout';
55
import { useNotify } from '../sideEffect';
66

7-
let authCheckPromise;
7+
let timer;
88

99
/**
1010
* Returns a callback used to call the authProvider.checkError() method
@@ -42,37 +42,44 @@ const useLogoutIfAccessDenied = (): LogoutIfAccessDenied => {
4242
const logout = useLogout();
4343
const notify = useNotify();
4444
const logoutIfAccessDenied = useCallback(
45-
(error?: any, disableNotification?: boolean) => {
46-
// Sometimes, a component might trigger multiple simultaneous
47-
// dataProvider calls which all fail and call this function.
48-
// To avoid having multiple notifications, we first verify if
49-
// a checkError promise is already ongoing
50-
if (!authCheckPromise) {
51-
authCheckPromise = authProvider
52-
.checkError(error)
53-
.then(() => false)
54-
.catch(async e => {
55-
const redirectTo =
56-
e && e.redirectTo
57-
? e.redirectTo
58-
: error && error.redirectTo
59-
? error.redirectTo
60-
: undefined;
61-
logout({}, redirectTo);
62-
const shouldSkipNotify =
63-
disableNotification ||
64-
(e && e.message === false) ||
65-
(error && error.message === false);
66-
!shouldSkipNotify &&
67-
notify('ra.notification.logged_out', 'warning');
45+
(error?: any, disableNotification?: boolean) =>
46+
authProvider
47+
.checkError(error)
48+
.then(() => false)
49+
.catch(async e => {
50+
//manual debounce
51+
if (timer) {
52+
// side effects already triggered in this tick, exit
6853
return true;
69-
})
70-
.finally(() => {
71-
authCheckPromise = undefined;
72-
});
73-
}
74-
return authCheckPromise;
75-
},
54+
}
55+
timer = setTimeout(() => {
56+
timer = undefined;
57+
}, 0);
58+
59+
const shouldNotify = !(
60+
disableNotification ||
61+
(e && e.message === false) ||
62+
(error && error.message === false)
63+
);
64+
if (shouldNotify) {
65+
// notify only if not yet logged out
66+
authProvider
67+
.checkAuth({})
68+
.then(() => {
69+
notify('ra.notification.logged_out', 'warning');
70+
})
71+
.catch(() => {});
72+
}
73+
const redirectTo =
74+
e && e.redirectTo
75+
? e.redirectTo
76+
: error && error.redirectTo
77+
? error.redirectTo
78+
: undefined;
79+
logout({}, redirectTo);
80+
81+
return true;
82+
}),
7683
[authProvider, logout, notify]
7784
);
7885
return authProvider

0 commit comments

Comments
 (0)