fix(server): JWT token checks (#14930)

[GeorgiYosifov]

Fixes: #14930

OIDC, Dex and Admin token revocation is done correctly.
API and CLI entry points are corrected.
Unit tests are added for each implementation change.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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.
• Optional. My organization is added to USERS.md.
• 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.

Please see Contribution FAQs if you have questions about your pull-request.

[crenshaw-dev]

@GeorgiYosifov would you mind adding unit tests?

[GeorgiYosifov]

Ok, I will add unit tests but I encounter that there is also problem with token revocation when I use "argocd cli". I will continue my work on the bug.
By the way fix of DCO check hope that not lead to problem in future to the PR.

[GeorgiYosifov]

@crenshaw-dev Can you review the PR at this state, before I add unit tests and test also with dex configuration manually?

[codecov]

Codecov Report

❌ Patch coverage is 47.88732% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.29%. Comparing base (83548e3) to head (694219f).

Files with missing lines	Patch %	Lines
cmd/argocd/commands/logout.go	0.00%	33 Missing ⚠️
util/session/sessionmanager.go	77.77%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15004      +/-   ##
==========================================
+ Coverage   62.26%   62.29%   +0.02%     
==========================================
  Files         351      351              
  Lines       49368    49430      +62     
==========================================
+ Hits        30741    30790      +49     
- Misses      15682    15708      +26     
+ Partials     2945     2932      -13     

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

[GeorgiYosifov]

Unit tests are added, still remains to test it with Dex configured.

[lsangelov]

@agaudreault and @crenshaw-dev can this changes be merged as it becomes harder and harder to keep changes up to date just to wait this PR to be merged? Is there something missing in order the PR to be merged?

[anandf]

@GeorgiYosifov, Thanks for working on this issue. I have verified the changes locally by integrating it with the master branch and it works well.
I see that the branch was NOT rebased but instead changes from master branch were merged to your feature branch. This is causing difficulty to rebase with the latest master. I faced lot of conflicts trying to rebase it locally. Would it be ok to resubmit after rebasing with the latest master. After failing to rebase it safely, I took the changes from each file and applied it in my branch. The changes are available here master...anandf:argo-cd:invalidate_jwt_token_on_logout

[anandf]

This http call needs to be made after reconfirming from the user, needs to be moved after checking thecanLogout.

[anandf]

This method will cause a fatal error and cause the program to terminate abruptly. If the remote server is unavailable or errors out due to network issue, it is probably its better to give a warning and logout locally and clean up the local storage.

[anandf]

better to check if the conversion to string was successful using an ok boolean (instead of ignoring). If the key is not found or if the conversion fails, id can be set to empty string probably with a warning log.

[anandf]

@GeorgiYosifov We are looking at speeding up the review and merge process. Please let us know if you can rebase the changes with the latest master branch. Let me know if you need any help. Thank you.

[anandf]

Tested the changes and it works well. Requires rebase with latest master and then it is good to merge.

[GeorgiYosifov]

Hi @anandf, Unfortunately I am out of the topic and I would not be able to cover your concerns in the above comments. You are good to go with any modifications on top of this branch in seek of success the PR to be merged someday.

[anandf]

@GeorgiYosifov my review comments are minor and can be done later as a separate PR on top of your changes too. Can you rebase the code with the latest master and we can priortize for the merge. Other minor comments can be addressed later.

[GeorgiYosifov]

I am done @anandf

[anandf]

LGTM. @GeorgiYosifov Can you add the file logout_test.go whatever is there in the latest master branch ? Looks like the file deletion was unintentional.

[GeorgiYosifov]

The old test from: "cmd/argocd/commands/logout_test.go" is removed because the new implementation here: https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/cmd/argocd/commands/logout.go is tested with: https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/server/logout/logout_test.go and https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/util/localconfig/localconfig_test.go

Forgot to mention this comment, feel free to commit, merge.

[anandf]

@agaudreault I see that you have requested a few changes. Could you take a re-look at this PR and help with the merge process. Thank you.