Add new --oidc-use-access-token flag to get-token
#1084
Conversation
Implements int128#1083. See description there for context. In its current form, this PR is bare bones functionality. I have not yet added any tests to confirm this behavior. Additionally, we could consider updtating some of the naming. It is confusing to return a `TokenSet` where `IDToken` actually has an `accessToken`. I'm open to feedback on how best to improve this. However, this PR is functional. I have validated it locally. Without adding `--oidc-use-access-token`, and `id_token` is successfully returned. Adding `--oidc-use-access-token` results in an `access_token` being successfully returned.
| IDToken: accessToken, | ||
| RefreshToken: token.RefreshToken, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Don't you think it should be enough to select the other token here?
kubelogin/pkg/oidc/client/client.go
Line 196 in 94ca6bc
The way how it's implemented now would actually lead to a verification of both tokens if --oidc-use-access-token is set, which is in my opinion not necessary.
There was a problem hiding this comment.
That is a valid alternative approach. I chose not do that initially here because the id_token has a nonce while the access_token does not. So if we only validate the access_token, we will lose the nonce check. It seemed preferred to me to maintain this nonce check but use the access_token.
|
I realize I have some tests failing because I haven't updated the mocks ( EDIT: This actually isn't so easy. The version of |
Needed to plumb through our new parameter `UseAccessToken` to the mocks as well.
| // New provides a mock function with given fields: ctx, p, tlsClientConfig | ||
| func (_m *MockFactoryInterface) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config) (Interface, error) { | ||
| // New provides a mock function with given fields: ctx, p, tlsClientConfig, useAccessToken | ||
| func (_m *MockFactoryInterface) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config, useAccessToken bool) (Interface, error) { |
There was a problem hiding this comment.
Note that I had to manually make these changes, which is not great. I couldn't get mockery v2.13.1 to work on my machine. I believe we have to upgrade to a newer version of mockery to work with this version of Go. I was getting this error:
$ ~/Downloads/mockery_2.13.1_Darwin_x86_64/mockery --all --inpackage --with-expecter
21 May 24 15:11 PDT INF Starting mockery dry-run=false version=v2.13.1
21 May 24 15:11 PDT INF Walking dry-run=false version=v2.13.1
2024/05/21 15:11:45 internal error: package "context" without types was imported from "github.com/int128/kubelogin/integration_test/httpdriver"
Which is mentioned here: https://vektra.github.io/mockery/v2.35/notes/#error-no-go-files-found-in-root-search-path. I decided not to tackle this in this PR. We can discuss doing so in a follow-up PR if that makes sense.
|
@int128 Are you able to take a look at this PR when you have a chance? I'm happy to update if you have suggestions. I wasn't sure if there are any better unit tests to add than the ones I added. And I'm unsure if it is worth updating |
Implements #1083. See description there for context.
In its current form, this PR is bare bones functionality.
I have not yet added any tests to confirm this behavior. Additionally, we could consider updating some of the naming. It is confusing to return aTokenSetwhereIDTokenactually has anaccessToken. I'm open to feedback on how best to improve this. However, changes to this may effect which token is cached and validated. So I'm hesitant to make this change unless we think it is necessary.However, this PR is functional. I have validated it locally. Without adding
--oidc-use-access-token, andid_tokenis successfully returned. Adding--oidc-use-access-tokenresults in anaccess_tokenbeing successfully returned.