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

Support for offline tokens #2468

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Support for offline tokens #2468

merged 1 commit into from
Mar 4, 2024

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Feb 29, 2024

Summary

This adds new sub-commands to minder auth which enable the usage and installation
of offline tokens. These tokens enable you to create long-lived credentials which are handy
for CI and automation cases.

Co-Authored-By: Eleftheria Stein-Kousathana eleftheria@stacklok.com

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@JAORMX JAORMX requested a review from a team as a code owner February 29, 2024 09:13
@coveralls
Copy link

coveralls commented Feb 29, 2024

Coverage Status

coverage: 39.351% (-0.2%) from 39.542%
when pulling 80dcfea on offline-tokens
into 5fd0f74 on main.

Copy link
Member

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

Added a few comments mostly around keeping the way we read/set flags consistent with the rest of the CLI commands.

cmd/cli/app/auth/offline_use.go Outdated Show resolved Hide resolved
cmd/cli/app/auth/offline_use.go Outdated Show resolved Hide resolved
cmd/cli/app/auth/offline_get.go Outdated Show resolved Hide resolved
cmd/cli/app/auth/offline_get.go Show resolved Hide resolved
cmd/cli/app/auth/offline_use.go Outdated Show resolved Hide resolved
cmd/cli/app/auth/offline_get.go Outdated Show resolved Hide resolved
@JAORMX JAORMX force-pushed the offline-tokens branch 4 times, most recently from 1bcd652 to 5242596 Compare February 29, 2024 12:19
@JAORMX JAORMX force-pushed the offline-tokens branch 3 times, most recently from 5fc7218 to a93844b Compare February 29, 2024 12:53
Copy link
Member

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

IMO if we're going to expose offline tokens to external users we need to show them how to revoke the tokens.
In theory it's possible now through the keycloak account management, but we don't direct users there.

Long: `The minder auth offline-token get command project lets you retrieve an offline token
for the minder control plane.

Offline tokens are used to authenticate to the minder control plane without
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this should say "to the minder CLI", since the tokens are scoped to the CLI client. In theory tokens generated by different clients can have different characteristics, although in our current implementation they don't.

@JAORMX
Copy link
Contributor Author

JAORMX commented Feb 29, 2024

IMO if we're going to expose offline tokens to external users we need to show them how to revoke the tokens. In theory it's possible now through the keycloak account management, but we don't direct users there.

I agree. And Keycloak has a token revocation endpoint we could use, want me to include that as part of this PR? Was thinking of adding a minder auth offline-token revoke command.

@eleftherias
Copy link
Member

IMO if we're going to expose offline tokens to external users we need to show them how to revoke the tokens. In theory it's possible now through the keycloak account management, but we don't direct users there.

I agree. And Keycloak has a token revocation endpoint we could use, want me to include that as part of this PR? Was thinking of adding a minder auth offline-token revoke command.

Yeah I think it should be part of this PR so we don't accidentally release a CLI version without the revocation option

@JAORMX
Copy link
Contributor Author

JAORMX commented Feb 29, 2024

@eleftherias sounds good! I'll work on that tomorrow.

cmd/cli/app/auth/offline_use.go Outdated Show resolved Hide resolved
cmd/cli/app/auth/offline_use.go Outdated Show resolved Hide resolved
@JAORMX JAORMX force-pushed the offline-tokens branch 3 times, most recently from 03890d3 to bf6714c Compare March 1, 2024 08:53
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
@JAORMX
Copy link
Contributor Author

JAORMX commented Mar 4, 2024

Just rebased

@JAORMX JAORMX merged commit b31ee90 into main Mar 4, 2024
20 checks passed
@JAORMX JAORMX deleted the offline-tokens branch March 4, 2024 17:52
Vyom-Yadav pushed a commit to Vyom-Yadav/minder that referenced this pull request Mar 5, 2024
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
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.

None yet

4 participants