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

76 google oauth config mutator #77

Merged

Conversation

chrishalbert
Copy link
Contributor

This PR builds on the original work from #53 and addresses/closes #76

  • Provides a means to set client id and client secret without a file
  • Adds the ability to redirect to login upon failed auth
  • Adds tests

google/google.go Outdated
@@ -74,6 +77,19 @@ func Setup(redirectURL, credFile string, scopes []string, secret []byte) {
}
}

// Setup the authorization path without a config file
Copy link
Member

Choose a reason for hiding this comment

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

Go doc requires the first word to be the same as the func name.

@szuecs
Copy link
Member

szuecs commented Jan 16, 2024

👍

@szuecs
Copy link
Member

szuecs commented Jan 17, 2024

👍

google/google.go Outdated
@@ -100,29 +116,33 @@ func GetLoginURL(state string) string {
return conf.AuthCodeURL(state)
}

func WithLoginURL(url string) {
loginURL = url
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loginURL = url
loginURL = strings.TrimSpace(s)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to use url.Parse(s), check error and if no error use u.String() to have a clean url string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the error handling, would you suggest the signature change to func WithLoginURL(url string) (bool, error) ?

Copy link
Member

Choose a reason for hiding this comment

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

I would say return error is enough. Either there is an error or not :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest we are aiming for consistency on API. It is either

func SetupFromString(redirectURL, clientID string, clientSecret string, scopes []string, secret []byte) error
func WithLoginURL(url string) error

or

func SetupFromString(redirectURL, clientID string, clientSecret string, scopes []string, secret []byte)
func WithLoginURL(url string)

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to go with the first, so return error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - that seems more idiomatic too. I'll have that shortly..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay - had some distractions. I was just getting ready to update the PR and noticed that SetupFromString() and Setup() would differ with regards to their return value. The Setup() function could more plausibly have errors, however, errors aren't being returned at this time. I do like the idea of returning an error WithLoginURL() since there is possibility for a bad URL being added, but I feel like if I return an error from SetupFromString(), I should also return an error for Setup(). While that wouldn't break functionality, it could cause linting errors since return values aren't being accepted and handled. I'm going to make the changes for WithLoginURL() as prescribed, but I'm interested to get your opinions on handling SetupFromString() and Setup().

Signed-off-by: Chris Halbert <[email protected]>
Signed-off-by: Chris Halbert <[email protected]>
@fogfish
Copy link
Member

fogfish commented Jan 20, 2024

👍

1 similar comment
@szuecs
Copy link
Member

szuecs commented Jan 20, 2024

👍

var testCases = []struct {
description string
urlParm string
expectUrlLogin string
Copy link
Member

Choose a reason for hiding this comment

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

Linter says we should use upper case URL.
I know we don't follow it elsewhere but let's follow the convention for new things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, will do!

@fogfish
Copy link
Member

fogfish commented Jan 22, 2024

👍

@szuecs
Copy link
Member

szuecs commented Jan 23, 2024

👍

1 similar comment
@gargravarr
Copy link
Contributor

👍

@gargravarr gargravarr merged commit 4b1fdca into zalando:master Jan 23, 2024
6 checks passed
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.

Provide mutator methods for google.go's oauth2 config.
5 participants