Skip to content
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 #36

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

norrisng-bc
Copy link
Contributor

Description

  • Update Node to latest LTS
  • Update npm dependencies
    • Swap out keycloak-connect for jsonwebtoken (it's since been deprecated)
  • Update GitHub Actions

https://apps.nrs.gov.bc.ca/int/jira/browse/SHOWCASE-3625

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

This PR is largely similar to bcgov/document-generation-showcase#95, but with the addition of unit tests for the replacement authentication middleware.

Copy link

github-actions bot commented Oct 8, 2024

Coverage Report (Application)

Totals Coverage
Statements: 73% ( 219 / 300 )
Methods: 62.86% ( 22 / 35 )
Lines: 77.5% ( 155 / 200 )
Branches: 64.62% ( 42 / 65 )

Copy link

github-actions bot commented Oct 8, 2024

Coverage Report (Frontend)

Totals Coverage
Statements: 28.94% ( 112 / 387 )
Methods: 31.48% ( 34 / 108 )
Lines: 32.09% ( 60 / 187 )
Branches: 19.57% ( 18 / 92 )

Copy link
Contributor

@TimCsaky TimCsaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to summarize, the authentication flow has changed.
with the old keycloak-connect component, we were verifying that the backend was a valid sso client.
.. but now we're ensuring only the frontend sso client is allowed to call our backend (by cross-checking the audience field) .
This feels like a more secure pattern we weren't even verifying the audience before (my bad). So this all looks good now.

..and the unit tests look great. even if the way mocking specific implementations of a function is not a reliable design.. i think it's the best we have.

I now believe the server.keycloak environment variables are redundant and could be removed. but would suggest leaving them there for now. in case we ever use them for something else.

@jatindersingh93 jatindersingh93 merged commit b69eb83 into master Oct 8, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants