-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Reject promise in checkError with logoutUser: false and without redirectTo causes an error #10172 #10177
base: master
Are you sure you want to change the base?
Conversation
…ectTo causes an error marmelab#10172
Hi, thanks for contributing. Can you please first explain the issue? |
Hi ,
As per understanding this issue is caused when redirect is null for some
use cases they don't want to redirect but using this to display an error
message.
…On Mon, Sep 2, 2024, 8:58 AM Gildas Garcia ***@***.***> wrote:
Hi, thanks for contributing. Can you please first explain the issue?
—
Reply to this email directly, view it on GitHub
<#10177 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEO463T6UPCA3YI4OJWNHX3ZURVIHAVCNFSM6AAAAABNOKDW3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRUHAZDGNBUGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your solution seems good to me, but it would be nice if you could prove it works by adding automated tests. Can you please add one?
// If there is not redirect return false saying loguout is not successful | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should rather return true
here, since logout was explicitly not requested, and I'm afraid this will throw another error in this code:
react-admin/packages/ra-core/src/dataProvider/useDataProvider.ts
Lines 124 to 136 in f67649d
return logoutIfAccessDenied(error).then( | |
loggedOut => { | |
if (loggedOut) | |
return { | |
data: arrayReturnTypes.includes( | |
type | |
) | |
? [] | |
: {}, | |
}; | |
throw error; | |
} | |
); |
if (logoutUser) { | ||
logout({}, redirectTo); | ||
} else { | ||
if (redirectTo.startsWith('http')) { | ||
// absolute link (e.g. https://my.oidc.server/login) | ||
window.location.href = redirectTo; | ||
if (redirectTo != null) { | ||
if (redirectTo.startsWith('http')) { | ||
// absolute link (e.g. https://my.oidc.server/login) | ||
window.location.href = redirectTo; | ||
} else { | ||
// internal location | ||
navigate(redirectTo); | ||
} | ||
} else { | ||
// internal location | ||
navigate(redirectTo); | ||
// If there is not redirect return false saying loguout is not successful | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better instead of using this nested if else condition we can make functions and use. This will help others to understand more easily and help to maintain the clean code e.g:
const handleRedirect = (url) => {
if (url.startsWith('http')) {
window.location.href = url;
} else {
navigate(url);
}
};
if (logoutUser) {
logout({}, redirectTo);
} else if (redirectTo) {
handleRedirect(redirectTo);
} else {
return false;
}
};
Add null check for redirect and When redirect is not provided, I returned false and let the called handles the redirect.
This may not be ideal solution, happy to discuss as this is my my first contribution.
Fixes #10172