From 06fd65cb31cdc65fc63b0e4f03b95ae138e3faf9 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 10:56:21 -0600 Subject: [PATCH] resolved reviewer's suggestions and made it pretty & user friendly --- auth/auth.go | 32 ++--- server/config.go | 42 +++--- server/config_internal_test.go | 226 +++++++++++++++++++++++++++------ server/server.go | 5 - 4 files changed, 221 insertions(+), 84 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 2abfda28f..de7ed303d 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -14,22 +14,22 @@ package auth -type AUTH struct { - ClientId string - ClientSecret string - AuthorizeURL string - TokenURL string - GroupEndpointURL string -} +type Auth struct { + // Enable AuthZ/AuthN for featurebase server + Enable bool `toml:"enable"` -func NewAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) AUTH { - a := AUTH{ - ClientId: ClientId, - ClientSecret: ClientSecret, - AuthorizeURL: AuthorizeURL, - TokenURL: TokenURL, - GroupEndpointURL: GroupEndpointURL, - } + // Application/Client ID + ClientId string `toml:"client-id"` - return a + // Client Secret + ClientSecret string `toml:"client-secret"` + + // Authorize URL + AuthorizeURL string `toml:"authorize-url"` + + // Token URL + TokenURL string `toml:"token-url"` + + // Group Endpoint URL + GroupEndpointURL string `toml:"group-endpoint-url"` } diff --git a/server/config.go b/server/config.go index e93244dcb..79ae314ad 100644 --- a/server/config.go +++ b/server/config.go @@ -25,6 +25,7 @@ import ( "strings" "time" + "github.com/molecula/featurebase/v2/auth" petcd "github.com/molecula/featurebase/v2/etcd" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/storage" @@ -243,25 +244,7 @@ type Config struct { SchemaDetailsOn bool `toml:"schema-details-on"` // Enable AuthZ/AuthN - Auth struct { - // Enable AuthZ/AuthN for featurebase server - Enable bool `toml:"enable"` - - // Application/Client ID - ClientId string `toml:"client-id"` - - // Client Secret - ClientSecret string `toml:"client-secret"` - - // Authorize URL - AuthorizeURL string `toml:"authorize-url"` - - // Token URL - TokenURL string `toml:"token-url"` - - // Group Endpoint URL - GroupEndpointURL string `toml:"group-endpoint-url"` - } `toml:"auth"` + Auth auth.Auth `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -626,7 +609,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin return addrs[0].String(), nil } -func (c *Config) ValidateAuth() error { +func (c *Config) ValidateAuth() ([]error, error) { authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret, @@ -635,23 +618,32 @@ func (c *Config) ValidateAuth() error { "GroupEndpointURL": c.Auth.GroupEndpointURL, } + errors := make([]error, 0) for name, value := range authConfig { if value == "" { - return fmt.Errorf("Empty string for auth config %s", name) + errors = append(errors, fmt.Errorf("Empty string for auth config %s", name)) + continue } if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { - return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) + errors = append(errors, fmt.Errorf("Invalid URL for auth config %s: %s", name, err)) + continue } } } - return nil + if len(errors) > 0 { + return errors, fmt.Errorf("there were errors validating config") + } + return errors, nil } func (c *Config) MustValidateAuth() { - if err := c.ValidateAuth(); err != nil { - panic(err) + if errors, err := c.ValidateAuth(); err != nil { + for _, e := range errors { + log.Println(e) + } + log.Fatal(err) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 5daac4233..927f65327 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -21,6 +21,8 @@ import ( "os" "strings" "testing" + + "github.com/molecula/featurebase/v2/auth" ) type addrs struct{ bind, advertise string } @@ -289,15 +291,6 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } } -type params struct { - enable bool - clientId string - clientSecret string - authorizeURL string - tokenURL string - groupEndpointURL string -} - func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty := "Empty string" errorMesgURL := "Invalid URL" @@ -310,43 +303,200 @@ func TestConfig_validateAuth(t *testing.T) { disable := false tests := []struct { - expErr string - input params + expErrs []string + input auth.Auth }{ - {errorMesgEmpty, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, - {errorMesgEmpty, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, - {errorMesgURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, - {emptyString, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}}, - {errorMesgEmpty, params{disable, emptyString, emptyString, emptyString, emptyString, emptyString}}, + + { + // Auth enabled, all configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: emptyString, + }, + }, + { + // Auth enabled, + []string{ + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: notValidURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + }, + }, + { + []string{ + errorMesgURL, + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: notValidURL, + GroupEndpointURL: notValidURL, + }, + }, + { + []string{ + errorMesgEmpty, + errorMesgURL, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: notValidURL, + }, + }, + { + []string{}, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + }, + }, + { + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: disable, + ClientId: emptyString, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + }, + }, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { c := NewConfig() - c.Auth.Enable = test.input.enable - c.Auth.ClientId = test.input.clientId - c.Auth.ClientSecret = test.input.clientSecret - c.Auth.AuthorizeURL = test.input.authorizeURL - c.Auth.TokenURL = test.input.tokenURL - c.Auth.GroupEndpointURL = test.input.groupEndpointURL + c.Auth = test.input - err := c.ValidateAuth() - - if err != nil && test.expErr == "" { - t.Fatal(err) - } else if err == nil && test.expErr != "" { - t.Fatalf("expected error string to contain %s, but got no error", test.expErr) - } else if err != nil && test.expErr != "" { - if !strings.Contains(err.Error(), test.expErr) { - t.Fatalf("expected error string to contain %s, but got %s", test.expErr, err.Error()) + errors, err := c.ValidateAuth() + if len(test.expErrs) > 0 { + if err == nil { + t.Fatal("expected errors, but none were found") + } + } + + if len(errors) != len(test.expErrs) { + fmt.Printf("%+v\n", errors) + t.Fatalf("expected %v errors but got %v", len(test.expErrs), len(errors)) + } + + for i, e := range errors { + if !strings.Contains(e.Error(), test.expErrs[i]) { + t.Errorf("expected error to contain %s, but got %s", test.expErrs[i], e.Error()) } - return } }) } diff --git a/server/server.go b/server/server.go index 768f000ed..943813b07 100644 --- a/server/server.go +++ b/server/server.go @@ -41,7 +41,6 @@ import ( "golang.org/x/sync/errgroup" pilosa "github.com/molecula/featurebase/v2" - "github.com/molecula/featurebase/v2/auth" "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/encoding/proto" petcd "github.com/molecula/featurebase/v2/etcd" @@ -56,7 +55,6 @@ import ( "github.com/molecula/featurebase/v2/statsd" "github.com/molecula/featurebase/v2/syswrap" "github.com/molecula/featurebase/v2/testhook" - "github.com/molecula/featurebase/v2/vprint" "github.com/pelletier/go-toml" "github.com/pkg/errors" ) @@ -238,9 +236,6 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable == true { m.Config.MustValidateAuth() - authArgs := auth.NewAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) - vprint.VV("Auth: %v", authArgs) - // print statement is so that binary compiles, and golang doesn't complaint about declared but unused var } // Initialize server.