-
Notifications
You must be signed in to change notification settings - Fork 24
enhance: add validate tool for github auth provider #736
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,37 +16,29 @@ import ( | |
"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/validation" | ||
"github.com/obot-platform/tools/auth-providers-common/pkg/env" | ||
"github.com/obot-platform/tools/auth-providers-common/pkg/state" | ||
"github.com/obot-platform/tools/github-auth-provider/pkg/config" | ||
"github.com/obot-platform/tools/github-auth-provider/pkg/profile" | ||
"github.com/sahilm/fuzzy" | ||
) | ||
|
||
type Options struct { | ||
ClientID string `env:"OBOT_GITHUB_AUTH_PROVIDER_CLIENT_ID"` | ||
ClientSecret string `env:"OBOT_GITHUB_AUTH_PROVIDER_CLIENT_SECRET"` | ||
ObotServerURL string `env:"OBOT_SERVER_URL"` | ||
PostgresConnectionDSN string `env:"OBOT_AUTH_PROVIDER_POSTGRES_CONNECTION_DSN" optional:"true"` | ||
AuthCookieSecret string `usage:"Secret used to encrypt cookie" env:"OBOT_AUTH_PROVIDER_COOKIE_SECRET"` | ||
AuthEmailDomains string `usage:"Email domains allowed for authentication" default:"*" env:"OBOT_AUTH_PROVIDER_EMAIL_DOMAINS"` | ||
AuthTokenRefreshDuration string `usage:"Duration to refresh auth token after" optional:"true" default:"1h" env:"OBOT_AUTH_PROVIDER_TOKEN_REFRESH_DURATION"` | ||
GitHubOrg *string `usage:"restrict logins to members of this GitHub organization" optional:"true" env:"OBOT_GITHUB_AUTH_PROVIDER_ORG"` | ||
GitHubAllowUsers *string `usage:"users allowed to log in, even if they do not belong to the specified org and team or collaborators" optional:"true" env:"OBOT_GITHUB_AUTH_PROVIDER_ALLOW_USERS"` | ||
} | ||
|
||
func main() { | ||
var opts Options | ||
if err := env.LoadEnvForStruct(&opts); err != nil { | ||
fmt.Printf("ERROR: github-auth-provider: failed to load options: %v\n", err) | ||
os.Exit(1) | ||
} | ||
opts, err := config.LoadEnv() | ||
if len(os.Args) > 1 && os.Args[1] == "validate" { | ||
if err != nil { | ||
var validationErr env.ValidationError | ||
if errors.As(err, &validationErr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, when validation fails, you should do this and return os.exit(0) https://github.com/obot-platform/enterprise-tools/pull/64/files#diff-01e49a110328ad7a46eea37ea6af13367c4de003c836988ef175f66f9750839aR27 This is so that the backend will not interpret it as an error and just read from output(json object) to decide if it is an error. |
||
if err := json.NewEncoder(os.Stdout).Encode(validationErr); err != nil { | ||
fmt.Printf("ERROR: github-auth-provider: failed to encode validation errors: %v\n", err) | ||
os.Exit(1) | ||
} | ||
} | ||
} | ||
|
||
refreshDuration, err := time.ParseDuration(opts.AuthTokenRefreshDuration) | ||
if err != nil { | ||
fmt.Printf("ERROR: github-auth-provider: failed to parse token refresh duration: %v\n", err) | ||
os.Exit(1) | ||
return | ||
} | ||
|
||
if refreshDuration < 0 { | ||
fmt.Printf("ERROR: github-auth-provider: token refresh duration must be greater than 0\n") | ||
if err != nil { | ||
fmt.Printf("ERROR: github-auth-provider: failed to load and validate options: %v\n", err) | ||
os.Exit(1) | ||
} | ||
|
||
|
@@ -64,12 +56,8 @@ func main() { | |
legacyOpts.LegacyProvider.ClientSecret = opts.ClientSecret | ||
|
||
// GitHub-specific options | ||
if opts.GitHubOrg != nil { | ||
legacyOpts.LegacyProvider.GitHubOrg = *opts.GitHubOrg | ||
} | ||
if opts.GitHubAllowUsers != nil { | ||
legacyOpts.LegacyProvider.GitHubUsers = strings.Split(*opts.GitHubAllowUsers, ",") | ||
} | ||
legacyOpts.LegacyProvider.GitHubOrg = opts.GitHubOrg | ||
legacyOpts.LegacyProvider.GitHubUsers = opts.GitHubAllowUsers | ||
|
||
oauthProxyOpts, err := legacyOpts.ToOptions() | ||
if err != nil { | ||
|
@@ -84,20 +72,14 @@ func main() { | |
oauthProxyOpts.Session.Postgres.ConnectionDSN = opts.PostgresConnectionDSN | ||
oauthProxyOpts.Session.Postgres.TableNamePrefix = "github_" | ||
} | ||
oauthProxyOpts.Cookie.Refresh = refreshDuration | ||
oauthProxyOpts.Cookie.Refresh = opts.AuthTokenRefreshDuration | ||
oauthProxyOpts.Cookie.Name = "obot_access_token" | ||
oauthProxyOpts.Cookie.Secret = string(cookieSecret) | ||
oauthProxyOpts.Cookie.Secure = strings.HasPrefix(opts.ObotServerURL, "https://") | ||
oauthProxyOpts.Cookie.CSRFExpire = 30 * time.Minute | ||
oauthProxyOpts.Templates.Path = os.Getenv("GPTSCRIPT_TOOL_DIR") + "/../auth-providers-common/templates" | ||
oauthProxyOpts.RawRedirectURL = opts.ObotServerURL + "/" | ||
if opts.AuthEmailDomains != "" { | ||
emailDomains := strings.Split(opts.AuthEmailDomains, ",") | ||
for i := range emailDomains { | ||
emailDomains[i] = strings.TrimSpace(emailDomains[i]) | ||
} | ||
oauthProxyOpts.EmailDomains = emailDomains | ||
} | ||
oauthProxyOpts.EmailDomains = opts.AuthEmailDomains | ||
oauthProxyOpts.Logging.RequestEnabled = false | ||
oauthProxyOpts.Logging.AuthEnabled = false | ||
oauthProxyOpts.Logging.StandardEnabled = false | ||
|
@@ -131,7 +113,7 @@ func main() { | |
} | ||
json.NewEncoder(w).Encode(userInfo) | ||
}) | ||
mux.HandleFunc("/obot-list-auth-groups", listGroups(*opts.GitHubOrg)) | ||
mux.HandleFunc("/obot-list-auth-groups", listGroups(opts.GitHubOrg)) | ||
mux.HandleFunc("/obot-list-user-auth-groups", listUserGroups) | ||
mux.HandleFunc("/", oauthProxy.ServeHTTP) | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,176 @@ | ||||||
package config | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
"regexp" | ||||||
"strings" | ||||||
"time" | ||||||
|
||||||
"github.com/obot-platform/tools/auth-providers-common/pkg/env" | ||||||
) | ||||||
|
||||||
// options is the private struct that holds raw environment variable values | ||||||
type options struct { | ||||||
ClientID string `env:"OBOT_GITHUB_AUTH_PROVIDER_CLIENT_ID"` | ||||||
ClientSecret string `env:"OBOT_GITHUB_AUTH_PROVIDER_CLIENT_SECRET"` | ||||||
ObotServerURL string `env:"OBOT_SERVER_URL"` | ||||||
PostgresConnectionDSN string `env:"OBOT_AUTH_PROVIDER_POSTGRES_CONNECTION_DSN" optional:"true"` | ||||||
AuthCookieSecret string `usage:"Secret used to encrypt cookie" env:"OBOT_AUTH_PROVIDER_COOKIE_SECRET"` | ||||||
AuthEmailDomains string `usage:"Email domains allowed for authentication" default:"*" env:"OBOT_AUTH_PROVIDER_EMAIL_DOMAINS"` | ||||||
AuthTokenRefreshDuration string `usage:"Duration to refresh auth token after" optional:"true" default:"1h" env:"OBOT_AUTH_PROVIDER_TOKEN_REFRESH_DURATION"` | ||||||
GitHubOrg *string `usage:"restrict logins to members of this GitHub organization" optional:"true" env:"OBOT_GITHUB_AUTH_PROVIDER_ORG"` | ||||||
GitHubAllowUsers *string `usage:"users allowed to log in, even if they do not belong to the specified org and team or collaborators" optional:"true" env:"OBOT_GITHUB_AUTH_PROVIDER_ALLOW_USERS"` | ||||||
} | ||||||
|
||||||
// Options is the public struct that holds validated and processed configuration values | ||||||
type Options struct { | ||||||
options | ||||||
AuthEmailDomains []string | ||||||
AuthTokenRefreshDuration time.Duration | ||||||
GitHubOrg string | ||||||
GitHubAllowUsers []string | ||||||
} | ||||||
|
||||||
var ( | ||||||
gitHubLoginRegex = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`) | ||||||
emailDomainRegex = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?)*$`) | ||||||
) | ||||||
|
||||||
// LoadEnv loads environment variables, validates them, and returns the completed configuration | ||||||
func LoadEnv() (*Options, error) { | ||||||
completed, err := loadEnv() | ||||||
if err != nil { | ||||||
return nil, env.ValidationError{Err: err} | ||||||
} | ||||||
|
||||||
return completed, nil | ||||||
} | ||||||
|
||||||
func loadEnv() (*Options, error) { | ||||||
var opts options | ||||||
if err := env.LoadEnvForStruct(&opts); err != nil { | ||||||
return nil, fmt.Errorf("failed to load environment variables: %w", err) | ||||||
} | ||||||
|
||||||
return complete(opts) | ||||||
} | ||||||
|
||||||
func complete(o options) (*Options, error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
var ( | ||||||
validationErrors env.FieldValidationErrors | ||||||
completedOptions = Options{ | ||||||
options: o, | ||||||
} | ||||||
) | ||||||
|
||||||
if o.AuthEmailDomains != "" { | ||||||
// TODO(njhale): Add validation for email domains | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Will remove this TODO; it's no longer valid. |
||||||
var ( | ||||||
emailDomains = strings.Split(o.AuthEmailDomains, ",") | ||||||
errorMsg string | ||||||
) | ||||||
for i := range emailDomains { | ||||||
switch domain := strings.TrimSpace(emailDomains[i]); { | ||||||
case domain == "": | ||||||
errorMsg = "cannot contain empty email domains" | ||||||
case domain == "*" && len(emailDomains) > 1: | ||||||
errorMsg = "cannot specify multiple email domains when * is provided" | ||||||
case domain != "*" && !emailDomainRegex.MatchString(domain): | ||||||
errorMsg = fmt.Sprintf("'%s' is not a valid email domain", domain) | ||||||
default: | ||||||
emailDomains[i] = domain | ||||||
continue | ||||||
} | ||||||
|
||||||
// Stop after the first email domain validation error | ||||||
break | ||||||
} | ||||||
Comment on lines
+66
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure this configuration is common to all auth providers. It has me wondering if part of this validation /completion should live in |
||||||
|
||||||
if errorMsg != "" { | ||||||
validationErrors = append(validationErrors, env.FieldValidationError{ | ||||||
EnvVar: "OBOT_AUTH_PROVIDER_EMAIL_DOMAINS", | ||||||
Message: errorMsg, | ||||||
Value: o.AuthEmailDomains, | ||||||
Sensitive: false, | ||||||
}) | ||||||
} else { | ||||||
completedOptions.AuthEmailDomains = emailDomains | ||||||
} | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
} | ||||||
|
||||||
refreshDuration, err := time.ParseDuration(o.AuthTokenRefreshDuration) | ||||||
if err != nil || refreshDuration <= 0 { | ||||||
validationErrors = append(validationErrors, env.FieldValidationError{ | ||||||
EnvVar: "OBOT_AUTH_PROVIDER_TOKEN_REFRESH_DURATION", | ||||||
Message: "must be a valid duration string and greater than 0", | ||||||
Value: o.AuthTokenRefreshDuration, | ||||||
Sensitive: false, | ||||||
}) | ||||||
} else { | ||||||
completedOptions.AuthTokenRefreshDuration = refreshDuration | ||||||
} | ||||||
Comment on lines
+102
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about generic/composable validation here. |
||||||
|
||||||
if o.GitHubOrg != nil && *o.GitHubOrg != "" { | ||||||
var ( | ||||||
org = *o.GitHubOrg | ||||||
errorMsg string | ||||||
) | ||||||
if len(org) > 39 { | ||||||
errorMsg = fmt.Sprintf("must be 39 characters or less, got %d characters", len(org)) | ||||||
} | ||||||
if !gitHubLoginRegex.MatchString(org) { | ||||||
errorMsg = "is not a valid GitHub organization login (must contain only alphanumeric characters and single hyphens, cannot start or end with hyphen)" | ||||||
} | ||||||
if errorMsg != "" { | ||||||
validationErrors = append(validationErrors, env.FieldValidationError{ | ||||||
EnvVar: "OBOT_GITHUB_AUTH_PROVIDER_ORG", | ||||||
Message: errorMsg, | ||||||
Value: org, | ||||||
Sensitive: false, | ||||||
}) | ||||||
} else { | ||||||
completedOptions.GitHubOrg = org | ||||||
} | ||||||
} | ||||||
|
||||||
if o.GitHubAllowUsers != nil && *o.GitHubAllowUsers != "" { | ||||||
var ( | ||||||
users = strings.Split(*o.GitHubAllowUsers, ",") | ||||||
errorMsg string | ||||||
) | ||||||
for i := range users { | ||||||
switch user := strings.TrimSpace(users[i]); { | ||||||
case user == "": | ||||||
errorMsg = "cannot contain empty users" | ||||||
case len(user) > 39: | ||||||
errorMsg = fmt.Sprintf("user '%s' must be 39 characters or less, got %d characters", user, len(user)) | ||||||
case !gitHubLoginRegex.MatchString(user): | ||||||
errorMsg = fmt.Sprintf("user '%s' is not a valid GitHub username (must contain only alphanumeric characters and single hyphens, cannot start or end with hyphen)", user) | ||||||
default: | ||||||
users[i] = user | ||||||
continue | ||||||
} | ||||||
|
||||||
// Stop after the first user validation error | ||||||
break | ||||||
} | ||||||
|
||||||
if errorMsg != "" { | ||||||
validationErrors = append(validationErrors, env.FieldValidationError{ | ||||||
EnvVar: "OBOT_GITHUB_AUTH_PROVIDER_ALLOW_USERS", | ||||||
Message: errorMsg, | ||||||
Value: *o.GitHubAllowUsers, | ||||||
Sensitive: false, | ||||||
}) | ||||||
} else { | ||||||
completedOptions.GitHubAllowUsers = users | ||||||
} | ||||||
} | ||||||
|
||||||
if len(validationErrors) > 0 { | ||||||
return nil, validationErrors | ||||||
} | ||||||
|
||||||
return &completedOptions, nil | ||||||
} |
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.
Not sure passing
Value
andSensitive
fields back is necessary, we don't use them in the front end ATM.