fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow #20202

austin5219 · 2024-10-02T16:22:27Z

Fixes #20111
Fixes #18045

This stores the users path during a PKCE auth flow and returns the user to the saved path on the PKCE callback in order to mirror the behavior that dex auth has. With the exception of the login path, the users path is stored in session storage during pkcelogin and retrieved during pkcecallback, defaulting to '/applications' when not present. This mirrors Dex's behavior with the return_url query parameter.

This fix should be cherry-picked to 2.12

Below is a screenshot of a pkce token expiry triggering a reauth that now redirects the user to their previous path. Certain sections have been redacted to not leak confidential or sensitive information.

Checklist:

bunnyshell · 2024-10-02T16:22:32Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

bunnyshell · 2024-10-02T16:22:32Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

ui/src/app/login/components/utils.ts

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

reggie-k

It is better to move this into the if that handles the error handling, cause needed only in case of error.

ui/src/app/app.tsx

reggie-k · 2024-10-16T09:49:10Z

@austin5219 regarding the error handling upon pkce-relogin and using the React context, please make sure to test the error use case to see the notification in action, I am not confident that using the React context there goes smooth.

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

…xt for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

…to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

reggie-k

Also had a session with Austin where he showed the behavior before and after the change, LGTM.

pasha-codefresh · 2024-10-30T10:37:22Z

Thank you @austin5219 , amazing work

pasha-codefresh · 2024-10-30T10:38:17Z

@austin5219 will it be big issue if we release it in 2.14 in next 3 months? it is available under latest tag, but because this bug fix is also affecting other components, i would consider it to release as part of 2.14 RC

austin5219 · 2024-10-30T15:02:50Z

@pasha-codefresh given it is a bug fix, is there any way it could be at least backported to 2.13?
also can this issue be closed as well as the changes for the fix were in this pr: #18045

pasha-codefresh · 2024-10-31T07:58:26Z

Yes, issue can be closed. Okay, we are doing 2.13 release on Monday, Can we cherry-pick it as part of 2.13.1? For example in 1 week after release

pasha-codefresh · 2024-11-04T18:44:19Z

/cherry-pick release-2.13

gcp-cherry-pick-bot · 2024-11-04T18:44:28Z

Cherry-pick failed with Merge error 092bb7328cc7aeeb75460fc25fafa575418cacd3 into temp-cherry-pick-49da72-release-2.13

