-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add a method to authenticate without a config file #53
base: master
Are you sure you want to change the base?
Conversation
@calston can you fix the DCO (git commit -s -amend), we need to be compliant |
@@ -69,6 +72,20 @@ func Setup(redirectURL, credFile string, scopes []string, secret []byte) { | |||
} | |||
} | |||
|
|||
// Setup the authorization path without a config file | |||
func SetupFromString(redirectURL, cLoginURL string, clientID string, clientSecret string, scopes []string, secret []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function should be Setup
or maybe create an Options{}
struct to pass it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting @calston modify the existing Setup
? Adding functionality with a new SetupFromString
would only be a minor version update, opposed to a major version update. I'm in favor of their current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe SetupWithOptions(o Options)
@@ -7,7 +7,7 @@ import ( | |||
"crypto/rand" | |||
"encoding/base64" | |||
"encoding/json" | |||
"fmt" | |||
//"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete line please
@@ -102,7 +119,8 @@ func Auth() gin.HandlerFunc { | |||
session := sessions.Default(ctx) | |||
retrievedState := session.Get("state") | |||
if retrievedState != ctx.Query("state") { | |||
ctx.AbortWithError(http.StatusUnauthorized, fmt.Errorf("Invalid session state: %s", retrievedState)) | |||
ctx.Redirect(302, loginURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirect makes sense, please just drop commented lines :)
@@ -69,6 +72,20 @@ func Setup(redirectURL, credFile string, scopes []string, secret []byte) { | |||
} | |||
} | |||
|
|||
// Setup the authorization path without a config file | |||
func SetupFromString(redirectURL, cLoginURL string, clientID string, clientSecret string, scopes []string, secret []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting @calston modify the existing Setup
? Adding functionality with a new SetupFromString
would only be a minor version update, opposed to a major version update. I'm in favor of their current implementation.
func randToken() string { | ||
b := make([]byte, 32) | ||
rand.Read(b) | ||
return base64.StdEncoding.EncodeToString(b) | ||
} | ||
|
||
// Setup the authorization path | ||
func Setup(redirectURL, credFile string, scopes []string, secret []byte) { | ||
func Setup(redirectURL, cLoginURL string, credFile string, scopes []string, secret []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't modify this function signature. Doing so will break existing functionality and require a major version increment. Could we maybe use a mutator/setter to set the loginURL
, something like SetLoginURL(loginURL string)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or something like WithLoginURL(s string)
I'd rather do this myself in a larger generic config file, probably other people to?