Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RawTokenKeyProvider #2420

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Conversation

wenerme
Copy link
Contributor

@wenerme wenerme commented Jan 26, 2022

What changed?

close #2382

Why?

How did you test it?

Potential risks

Is hotfix candidate?

No

@wenerme wenerme requested a review from a team January 26, 2022 08:32
@yiminc yiminc requested a review from sergeybykov January 26, 2022 17:08
@@ -39,4 +41,11 @@ type TokenKeyProvider interface {
Close()
}

// RawTokenKeyProvider is a TokenKeyProvider that provides keys for validating JWT tokens
Copy link
Member

Choose a reason for hiding this comment

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

Need to add comments that this is an optional alternative version of token key provider. However, if provider implements this interface, it will take precedent and shadow the implementation of TokenKeyProvider and be used to validate JWT token.

Copy link
Member

Choose a reason for hiding this comment

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

If RawTokenKeyProvider is a TokenKeyProvider, shall we embed TokenKeyProvider inside RawTokenKeyProvider interface and only specify additional methods?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as it is and there's no need to add TokenKeyProvider methods to RawTokenKeyProvider.

@yiminc
Copy link
Member

yiminc commented Jan 27, 2022

buildkite fail because you need to run make goimports in your repo's root directory.

Add an empty line to make goimports happy.
@yiminc yiminc merged commit 4600713 into temporalio:master Feb 1, 2022
@sergeybykov
Copy link
Member

Thank you, @wenerme, for your contribution!

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.

pass Token to authorization.TokenKeyProvider when get keys
4 participants