… like dex auth flow (argoproj#20202) * Adding non-default basehref support for PKCE auth flow Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding ; for linting Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * removing hook function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Moving unauthorized error handling to class component to access context for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Store the subsrition handle to close in unmount Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * reorder imports Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Actually saving the subscriptions now Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * returning the 401 subscription from helper function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Handle the promise of a subscription Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Removing then from non async subscribe Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Linter fixes Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding path caching to sessionStorage on pkceLogin and redirect step to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --------- Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

…20675) * fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow (#20202) * Adding non-default basehref support for PKCE auth flow Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding ; for linting Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * removing hook function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Moving unauthorized error handling to class component to access context for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Store the subsrition handle to close in unmount Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * reorder imports Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Actually saving the subscriptions now Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * returning the 401 subscription from helper function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Handle the promise of a subscription Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Removing then from non async subscribe Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Linter fixes Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding path caching to sessionStorage on pkceLogin and redirect step to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --------- Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Merge Conflict fix Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --------- Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

… like dex auth flow (argoproj#20202) * Adding non-default basehref support for PKCE auth flow Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding ; for linting Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * removing hook function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Moving unauthorized error handling to class component to access context for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Store the subsrition handle to close in unmount Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * reorder imports Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Actually saving the subscriptions now Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * returning the 401 subscription from helper function Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Handle the promise of a subscription Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Removing then from non async subscribe Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Linter fixes Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> * Adding path caching to sessionStorage on pkceLogin and redirect step to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> --------- Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com> Signed-off-by: Adrian Aneci <aneci@adobe.com>

austin5219 requested a review from a team as a code owner October 2, 2024 16:22

todaywasawesome assigned pasha-codefresh Oct 3, 2024

todaywasawesome modified the milestone: 2.12 Oct 3, 2024

reggie-k reviewed Oct 10, 2024

View reviewed changes

ui/src/app/login/components/utils.ts Show resolved Hide resolved

reggie-k mentioned this pull request Oct 13, 2024

REQUEST: Promotion to Reviewer for reggie-k argoproj/argoproj#329

Closed

7 tasks

austin5219 added 2 commits October 15, 2024 22:52

Adding non-default basehref support for PKCE auth flow

765f05b

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Adding ; for linting

46b66c0

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

austin5219 force-pushed the pkce-return-path branch from 793950e to 81fac2b Compare October 16, 2024 04:53

reggie-k suggested changes Oct 16, 2024

View reviewed changes

ui/src/app/app.tsx Outdated Show resolved Hide resolved

removing hook function

4fcb087

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

austin5219 force-pushed the pkce-return-path branch from 81fac2b to df8f07d Compare October 16, 2024 15:07

austin5219 added 9 commits October 16, 2024 21:24

Moving unauthorized error handling to class component to access conte…

391f577

…xt for error handling within 401 error Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Store the subsrition handle to close in unmount

752ce6e

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

reorder imports

b62ab8f

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Actually saving the subscriptions now

4673121

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

returning the 401 subscription from helper function

905fb28

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Handle the promise of a subscription

4070e08

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Removing then from non async subscribe

ad31a3a

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Linter fixes

d2a6e1f

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

Adding path caching to sessionStorage on pkceLogin and redirect step …

a5091b7

…to cached path if available in pkceCallback to mirror Dex functionality Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

austin5219 force-pushed the pkce-return-path branch from df8f07d to a5091b7 Compare October 17, 2024 14:36

reggie-k approved these changes Oct 30, 2024

View reviewed changes

pasha-codefresh approved these changes Oct 30, 2024

View reviewed changes

pasha-codefresh merged commit 092bb73 into argoproj:master Oct 30, 2024
23 checks passed

This was referenced Oct 30, 2024

fix(pkce): 18045 PKCE respect base href and Reauth to PKCE on token expire #20110

Closed

OIDC: PKCE Incorrectly Uses Base Redirect URL for ArgoCD Hosted Under Custom Path #18045

Closed

austin5219 deleted the pkce-return-path branch October 30, 2024 15:11

austin5219 mentioned this pull request Nov 5, 2024

fix(pkce): 20202 Backport PKCE auth flow fix for basehref and reauth #20675

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow #20202

fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow #20202

austin5219 commented Oct 2, 2024 •

edited

Loading

bunnyshell bot commented Oct 2, 2024 •

edited

Loading

bunnyshell bot commented Oct 2, 2024 •

edited

Loading

reggie-k left a comment

reggie-k commented Oct 16, 2024

reggie-k left a comment

pasha-codefresh commented Oct 30, 2024

pasha-codefresh commented Oct 30, 2024

austin5219 commented Oct 30, 2024 •

edited

Loading

pasha-codefresh commented Oct 31, 2024

pasha-codefresh commented Nov 4, 2024

gcp-cherry-pick-bot bot commented Nov 4, 2024

fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow #20202

fix(pkce): 20111 PKCE auth flow does not return user to previous path like dex auth flow #20202

Conversation

austin5219 commented Oct 2, 2024 • edited Loading

bunnyshell bot commented Oct 2, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

bunnyshell bot commented Oct 2, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

reggie-k left a comment

Choose a reason for hiding this comment

reggie-k commented Oct 16, 2024

reggie-k left a comment

Choose a reason for hiding this comment

pasha-codefresh commented Oct 30, 2024

pasha-codefresh commented Oct 30, 2024

austin5219 commented Oct 30, 2024 • edited Loading

pasha-codefresh commented Oct 31, 2024

pasha-codefresh commented Nov 4, 2024

gcp-cherry-pick-bot bot commented Nov 4, 2024

austin5219 commented Oct 2, 2024 •

edited

Loading

bunnyshell bot commented Oct 2, 2024 •

edited

Loading

bunnyshell bot commented Oct 2, 2024 •

edited

Loading

austin5219 commented Oct 30, 2024 •

edited

Loading