[ENH] Implemented OIDC flow that results in a query sent to the f-API #205
Conversation
rmanaem
added
the
pr-minor-breaking
✅ Deploy Preview for neurobagel-query ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Interesting behaviour for the oauth prompt: it is written in the local language from where the request is sent. So from Germany it's in German. I changed the string check and also the login prompt language
There was a problem hiding this comment.
Very cool PR @rmanaem! 🎉 Very exciting new functionality!
I read through it and it looks and feels really nice. As you said, there are some things we can refactor / clean up soon, but the logic is really clear and the functionality works cleanly for me. I left some comments here so we can go back and look at them together. But for now I will merge this!
🧑🍳
| <GoogleOAuthProvider clientId="mock-client-id"> | ||
| {' '} | ||
| <AuthDialog isLoggedIn={props.isLoggedIn} onAuth={props.onAuth} /> | ||
| </GoogleOAuthProvider> | ||
| ); |
There was a problem hiding this comment.
Is this a special trick to get the auth provider to run inside the e2e test? Shouldn't hold up the PR, but we should note that down in our notes
cypress/component/AuthDialog.cy.tsx
Outdated
| 'Please login using one of the following before proceeding' | ||
| ); | ||
| cy.get('[data-cy="auth-dialog"]').within(() => { | ||
| cy.contains('Sign in with Google'); |
There was a problem hiding this comment.
Pretty interesting error I ran into here: The "Sign in with Google" uses the language of the country from where you make the request. So the test would fail in Germany because here the string will be
Something to keep in mind for e2e tests. For now I'll switch the check to just "Google"
| function login(credential: string | undefined) { | ||
| setIsLoggedIn(true); | ||
| const jwt: GoogleJWT = credential ? jwtDecode(credential) : ({} as GoogleJWT); | ||
| setIDToken(credential); | ||
| setName(jwt.given_name); | ||
| setProfilePic(jwt.picture); | ||
| } |
There was a problem hiding this comment.
That'd be a good candidate for a custom hook in the future afaict - so we can subscribe to the global "logged in" status with all components that need it.
|
|
||
| const handleClick = (event: React.MouseEvent<HTMLElement>) => { | ||
| setAnchorEl(event.currentTarget); | ||
| }; | ||
| const handleClose = () => { | ||
| setAnchorEl(null); | ||
| }; |
There was a problem hiding this comment.
We should revisit this bit soon and see if by refactoring the whole login section into its own component we can get rid of the custom click handling. Could you open an issue please.
|
|
||
| ReactDOM.createRoot(document.getElementById('root')!).render( | ||
| enableAuth ? <GoogleOAuthProvider clientId={clientID}>{app}</GoogleOAuthProvider> : app | ||
| ); |
| const [isLoggedIn, setIsLoggedIn] = useState<boolean>(false); | ||
| const [name, setName] = useState<string>(''); | ||
| const [profilePic, setProfilePic] = useState<string>(''); | ||
| const [IDToken, setIDToken] = useState<string | undefined>(''); | ||
|
|
There was a problem hiding this comment.
could be good candidate for single state object and reducer
| const action = (snackbarId: SnackbarKey) => ( | ||
| <IconButton | ||
| onClick={() => { | ||
| closeSnackbar(snackbarId); | ||
| }} | ||
| > | ||
| <CloseIcon className="text-white" /> | ||
| </IconButton> | ||
| ); | ||
|
|
There was a problem hiding this comment.
nice - we probably want to also refactor this soon into it's own component though
|
|
||
| function logout() { | ||
| googleLogout(); | ||
| setIsLoggedIn(false); | ||
| setIDToken(''); | ||
| setName(''); | ||
| setProfilePic(''); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think that's also a good reducer candidate for a cleanup / refactor
| const jwt: GoogleJWT = credential ? jwtDecode(credential) : ({} as GoogleJWT); | ||
| setIDToken(credential); | ||
| setName(jwt.given_name); | ||
| setProfilePic(jwt.picture); |
| const response = await axios.get(url, { | ||
| headers: { | ||
| Authorization: `Bearer ${IDToken}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }); | ||
| // TODO: remove this branch once there is no more non-federation option |
There was a problem hiding this comment.
If auth is NOT enabled, does this just send the header anyway, but with an empty ID token? That's probably alright for now, but we should revisit!
Changes proposed in this pull request:
Checklist
This section is for the PR reviewer
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF],[MODEL],[DOC]) (see our Contributing Guidelines for more info)skip-release(to be applied by maintainers only)Closes #XXXXFor new features:
For bug fixes: