Skip to content

Registry tool: add auth handling #681

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Aug 11, 2022
Merged

Registry tool: add auth handling #681

merged 11 commits into from
Aug 11, 2022

Conversation

@theganyo
Copy link
Member

@theganyo theganyo commented Aug 10, 2022 •
edited
Loading

Set and view registry CLI authentication.

Added auth command:

$ registry auth
Manage registry CLI authentication.

Usage:
  registry auth [command]

Available Commands:
  print-token Invoke token source and print token.

To set or get token-source, use commands:

$ registry config set token-source
$ registry config get token-source

Fixes #675

@timburks
Copy link
Contributor

timburks commented Aug 11, 2022 •
edited
Loading

Is there a strong reason to prefer "generator" over other command names? My initial preference on seeing it was to suggest "provider", but when I looked for other precedents I found "source" in the Go oauth2 package, which I think has a stronger case to be made for it.

"generator" feels awkward to me because we have so many situations where code is generated. Tokens are more short-lived.

...

Could setting this be handed with registry config set token-source?

timburks
timburks reviewed Aug 11, 2022
View reviewed changes
timburks
timburks reviewed Aug 11, 2022
View reviewed changes
pkg/config/configuration.go
}

// Resolve derived values (eg. Registry.Token from Registry.TokenCmd)
func (c *Configuration) Resolve() error {
Copy link
Contributor

@timburks timburks Aug 11, 2022

Choose a reason for hiding this comment

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

How frequently is this run?

  1. before every token usage?
  2. once for each run of the registry tool?
  3. when tokens expire?

Copy link
Member Author

@theganyo theganyo Aug 11, 2022

Choose a reason for hiding this comment

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

Currently, this would only happen on load - ie. #2.

timburks
timburks reviewed Aug 11, 2022
View reviewed changes
@codecov
Copy link

codecov bot commented Aug 11, 2022 •
edited
Loading

Codecov Report

Merging #681 (4437a98) into main (8b58a46) will decrease coverage by 0.06%.
The diff coverage is 63.38%.

@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
- Coverage   55.56%   55.49%   -0.07%     
==========================================
  Files          89       93       +4     
  Lines        7712     7837     +125     
==========================================
+ Hits         4285     4349      +64     
- Misses       2972     3021      +49     
- Partials      455      467      +12
Impacted Files Coverage Δ
cmd/registry/cmd/config/config.go 85.71% <ø> (+4.23%) ⬆️
cmd/registry/cmd/config/unset.go 71.42% <25.00%> (+1.42%) ⬆️
cmd/registry/cmd/config/configurations/create.go 58.62% <50.00%> (ø)
cmd/registry/cmd/config/configurations/delete.go 88.57% <50.00%> (ø)
cmd/registry/cmd/config/set.go 80.64% <50.00%> (-1.71%) ⬇️
cmd/registry/cmd/auth/print_token.go 55.55% <55.55%> (ø)
pkg/config/config.go 56.77% <56.77%> (ø)
pkg/config/configuration.go 62.85% <62.85%> (ø)
cmd/registry/cmd/config/configurations/activate.go 60.86% <66.66%> (ø)
cmd/registry/cmd/auth/auth.go 81.25% <81.25%> (ø)
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@theganyo
Copy link
Member Author

theganyo commented Aug 11, 2022

Is there a strong reason to prefer "generator" over other command names? My initial preference on seeing it was to suggest "provider", but when I looked for other precedents I found "source" in the Go oauth2 package, which I think has a stronger case to be made for it.

Nope. I think "token-source" would be fine.

Could setting this be handed with registry config set token-source?

I was trying to keep with your idea of putting this auth stuff on "auth" instead of config. Is that not desirable?

@timburks
Copy link
Contributor

timburks commented Aug 11, 2022

I was trying to keep with your idea of putting this auth stuff on "auth" instead of config. Is that not desirable?

I guess I'm undecided about this - on one hand, it's good to have auth token-source because it's in the auth hierarchy. But I got the impression that the token source was just one more piece of configuration that would be stored alongside all the other values that are set with config set.

@theganyo
Copy link
Member Author

theganyo commented Aug 11, 2022

I guess I'm undecided about this - on one hand, it's good to have auth token-source because it's in the auth hierarchy. But I got the impression that the token source was just one more piece of configuration that would be stored alongside all the other values that are set with config set.

Yeah, I don't have a strong opinion either way. We could do auth config set token-source xxxx... it's a bit more verbose, fwiw, but I'm really uncertain which is more intuitive.

@theganyo theganyo merged commit ba99327 into apigee:main Aug 11, 2022
@theganyo theganyo deleted the theganyo/issue675 branch August 11, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@timburks timburks timburks approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Apigee API Registry
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Registry tool: add auth handling

2 participants

@theganyo @timburks