-
Notifications
You must be signed in to change notification settings - Fork 26
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
Support SSO #366
Support SSO #366
Conversation
getAccessToken(); | ||
} else if (isAuthenticated) { | ||
if (accountHasBeenFulfilled) { |
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.
Not following why you need to trust the URL to see if the user has completed the account. I think it's possible by checking the metadata only.
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.
Something like this is not working?
if (isAuthenticated && userMetadata) {
getAccessToken();
}
else if (isAuthenticated && !userMetadata) {
const searchParams = new URLSearchParams({
redirectUri: buildAccountFulfilledUri(),
});
// Redirect to: accounts-www for signup on cloud-native
window.location.href = `${redirectAccountUri}?${searchParams}`;
}
else {
loginWithRedirect();
}
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.
No, to get the metadata you need to request a new token (with loginWithRedirect). As you were loged in before having the user_metadata you need to check the url and log in again.
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.
🤔
@@ -1,21 +1,63 @@ | |||
import { useDispatch } from 'react-redux'; |
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.
Not for this PR, but I would like to deprecated template-base-3 for javascript
dispatch(setCredentials({ accessToken })); | ||
}; | ||
if (hasForceLogin) { | ||
// if FORCE_LOGIN_PARAM is present we have to login again |
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.
// if FORCE_LOGIN_PARAM is present we have to login again | |
// if FORCE_LOGIN_PARAM is set a relogin is required to refresh userMetadata |
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.
Very small tweaks in comments requested
https://app.shortcut.com/cartoteam/story/333386/cra-template-signup-with-cn
Depends on: CartoDB/carto-react#750 and https://github.com/CartoDB/cloud-native/pull/13207