[auth] Add multi-session support for auth pkg #240
Conversation
pkg/auth/auth.go
Outdated
| } | ||
| } | ||
|
|
||
| return tokens, errors.Join(refreshErrors...) |
There was a problem hiding this comment.
In the current design, we return tokens as well as a joined-error. How is the caller expected to handle these tokens if the error is non-nil?
Is it something like:
tokens, err := c.GetSessions()
if err != nil {
fmt.Println("WARNING: some tokens failed to refresh")
// optionally, internally log `err` to sentry
}
for _, tok := range tokens {
if tok.IsValid() {
// do something useful with tok
}
}
?
An alternate design would be:
callers can read both tokens[i] amd errors[i] and see the reason for why that token is possibly invalid. But i'm not sure how this API is intended to be used, so having trouble discerning which design is better.
There was a problem hiding this comment.
The idea behind this API is to allow the caller to still display useful into even if error. For example, you have 3 tokens, 2 work and one failed to refresh. The caller wants to show what orgs are available. In that case, it can display:
- org 1
- org 2
- org 3 (requires re-login)
instead of just erroring out and requiring full login no matter what.
returning a slice of tokens and a slice or errors would be very un-go'ish. Another alternative is to return something like:
type TokenSource interface {
Token() (*Token, error)
}(this interface is actually part of oauth2 package)
| type Store struct { | ||
| rootDir string | ||
| } | ||
|
|
||
| type storeData struct { | ||
| Version string `json:"version"` | ||
| Tokens []*session.Token `json:"tokens"` |
There was a problem hiding this comment.
Would be nice to comment here that the order matters: tokens in front are given priority, and a new token is pre-pended to the existing list
pkg/auth/auth.go
Outdated
| // expired tokens, and returns an error if any tokens are unable to be | ||
| // refreshed. If there are errors, it still returns all tokens, some of which | ||
| // may be expired. | ||
| func (c *Client) GetSessions(ctx context.Context) ([]*session.Token, error) { |
There was a problem hiding this comment.
not currently used, but the plan is for envsec to use it to show what orgs you can use to initialize a project.
| tokens := []*session.Token{tok} | ||
| for _, t := range sd.Tokens { | ||
| if t.IDClaims().Subject != tok.IDClaims().Subject { | ||
| tokens = append(tokens, t) | ||
| } | ||
| } | ||
| sd.Tokens = tokens |
There was a problem hiding this comment.
The order seems to matter and have product implications in terms of what is returned from ReadToken. Given that:
Can we distinguish login-writes from refresh-writes?
I can understand wanting to move login-write-tokens to the front for priority. However, its unclear to me that refresh-write-tokens should move to the front, or whether they should be kept in their current position
There was a problem hiding this comment.
will do, this is a bug in my implementation. Refreshing a token should not change the order, only logins should.
Summary
Adds multi-session support to auth.
GetSessionsfunction.GetSessionsreturns refreshable tokens, instead of tokens. This allows the caller to inspect each token to find which one they want without having to refresh all. It also allows them to do useful stuff if only some tokens refresh successfully.This will support allowing envsec to:
How was it tested?
envsec auth login