-
Notifications
You must be signed in to change notification settings - Fork 805
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
Feature cont.: authorize CLI as admin with private #4338
Feature cont.: authorize CLI as admin with private #4338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
tools/cli/app.go
Outdated
@@ -69,6 +69,11 @@ func NewCliApp() *cli.App { | |||
Usage: "optional JWT for authorization", | |||
EnvVar: "CADENCE_CLI_JWT", | |||
}, | |||
cli.StringFlag{ | |||
Name: FlagJWTPrivateKey, | |||
Usage: "optional private key path to create JWT", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: how about adding a comment about only one of the two options is needed to use jwt authorization?
KeyTypePublic KeyType = "public key" | ||
) | ||
|
||
func loadRSAKey(path string, keyType KeyType) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍👍
tools/cli/app.go
Outdated
EnvVar: "CADENCE_CLI_JWT", | ||
}, | ||
cli.StringFlag{ | ||
Name: FlagJWTPrivateKeyWithAlias, | ||
Usage: "optional private key path to create JWT. Either this or --jwt is needed for jwt authentication. --jwt flag has priority over this one if both provided", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never been quite good with text description @longquanzheng is this decent enough? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh can you say authorization instead of authentication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature doesn’t verify the user identity. The provided jwt token is to verify the access. I think by concept the authentication part (using username password or one login) is done in the OAuth integration by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my classic typo. Updated :)
Pull Request Test Coverage Report for Build d63d614f-7e0a-4f62-8c57-17b687794216
💛 - Coveralls |
What changed?
--jwt-private-key
flag added to CLIWhy?
Requested in this proposal
How did you test it?
Tested locally
and
Potential risks
Nothing
Release notes
When the whole proposal is implemented
Documentation Changes
When the whole proposal is implemented