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

Implementation of JWTAuthorizationProvider #1116

Merged

Conversation

iamrodrigo
Copy link
Contributor

@iamrodrigo iamrodrigo commented Aug 2, 2021

What changed?
Implementation of JWTAuthorizationProvider in both worker and client

Why?
Part of this proposal

How did you test it?
Running the server with oauth file

./cadence-server --env development_oauth start

and modifying cadence-samples

Adding Authorization value in NewClient here

Adding Authorization value in NewDomainClient here

Adding Authorization value in worker.Options here

Potential risks
None

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2021

CLA assistant check
All committers have signed the CLA.

internal/internal_worker.go Outdated Show resolved Hide resolved
worker/worker.go Outdated
@@ -173,6 +173,12 @@ type (
Run() error
}

AuthorizationProvider interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is giving me a cyclic error @longquanzheng.

If I would like to place this on "go.uber.org/cadence/.gen/go/shared" but I'm wondering if everything inside of .gen is generated and if that's the case how could I generate that struct there? Or where do you recommend me to place it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah don’t put it under .gen. There should be other places to put it . Would it work if you put it under auth/ ? And then use type aliases to export to public

Copy link
Collaborator

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

Over all looks great 👍

@@ -29,6 +29,7 @@ import (
"encoding/hex"
"errors"
"fmt"
"go.uber.org/cadence/internal/common/auth"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be moved to the below section/group

@@ -225,6 +226,10 @@ type (
// Optional: Flags to turn on/off some server side options
// default: all the features in the struct are turned off
FeatureFlags FeatureFlags

// Optional: Authorization interface to get the Auth Token
// default: empty token
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default should be no provider

Copy link
Collaborator

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

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

LGTM, it would be better to have some unit tests.

@iamrodrigo iamrodrigo changed the title [WIP] service wrapper created for auth Implementation of JWTAuthorizationProvider Aug 6, 2021
@iamrodrigo
Copy link
Contributor Author

iamrodrigo commented Aug 6, 2021

@longquanzheng test added :), check internal/common/auth/service_wrapper_test.go and internal/jwt_authorization_test.go

@coveralls
Copy link

Pull Request Test Coverage Report for Build ebefd216-b2af-484f-8ba4-55a8796259d8

  • 346 of 391 (88.49%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 71.084%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/internal_worker.go 1 3 33.33%
worker/worker.go 0 3 0.0%
internal/client.go 6 10 60.0%
internal/jwt_authorization.go 21 25 84.0%
internal/common/util/rsa.go 0 32 0.0%
Totals Coverage Status
Change from base Build b792b9b0-c0a6-4a59-9363-e60d3948b178: 0.4%
Covered Lines: 12068
Relevant Lines: 16977

💛 - Coveralls

@longquanzheng longquanzheng merged commit 7c70757 into uber-go:master Aug 6, 2021
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.

4 participants