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.

Sign up for GitHub

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

Jump to bottom

SIMSBIOHUB-331: Keycloak Library Updates #1151

Merged
merged 12 commits into from
Nov 2, 2023
Merged

SIMSBIOHUB-331: Keycloak Library Updates #1151

merged 12 commits into from
Nov 2, 2023

Conversation

NickPhura
Copy link
Collaborator

@NickPhura NickPhura commented Oct 27, 2023 •
edited
Loading

Links to Jira Tickets

https://apps.nrs.gov.bc.ca/int/jira/browse/SIMSBIOHUB-331

Description of Changes

Swap out old keycloak libraries for newer more up to date libraries.

Remove login and logout pages.

Refactor the keycloakWrapper, and update code/tests as necessary.

Testing Notes

Anything to do with logging in/out works as before.

  • Logging in as an IDIR/BCEID user
  • Logging in without access, requesting access, being granted access, etc
  • Logging in with a non-admin system role and only being able to access projects you have a project level role on

@github-actions
Copy link

Openshift URLs for the PR Deployment:

@NickPhura NickPhura added the Staging Will merge into staging branch label Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023 •
edited
Loading

Codecov Report

Merging #1151 (dc41064) into dev-staging (9143004) will decrease coverage by 0.31%.
Report is 8 commits behind head on dev-staging.
The diff coverage is 52.86%.

@@               Coverage Diff               @@
##           dev-staging    #1151      +/-   ##
===============================================
- Coverage        62.13%   61.83%   -0.31%     
===============================================
  Files              554      549       -5     
  Lines            16929    16572     -357     
  Branches          2599     2534      -65     
===============================================
- Hits             10519    10247     -272     
+ Misses            5703     5648      -55     
+ Partials           707      677      -30
Files Coverage Δ
...repositories/administrative-activity-repository.ts 94.73% <ø> (ø)
api/src/repositories/user-repository.ts 59.30% <ø> (ø)
app/src/components/security/Guards.tsx 69.23% <100.00%> (ø)
...pp/src/features/admin/users/AddSystemUsersForm.tsx 15.38% <ø> (ø)
...c/features/admin/users/ReviewAccessRequestForm.tsx 100.00% <ø> (ø)
...src/features/projects/create/CreateProjectForm.tsx 81.81% <ø> (-0.80%) ⬇️
...ojects/participants/AddProjectParticipantsForm.tsx 15.38% <ø> (ø)
.../projects/participants/ProjectParticipantsPage.tsx 0.00% <ø> (ø)
app/src/features/surveys/view/SurveyHeader.tsx 57.44% <100.00%> (ø)
app/src/hooks/api/useAxios.ts 92.00% <100.00%> (ø)
... and 19 more

... and 90 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@NickPhura NickPhura added the Ready For Review PR is ready for review label Oct 27, 2023
curtisupshall
curtisupshall reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@curtisupshall curtisupshall left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great refactor! 👏

app/src/components/security/RouteGuards.tsx Outdated Show resolved Hide resolved
app/src/contexts/useAuthStateContext.tsx Outdated Show resolved Hide resolved
@curtisupshall curtisupshall self-assigned this Oct 27, 2023
@NickPhura
Remove hasSystemRole function from simsUserWrapper, using external ut…
c622928 
…il function instead

Add/update RouteGuards tests.
Add "fix" npm command to app/api level.
al-rosenthal
al-rosenthal reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@al-rosenthal al-rosenthal left a comment

Choose a reason for hiding this comment

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

running this locally I was not able to approve an access request

al-rosenthal
al-rosenthal approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@al-rosenthal al-rosenthal left a comment

Choose a reason for hiding this comment

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

🚜

JeremyQuartech
JeremyQuartech reviewed Oct 31, 2023
View reviewed changes
Copy link
Contributor

@JeremyQuartech JeremyQuartech left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Something I noticed while testing out permissions is that updating my own permission from admin to creator caused the app to crash as well as an infinite refresh loop. I think this is due to not having permission to the endpoint after the request has been processed, and maybe a non-issue.

Update: looks like without these new changes it still causes an app crash, but not a infinite reload loop.

@NickPhura
Fix bug where changing your own system role caused an infinite error.
e9bc050 
Add git action to clean PR builds for PRs against non-base branches
@NickPhura NickPhura marked this pull request as ready for review November 1, 2023 18:55
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 1, 2023

Openshift URLs for the PR Deployment:

curtisupshall
curtisupshall reviewed Nov 1, 2023
View reviewed changes
Copy link
Contributor

@curtisupshall curtisupshall left a comment

Choose a reason for hiding this comment

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

Code looks good; Testing locally...

curtisupshall

This comment was marked as resolved.

Sign in to view

curtisupshall

This comment was marked as resolved.

Sign in to view

@NickPhura
Copy link
Collaborator Author

NickPhura commented Nov 1, 2023 •
edited
Loading

Logging out as an IDIR and then logging back in as a BCeID doesn't appear to work:

image

As discussed, Pushed up a commit to re-add the siteminder logout redirect. Seems to fix the problem.

JeremyQuartech
JeremyQuartech approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@JeremyQuartech JeremyQuartech left a comment

Choose a reason for hiding this comment

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

Tested locally and everything looks good!

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@NickPhura NickPhura merged commit 2876f50 into dev-staging Nov 2, 2023
18 of 20 checks passed
@NickPhura NickPhura deleted the SIMSBIOHUB-331 branch November 2, 2023 00:03
KjartanE pushed a commit that referenced this pull request Nov 6, 2023
@NickPhura @KjartanE
SIMSBIOHUB-331: Keycloak Library Updates (#1151)
ba7af18 
* Swap out old keycloak libraries for newer more up to date libraries.
* Add RouteGuards tests.
* Add git action to clean PR builds for PRs against non-base branches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeremyQuartech JeremyQuartech JeremyQuartech approved these changes

@al-rosenthal al-rosenthal al-rosenthal approved these changes

@KjartanE KjartanE Awaiting requested review from KjartanE

@MacQSL MacQSL Awaiting requested review from MacQSL

@GrahamS-Quartech GrahamS-Quartech Awaiting requested review from GrahamS-Quartech

@curtisupshall curtisupshall Awaiting requested review from curtisupshall

Assignees

@curtisupshall curtisupshall

Labels
Ready For Review PR is ready for review Staging Will merge into staging branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NickPhura @al-rosenthal @curtisupshall @JeremyQuartech