-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update npm dependencies, Node, GitHub Actions; swap out keycloak-connect for jsonwebtoken #95
Conversation
/** | ||
* Performs JWT verification. | ||
*/ | ||
authenticate: (req, res, next) => { |
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.
Function authenticate
has a Cognitive Complexity of 16 (exceeds 5 allowed). Consider refactoring.
/** | ||
* Performs JWT verification. | ||
*/ | ||
authenticate: (req, res, next) => { |
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.
Function authenticate
has 40 lines of code (exceeds 25 allowed). Consider refactoring.
Code Climate has analyzed commit 0352b94 and detected 2 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 0.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 20.2% (-1.6% change). View more on Code Climate. |
keycloak-connect has since been deprecated
a446f8d
to
0352b94
Compare
/** | ||
* Performs JWT verification. | ||
*/ | ||
authenticate: (req, res, next) => { |
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.
Function authenticate
has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.
/** | ||
* Performs JWT verification. | ||
*/ | ||
authenticate: (req, res, next) => { |
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.
Function authenticate
has 39 lines of code (exceeds 25 allowed). Consider refactoring.
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.
all looks fine to me.
The helm chart/github actions doesnt create a separate config-map and environment variables for the pr deployment.
I did just manually add FRONTEND_KC_PUBLICKEY in openshift (dev) so if the pr is updated it should work now.
But when it's merged to master it should work fine anyway.
healthCheck(req, res, next); | ||
}); | ||
|
||
routes.post('/template/render', keycloak.protect(protector), (req, res, next) => { |
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 this keycloak-protect() function we were using before was only verifying the jwt was from the realm in server.keycloak
properties, and not verifying 'audience'.
But with the new authenticate() function we are instead verifying the jwt came from the realm and has audience
matching attributes of frontend.keyclaok
.
But I think the new pattern is correct.
Description
keycloak-connect
forjsonwebtoken
(it's since been deprecated)https://apps.nrs.gov.bc.ca/int/jira/browse/SHOWCASE-3623
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist
Further comments
N/A