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

fix: replace auth with simiplier updated solution #23

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

areisle
Copy link
Collaborator

@areisle areisle commented Jan 28, 2022

this is required to get the client working locally. I've targeted develop, but if we want other changes to go into master before develop is merged this should probably target master instead

KBDEV-960
was unable to narrow down what was actually causing the
infinite redirect loop previously,
but replacing it fixed it

KBDEV-960
was unable to narrow down what was actually causing the
infinite redirect loop previously,
but replacing it fixed it
@areisle areisle requested review from creisle, elewis2 and kttkjl January 28, 2022 04:59
Copy link
Member

@creisle creisle left a comment

Choose a reason for hiding this comment

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

Testing Comments

  • when i click on logout, nothing happens and it auto re-logs me in

src/components/Auth.js Outdated Show resolved Hide resolved
src/components/Auth.js Outdated Show resolved Hide resolved
src/components/Auth.js Outdated Show resolved Hide resolved
src/components/Auth.js Outdated Show resolved Hide resolved
src/components/Auth.js Outdated Show resolved Hide resolved
@creisle creisle added the bug Something isn't working label Feb 1, 2022
@creisle
Copy link
Member

creisle commented Feb 1, 2022

So overall there are a couple of diff error login states / permission states a user can have.

  • missing kc (authorization) token, need to login
  • error during auth/kc
  • has kc token, missing gkb (authorization) token
  • both tokens, user has not signed the license agreement (error, should point them to the about/terms page to sign it)
  • both tokens, insufficient permissions for route

@creisle
Copy link
Member

creisle commented Feb 1, 2022

both tokens, user has not signed the license agreement (error, should point them to the about/terms page to sign it)

for this case I am going to make a separate ticket since it doesn't actually stop them from logging in (#25)

@github-actions

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #23 (62b7609) into develop (3d0cdd9) will decrease coverage by 0.36%.
The diff coverage is 30.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #23      +/-   ##
===========================================
- Coverage    56.22%   55.85%   -0.37%     
===========================================
  Files          110      108       -2     
  Lines         4217     4191      -26     
  Branches      1254     1255       +1     
===========================================
- Hits          2371     2341      -30     
+ Misses        1579     1576       -3     
- Partials       267      274       +7     
Impacted Files Coverage Δ
src/views/AboutView/components/AboutUsageTerms.js 0.00% <0.00%> (ø)
src/views/AboutView/components/Matching/index.js 0.00% <ø> (ø)
src/views/MainView/components/MainAppBar.js 0.00% <0.00%> (ø)
src/components/Auth/index.js 23.07% <23.07%> (ø)
src/views/MainView/components/MainNav.js 51.72% <83.33%> (ø)
src/components/DetailDrawer/index.js 84.52% <100.00%> (-3.58%) ⬇️
src/components/StatementForm/ReviewDialog.js 95.23% <100.00%> (ø)
src/components/StatementForm/index.js 54.60% <100.00%> (ø)
src/components/util.js 65.00% <0.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d0cdd9...62b7609. Read the comment docs.

in v1 there seems to be a bug where mutate variables
are the same as whatever it was called with first
in subsequent calls. this is fixed in v2
@areisle
Copy link
Collaborator Author

areisle commented Feb 1, 2022

Testing Comments

  • when i click on logout, nothing happens and it auto re-logs me in

62b7609

@github-actions

This comment has been minimized.

@areisle
Copy link
Collaborator Author

areisle commented Feb 1, 2022

So overall there are a couple of diff error login states / permission states a user can have.

  • missing kc (authorization) token, need to login

this will redirect to login page

  • error during auth/kc

you mean an error just for keycloak? I assume this would show on the keycloak login page. if it had an error before the redirect, it would display the error though

  • has kc token, missing gkb (authorization) token

displays error

  • both tokens, user has not signed the license agreement (error, should point them to the about/terms page to sign it)

behavior should be unchanged

  • both tokens, insufficient permissions for route

display error

@areisle areisle requested a review from creisle February 1, 2022 21:29
@areisle areisle merged commit 248e73b into develop Feb 2, 2022
@areisle areisle deleted the fix/KBDEV-960-fix-auth branch February 2, 2022 20:00
@github-actions

This comment has been minimized.

@github-actions
Copy link

Unit Test Results

    1 files  ±  0    40 suites   - 2   35s ⏱️ +5s
167 tests  - 14  167 ✔️  - 14  0 💤 ±0  0 ❌ ±0 
163 runs   - 14  163 ✔️  - 14  0 💤 ±0  0 ❌ ±0 

Results for commit 248e73b. ± Comparison against base commit fa69be4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants