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

Pass user token to API via an HTTP header #1112

Merged
merged 2 commits into from Nov 22, 2021
Merged

Pass user token to API via an HTTP header #1112

merged 2 commits into from Nov 22, 2021

Conversation

yiannistri
Copy link
Contributor

Closes: weaveworks/weave-gitops-enterprise#203

What changed?

  • Refactored out code that returns the git provider token (either via an env var or via the git provider auth flow)
  • Pass the git provider token to the API via an HTTP header

Why?
This is so that the Weave GitOps Enterprise API can make calls to the git provider on behalf of the end user.

How did you test it?
Tested locally + unit tests

Release notes
N/A

Documentation Changes
N/A

@yiannistri yiannistri added the type/enhancement New feature or request label Nov 19, 2021
@yiannistri yiannistri marked this pull request as ready for review November 19, 2021 18:50
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Looks good. The only thing holding me up is the unsetting of an env variable.

@@ -141,3 +143,77 @@ func TestEndpointNotSet(t *testing.T) {
err := cmd.Execute()
assert.EqualError(t, err, "the Weave GitOps Enterprise HTTP API endpoint flag (--endpoint) has not been set")
}

func TestGitProviderToken(t *testing.T) {
os.Setenv("GITHUB_TOKEN", "test-token")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to "reset" this back to the old value? Might cause some weird bugs in a user's environment otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've tested this with my own GITHUB_TOKEN and it seemed unaffected after running these tests. Also in the next line I'm resetting that env var.

Copy link
Contributor

Choose a reason for hiding this comment

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

So long as os.Unsetenv() reset it to the previous value, or at least doesn't change the user's environment 👍

@@ -24,6 +26,9 @@ func TestEndpointNotSet(t *testing.T) {
}

func TestPayload(t *testing.T) {
os.Setenv("GITHUB_TOKEN", "test-token")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above with saving the previous value.

cmd/internal/provider.go Show resolved Hide resolved
pkg/adapters/http.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

LGTM with the caveat that the user's environment doesn't get changed.

@@ -141,3 +143,77 @@ func TestEndpointNotSet(t *testing.T) {
err := cmd.Execute()
assert.EqualError(t, err, "the Weave GitOps Enterprise HTTP API endpoint flag (--endpoint) has not been set")
}

func TestGitProviderToken(t *testing.T) {
os.Setenv("GITHUB_TOKEN", "test-token")
Copy link
Contributor

Choose a reason for hiding this comment

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

So long as os.Unsetenv() reset it to the previous value, or at least doesn't change the user's environment 👍

@yiannistri
Copy link
Contributor Author

Thanks @jpellizzari for the review. While I haven't had any issues locally, I'll change this to make it more robust. Bonus test helper for when we switch to Go 1.17 🎉

@yiannistri yiannistri merged commit 350e78d into main Nov 22, 2021
@yiannistri yiannistri deleted the pass-user-token branch November 22, 2021 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI] Implement github oauth in PR creation in EE
2 participants