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

STCOR-853 do not include credential in /authn/token request #1477

Closed
wants to merge 2 commits into from

Conversation

zburke
Copy link
Member

@zburke zburke commented May 23, 2024

The request to /authn/token pulls an OTP from the query string and exchanges it for AT/RT cookies. If, somehow, the browser already has cookies and sends them along on this request, it causes a negative feedback look because the OTP and the cookies are out of sync. The old AT/RT cookies will cause the endpoint to return 4xx, which will result in a redirect back to keycloak, which will find its (still perfectly valid) authentication cookies, which will cause it redirect back to stripes with a new OTP ... and the cycle repeats.

Thus, when we are exchanging an OTP, we don't want to send any cookies. We want the to send the OTP and have new cookies from the response overwrite anything that was previously stored.

Refs STCOR-853

The request to `/authn/token` pulls an OTP from the query string and
exchanges it for AT/RT cookies. If, somehow, the browser already has
cookies and sends them along on this request, it causes a negative
feedback look because the OTP and the cookies are out of sync.
The old AT/RT cookies will cause the endpoint to return 4xx, which will
result in a redirect back to keycloak, which will find its (still
perfectly valid) authentication cookies, which will cause it redirect
back to stripes with a new OTP ... and the cycle repeats.

Thus, when we are exchanging an OTP, we don't want to send any cookies.
We want the to send the OTP and have new cookies from the response
overwrite anything that was previously stored.

Refs STCOR-853
Copy link

github-actions bot commented May 23, 2024

Jest Unit Test Statistics

228 tests  +2   228 ✔️ +2   56s ⏱️ -3s
  45 suites +1       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 1329419. ± Comparison against base commit bc04288.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 23, 2024

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   10s ⏱️ ±0s
266 tests ±0  260 ✔️ ±0  6 💤 ±0  0 ±0 
269 runs  ±0  263 ✔️ ±0  6 💤 ±0  0 ±0 

Results for commit 1329419. ± Comparison against base commit bc04288.

♻️ This comment has been updated with latest results.

Copy link
Member

@ryandberger ryandberger left a comment

Choose a reason for hiding this comment

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

Glad you could correct this with a small change. Thanks for adding test coverage!

The previous commit re-enabled logout by correctly passing the
`x-okapi-tenant` header in the `/authn/logout` request. It turns out
that if you want read the tenant from the store in a test, you have to
mock the store in your test. WHO KNEW???
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@zburke
Copy link
Member Author

zburke commented May 23, 2024

Like #1478, branches got wacky due to an accidental merge; there should only be one commit on this branch :/ Gonna try re-opening it against current tip of keycloak-ramsons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants