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 credentials loading after logging out #6938

Merged
merged 7 commits into from
Feb 21, 2023
Merged

Conversation

drewvolz
Copy link
Member

@drewvolz drewvolz commented Feb 12, 2023

Closes #6928

Summary

useCredentials calls loadCredentials which throws an error when the credentials are false. This blocks the logout state from finishing. We can fix this by changing two things:

  1. Remove the throw statement
  2. Convince everything else that consumes useCredentials that false is an expected value

Working demo can be seen in screen recording attached in #6939

Understanding the problem

If useCredentials --> useQuery --> loadCredentials --> throws

It wouldn't get the new values. Ever. Right?

So things are invalidated, which is why we get the new correct UI after dismissing settings

  • We are throwing for no credentials in signing in, which is good, and our UI prevents us from hitting that
  • We only need to throw for no credentials when signing in (performLogin) but not when requesting the credentials
  • loadCredentials should never be allowed to throw
  • But we need to still guarantee to the consumers that "false" is an okay value

Testing the fix

  1. Add the following routes to ccc-server:
if (process.env.NODE_ENV !== 'production') {
    api.redirect('/login', 'https://stolaf.api.frogpond.tech')
    api.redirect('/login/error', 'https://stolaf.api.frogpond.tech/?error=login')
}
  1. Replace the ky client in source/lib/login.ts with our local client
+import { client } from '@frogpond/api'

-const loginResponse = await ky(OLECARD_AUTH_URL, {
+const loginResponse = await client.post('login', {
  1. Start up CCC
cd ccc-server
npm run stolaf-college
  1. Configure All About Olaf to use ccc
  1. Settings > Server UrL
  2. Type http://localhost:3000/v1
  3. Enter / Save
  1. Switching between responses and observing the outcome
ky.post(OLECARD_AUTH_URL)
client.post('login')
client.post('login/error')

We see that

  • ky correctly shows us a 403 error with invalid credentials (302 Found > 403 Login Required)
  • mocked client lets us authenticate and then logout
  • mocked client error prevents login

drewvolz and others added 2 commits February 11, 2023 18:22
* prevent throwing an error from within loadCredentials
* update QueryFnData and performLogin for falsey login credentials

Co-authored-by: Hawken Rives <hawkrives@fastmail.fm>
* fix URLSearchParams.get error, requires a polyfill in react native
* search for "error" instead of "message" in login redirect

Co-authored-by: Hawken Rives <hawkrives@fastmail.fm>
source/lib/login.ts Outdated Show resolved Hide resolved
source/lib/login.ts Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #6938 (7f3d5b8) into master (85b32b0) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #6938   +/-   ##
======================================
  Coverage    8.47%   8.48%           
======================================
  Files         303     303           
  Lines        5119    5113    -6     
  Branches     1375    1373    -2     
======================================
  Hits          434     434           
+ Misses       4663    4651   -12     
- Partials       22      28    +6     

source/lib/login.ts Outdated Show resolved Hide resolved
source/lib/login.ts Outdated Show resolved Hide resolved
source/views/sis/balances.tsx Outdated Show resolved Hide resolved
source/views/sis/balances.tsx Outdated Show resolved Hide resolved
source/views/stoprint/print-release.tsx Outdated Show resolved Hide resolved
source/views/stoprint/query.ts Outdated Show resolved Hide resolved
source/views/stoprint/query.ts Outdated Show resolved Hide resolved
source/views/stoprint/query.ts Outdated Show resolved Hide resolved
source/views/stoprint/query.ts Outdated Show resolved Hide resolved
Co-authored-by: Hawken Rives <hawkrives@fastmail.fm>
@drewvolz drewvolz requested a review from rye February 15, 2023 06:07
@drewvolz drewvolz merged commit 60f820e into master Feb 21, 2023
@drewvolz drewvolz deleted the drew/fix-credentials-logout branch February 21, 2023 05:43
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.

Settings attempts to login without credentials
3 participants