Oidc #335
Oidc #335
Conversation
|
Welcome @louis-murray! |
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
I have signed the cla |
|
related to #294 |
k8s-ci-robot
added
size/XL
|
ok, I had to set the openid-client package to version 2.5.0, same version used by the go daddy kube client, to support node 8. |
|
/assign @brendandburns |
|
/check-cla |
1 similar comment
|
/check-cla |
k8s-ci-robot
added
cncf-cla: yes
src/oidc_auth.ts
Outdated
| return this.refresh(user, client); | ||
| } | ||
|
|
||
| private async refresh(user: User, client): Promise<string> { |
There was a problem hiding this comment.
shouldn't client have a type?
There was a problem hiding this comment.
it definitely should. the public types for the oidc client are missing a reference to the only function was actually call on it. I just opened a PR to fix that DefinitelyTyped/DefinitelyTyped#37968
|
Thanks for the PR! Minor comments. Please also squash everything into one single commit. Thanks! |
|
thanks for the feedback! I think I touched on everything you brought up. the only thing I don't have an immediate resolution for is the client types. |
src/oidc_auth.ts
Outdated
|
|
||
| export class OpenIDConnectAuth implements Authenticator { | ||
| // public for testing purposes. | ||
| public currentTokenExpiration = 0; |
There was a problem hiding this comment.
nit: instead of making this public you can use:
(oidcAuth as any).currentTokenExpiration = <foo>;in the test code. I think that's preferable vs exposing this as public.
There was a problem hiding this comment.
that's slick, I'll make that change
|
Thanks, way cleaner! one minor comment about how you can expose private variables to tests w/o changing their visibility in the class. Thanks again! |
openid token management impl oidc token management implement oidc token refreshing add openid-client add oidc refresh logic add tests for oidc logic introspect needs special urls so removing updated tests to reflect introspect not being able to be used prettier changes chore: use a version of open-id client that supports node 8 fix id-token ref to id_token update token expiration to be more flexible replace the tokenExp with tokenExpirationCache update if for first time interaction with a user replace token expiration map with single expiration update test descriptions to be more accurate correct test to set initial expiration add oidc types reference types in class cast any so we can keep properties private prettier use expires_at over expiration
|
just updated based on your comments, I also added the updated types for oidc and replaced the call when setting the |
|
/lgtm Thanks for the patience! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, louis-murray The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
No description provided.