resolved reviewer's suggestions and made it pretty & user friendly

This commit is contained in:
Souhaila Noor 2021-12-03 10:56:21 -06:00
parent 978f236c59
commit 06fd65cb31
4 changed files with 221 additions and 84 deletions

View file

@ -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"`
}

View file

@ -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)
}
}

View file

@ -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
}
})
}

View file

@ -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.