-
Notifications
You must be signed in to change notification settings - Fork 6
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
Made changes so auth on openapi schema works. #143
Conversation
Now requests that require BE to check token will be sent with Bearer JWT from FE as a token.
Deploy preview for hkn-ucsd-portal-dev ready! Built with commit 39eeaab |
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 small thing but overall looks good
@@ -52,7 +52,7 @@ | |||
"storybook": "start-storybook -p 6006 -s public", | |||
"build-storybook": "build-storybook -s public", | |||
"precodegen": "rimraf src/api/*", | |||
"codegen": "npx openapi-generator generate -i http://dev.api.hknucsd.com/api/docs/json -g typescript-fetch --additional-properties=typescriptThreePlus=true -o src/api/" | |||
"codegen": "npx openapi-generator generate -i http://dev.api.hknucsd.com/api/docs/json -g typescript-fetch --additional-properties=typescriptThreePlus=true -o src/services/api" |
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.
oof sorry if this caused issues it's mb :(
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.
np it didn't
src/pages/App/index.js
Outdated
const { claims } = tokenResult; | ||
const { claims, token } = tokenResult; | ||
|
||
ApiConfigStore.setToken(token); |
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.
might also have to call this in setClaims
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.
we could have a state field that's token instead of userRoles - what do you think
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.
setClaims doesn't have access to tokenResult.token though. Also, I use userRoles for the RBAC HOCs so I'd rather it still be there. We can always just have a userToken state field in addition to the other two.
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.
yeah i see ur point about the userRoles, but after a user logins you should be able to get their token
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.
so then we can add the userToken as a state field
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.
Yes that's fine. What about the calling setToken() in setClaims that you said in your first comment of this thread?
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.
yeah cuz after someone signs in we should set the token in setClaims instead of waiting for the firebase authListener to do it
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 ran into some weird issues before doing this
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.
Should I put userToken into userClaims then, since it's an object? Also, if I were to call setToken in setClaims, do I just get the token from this.state.userClaims?
I think the idea is just to longterm drop the concept of claims and rely on the roles that we get from /users/:userID/role.
We can just add token as a string since it’s a jwt string
… On Aug 26, 2020, at 5:11 PM, Thai ***@***.***> wrote:
@thaigillespie commented on this pull request.
In src/pages/App/index.js <#143 (comment)>:
> @@ -42,7 +43,9 @@ class App extends React.Component {
firebase.auth().onAuthStateChanged(async user => {
if (user) {
const tokenResult = await user.getIdTokenResult();
- const { claims } = tokenResult;
+ const { claims, token } = tokenResult;
+
+ ApiConfigStore.setToken(token);
Should I put userToken into userClaims then, since it's an object? Also, if I were to call setToken in setClaims, do I just get the token from this.state.userClaims?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH25RGJKYKMOB4E5UJ2QHB3SCWQENANCNFSM4QML3ILA>.
|
Okay so token will be its own state field then got it. |
Now requests that require BE to check token will be sent with
Bearer JWT from FE as a token.