fix(ui): add state parameter in the pkce flow

[js3692]

Fixes #17217

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.54%. Comparing base (2077688) to head (8c73d2a).

❗ Current head 8c73d2a differs from pull request most recent head 6a34507

Please upload reports for the commit 6a34507 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17235      +/-   ##
==========================================
+ Coverage   44.99%   49.54%   +4.55%     
==========================================
  Files         355      273      -82     
  Lines       47931    48288     +357     
==========================================
+ Hits        21565    23925    +2360     
+ Misses      23563    21999    -1564     
+ Partials     2803     2364     -439     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[blakepettersson]

Hi @js3692!

Could you add unit tests for this? And can you add docs / comments in regards to this (similar to what you did in #17217)?

[js3692]

Hello @blakepettersson! Thanks for the pointers - I was largely going off of the original PR #15889 in terms of tests / docs / comments. I'll see what I can add

[csantanapr]

Hi @js3692 would be very useful to get this PR merge, if you could you take a look again to the request from @blakepettersson

[js3692]

Argh yes, sry, I'll take another look.

I didn't see any existing tests around this functionality last time. Let me see what makes sense here

[js3692]

Hi @js3692 would be very useful to get this PR merge, if you could you take a look again to the request from @blakepettersson

Hmm, I might be missing something here, but what exactly are you looking for? 😅

I can add some comments in the code, but I can't find any docs or unit tests for the UI. I'd appreciate some pointers.

[blakepettersson]

Hi @js3692!

Sorry about that - I missed that this was just a UI change (there definitely should be UI tests IMO, but that's another matter and not pertaining to anything you need to do). LGTM 👍

[csantanapr]

Thank you @blakepettersson for the reply and clarification

What's the next step for the PR to progress?

[amcinnes]

Is there anything blocking this PR? I'd love to see it merged so I can have the PKCE flow working with Okta - because then I can hopefully have Argo's web UI and CLI both working using one Okta app.

[sidewinder12s]

@blakepettersson Would you be able to help here?

[Marvin9]

Apologies for late response. I have tested it and it works as expected.

You would need to remove the state from session storage after verification. You can add here

[Marvin9]

Will raise the followup PR. The changes I requested should not block this long pending PR

[js3692]

Thanks all - almost forgot about this PR 😅. Updated with the unset + made it more consistent with the other state handler.

[Marvin9]

Thank you! LGTM. Needs an approval from someone with access. @blakepettersson can you please approve or tag relevant user who can approve?

[blakepettersson]

I don't have the prerequisite powers, but @gdsoumya does 😄