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

feat: Adds support for authenticating with access token #270

Merged
merged 5 commits into from
Sep 8, 2023

Conversation

geofflamrock
Copy link
Contributor

@geofflamrock geofflamrock commented Sep 7, 2023

This PR adds support for authenticating using an access token as a bearer token in the Authorization header. Access tokens are obtained using OpenID Connect and by nature are short-lived. As such, storing access tokens in the cli configuration (octopus config set) is not supported, and are only read from the OCTOPUS_ACCESS_TOKEN environment variable. This environment variable will be set automatically when using the OctopusDeploy/login action which is currently only released internally.

Note: This PR requires OctopusDeploy/go-octopusdeploy#201 to be completed and a new version of the go-octopusdeploy client to be released before it will build. Update: This PR is merged now.

Note: The OpenID Connect feature is currently under development and may not be available on all Octopus instances at the moment, so this PR doesn't make any changes to the readme for the CLI. Once the feature is released then a follow up PR will add documentation around this.

@geofflamrock geofflamrock requested a review from a team September 7, 2023 06:45
Copy link

@johnsimons johnsimons left a comment

Choose a reason for hiding this comment

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

A small comment regarding the init function, but overall seems reasonable to me.
That said, this is the first time I am looking at this code base, so it may be a good idea to ping others who are more familiar with this code base.

pkg/apiclient/client_factory.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benPearce1 benPearce1 left a comment

Choose a reason for hiding this comment

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

I would like to see the ApiCredentials struct dropped in favour of an interface with an Api Key implementation and Access Token implementation.
This is actually a change in the client that would impact this PR

Copy link
Contributor

@benPearce1 benPearce1 left a comment

Choose a reason for hiding this comment

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

👍

@geofflamrock geofflamrock merged commit 75a8ff7 into main Sep 8, 2023
4 checks passed
@geofflamrock geofflamrock deleted the geoffl/access-token-auth branch September 8, 2023 02:45
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

3 participants