Skip to content

Conversation

@alexmt
Copy link
Collaborator

@alexmt alexmt commented Feb 26, 2021

Signed-off-by: Alexander Matyushentsev [email protected]

PR is a follow-up for #5477 that introduced built-in token expiration. To make sure user experience is good API server will regenerate token for requests with valid token in case if token is expiring soon (5 minutes)

@alexmt alexmt requested review from jannfis and jessesuen February 26, 2021 21:08
@alexmt alexmt force-pushed the regenerate-account-login-tokens branch 2 times, most recently from a8011fe to b182fd4 Compare February 26, 2021 21:20
server/server.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how this works? Is this is a new cookie key and why it needed in addition to the argocd-token?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to somehow return regenerated token from the grpc service back to grpc gateway so it can be translated into the cookie.

GRPC provides outgoing server metadata for such purposes but GRPC gateway generates code that allows accessing only grpc-gateway.ServerMetadata that contains "headers" and "trailers" metadata.

So "Authenticate" method adds regenerated token into outgoing ServerMetadata.HeaderMD field. Then grpc-gateway response forwarder ( translateGrpcCookieHeader ) transforms this header into a proper cookie header

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. So it's not a key that a client is ever exposed to and is only used for internal purposes due to the way grpc-gateway works.

Do you mind adding a comment on how this works somewhere in the code for future reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. adding comment about it

@alexmt alexmt force-pushed the regenerate-account-login-tokens branch from b182fd4 to 816510f Compare March 3, 2021 02:03
@codecov-io
Copy link

Codecov Report

Merging #5629 (816510f) into master (cdabf31) will increase coverage by 0.01%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5629      +/-   ##
==========================================
+ Coverage   40.96%   40.97%   +0.01%     
==========================================
  Files         144      144              
  Lines       19325    19344      +19     
==========================================
+ Hits         7917     7927      +10     
- Misses      10303    10310       +7     
- Partials     1105     1107       +2     
Impacted Files Coverage Δ
server/server.go 55.11% <53.57%> (-0.75%) ⬇️
util/session/sessionmanager.go 70.63% <56.00%> (+0.90%) ⬆️
server/logout/logout.go 75.51% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdabf31...816510f. Read the comment docs.

@alexmt alexmt merged commit 0ccd573 into argoproj:master Mar 3, 2021
@alexmt alexmt deleted the regenerate-account-login-tokens branch March 3, 2021 02:24
shubhamagarwal19 pushed a commit to shubhamagarwal19/argo-cd that referenced this pull request Apr 15, 2021
…5629)

* feat: regenerate active users token if it is expiring soon

Signed-off-by: Alexander Matyushentsev <[email protected]>

* Comment how 'renew-token' header is used

Signed-off-by: Alexander Matyushentsev <[email protected]>
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.

3 participants