From 28e1be754032072f787a731c1e6cdc34a1ba5b98 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 29 Nov 2021 09:40:05 -0600 Subject: [PATCH 01/16] added AuthN/AuthZ parameters to featurebase server configuration --- ctl/server.go | 7 +++++++ server/config.go | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/ctl/server.go b/ctl/server.go index d53bf81af..ac1dcd204 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -121,4 +121,11 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // Toggle /schema/details endpoint. flags.BoolVar(&srv.Config.SchemaDetailsOn, "schema-details-on", true, "Disable /schema/details endpoint") + + // OAuth2.0 identity provider configuration + flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") + flags.StringVar(&srv.Config.Auth.IdentityProviderURL, "auth.identity-provider-url", srv.Config.Auth.IdentityProviderURL, "Base URL for identity provider.") + flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Base URL for authorize.") + flags.StringVar(&srv.Config.Auth.UserInfoURL, "auth.user-info-url", srv.Config.Auth.UserInfoURL, "Base URL for user info.") + flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Application/Client ID") } diff --git a/server/config.go b/server/config.go index a42eb7917..da9c9e43a 100644 --- a/server/config.go +++ b/server/config.go @@ -240,6 +240,24 @@ type Config struct { // Toggles /schema/details endpoint. If off, it returns empty. SchemaDetailsOn bool `toml:"schema-details-on"` + + // Enable AuthZ/AuthN + Auth struct { + // Enable AuthZ/AuthN for featurebase server + Enable bool `toml:"enable"` + + // Base URL for identity provider + IdentityProviderURL string `toml:"identity-provider-url"` + + // Authorize URL + AuthorizeURL string `toml:"authorize-url"` + + // User info URL + UserInfoURL string `toml:"user-info-url"` + + // Application/Client ID + ClientId string `toml:"client-id"` + } `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -392,6 +410,13 @@ func NewConfig() *Config { // Schema Details Toggle c.SchemaDetailsOn = true + // AuthZ/AuthN disabled by default + c.Auth.Enable = false + c.Auth.IdentityProviderURL = "" + c.Auth.AuthorizeURL = "" + c.Auth.UserInfoURL = "" + c.Auth.ClientId = "" + return c } From dc1c39fd21a76155110e9074820dfe3434cc24f8 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 29 Nov 2021 16:46:55 -0600 Subject: [PATCH 02/16] updated featurebase.conf --- install/featurebase.conf | 8 ++++++++ server/auth.go | 35 +++++++++++++++++++++++++++++++++++ server/config.go | 9 +++++---- server/server.go | 8 ++++++++ 4 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 server/auth.go diff --git a/install/featurebase.conf b/install/featurebase.conf index 73895ed6d..d3fd41deb 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -371,3 +371,11 @@ log-path = "/var/log/molecula/featurebase.log" # ============================================================================== +# Enable/Disable AuthN/AuthZ for featurebase +# Can choose identity provider, pass authorize and user-info endpoints, and client id +# [auth] +# enable = false +# identity-provider-url = "http://place-holder" +# authorize-url = "http://place-holder" +# user-info-url = "http://place-holder" +# client-id = "http://place-holder" \ No newline at end of file diff --git a/server/auth.go b/server/auth.go new file mode 100644 index 000000000..f74444963 --- /dev/null +++ b/server/auth.go @@ -0,0 +1,35 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +type Options struct { + Enable bool `toml:"enable"` + IdentityProviderURL string `toml:"identity-provider-url"` + AuthorizeURL string `toml:"authorize-url"` + UserInfoURL string `toml:"user-info-url"` + ClientId string `toml:"client-id"` +} + +func authenticateUser(opt Options) (resp bool) { + + // fmt.Println("IdentityProviderURL", opt.IdentityProviderURL) + // fmt.Println("AuthorizeURL", opt.AuthorizeURL) + // fmt.Println("UserInfoURL", opt.UserInfoURL) + // fmt.Println("ClientId", opt.ClientId) + + resp = false + + return resp +} diff --git a/server/config.go b/server/config.go index da9c9e43a..c726cb5be 100644 --- a/server/config.go +++ b/server/config.go @@ -411,11 +411,12 @@ func NewConfig() *Config { c.SchemaDetailsOn = true // AuthZ/AuthN disabled by default + // default identity provider is azure active directory c.Auth.Enable = false - c.Auth.IdentityProviderURL = "" - c.Auth.AuthorizeURL = "" - c.Auth.UserInfoURL = "" - c.Auth.ClientId = "" + c.Auth.IdentityProviderURL = "http://holder-identity-provider" + c.Auth.AuthorizeURL = "http://holder-authorize-url" + c.Auth.UserInfoURL = "http://holder-user-info-url" + c.Auth.ClientId = "http://holder-client-id" return c } diff --git a/server/server.go b/server/server.go index 29eb20f1d..e24c261cb 100644 --- a/server/server.go +++ b/server/server.go @@ -234,6 +234,14 @@ func (m *Command) Start() (err error) { return errors.Wrap(err, "setting resource limits") } + if m.Config.Auth.Enable == true { + // check authentication for user + resp := authenticateUser(m.Config.Auth) + if resp == false { + log.Fatalf("Authentication failed: Unable to access to featurebase server") + } + } + // Initialize server. if err = m.Server.Open(); err != nil { return errors.Wrap(err, "opening server") From b5ba3fb2ead2f9ef2fe42746826691ded51e83d0 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 13:09:13 -0600 Subject: [PATCH 03/16] added auth arg validation and set up auth package --- auth/auth.go | 47 +++++++++++++++++++ ctl/server.go | 10 ++-- install/featurebase.conf | 9 ++-- server/auth.go | 35 -------------- server/config.go | 56 ++++++++++++++++++----- server/config_internal_test.go | 83 ++++++++++++++++++++++++++++++++++ server/server.go | 8 ++-- 7 files changed, 189 insertions(+), 59 deletions(-) create mode 100644 auth/auth.go delete mode 100644 server/auth.go diff --git a/auth/auth.go b/auth/auth.go new file mode 100644 index 000000000..95b222bf4 --- /dev/null +++ b/auth/auth.go @@ -0,0 +1,47 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +type AUTH struct { + ClientId string + ClientSecret string + AuthorizeURL string + TokenURL string + GroupEndpointURL string +} + +// func (c *config) Init(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) { +// c.ClientId = ClientId +// c.ClientSecret = ClientSecret +// c.AuthorizeURL = AuthorizeURL +// c.TokenURL = TokenURL +// c.GroupEndpointURL = GroupEndpointURL +// } + +// apiOption is a functional option type for pilosa.API +type authOption func(*AUTH) error + +func OptAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) authOption { + return func(a *AUTH) error { + a.ClientId = ClientId + a.ClientSecret = ClientSecret + a.AuthorizeURL = AuthorizeURL + a.TokenURL = TokenURL + a.GroupEndpointURL = GroupEndpointURL + return nil + } +} + +// redirectURL diff --git a/ctl/server.go b/ctl/server.go index ac1dcd204..66a8c63d4 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -124,8 +124,10 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // OAuth2.0 identity provider configuration flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") - flags.StringVar(&srv.Config.Auth.IdentityProviderURL, "auth.identity-provider-url", srv.Config.Auth.IdentityProviderURL, "Base URL for identity provider.") - flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Base URL for authorize.") - flags.StringVar(&srv.Config.Auth.UserInfoURL, "auth.user-info-url", srv.Config.Auth.UserInfoURL, "Base URL for user info.") - flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Application/Client ID") + flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Identity Provider's Application/Client ID.") + flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Application/Client Secret.") + flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") + flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL for identity provider.") + flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") + } diff --git a/install/featurebase.conf b/install/featurebase.conf index d3fd41deb..7807134e7 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -375,7 +375,8 @@ log-path = "/var/log/molecula/featurebase.log" # Can choose identity provider, pass authorize and user-info endpoints, and client id # [auth] # enable = false -# identity-provider-url = "http://place-holder" -# authorize-url = "http://place-holder" -# user-info-url = "http://place-holder" -# client-id = "http://place-holder" \ No newline at end of file +# client-id = "" +# client-secret = "" +# authorize-url = "" +# token-url = "" +# group-endpoint-url = "" \ No newline at end of file diff --git a/server/auth.go b/server/auth.go deleted file mode 100644 index f74444963..000000000 --- a/server/auth.go +++ /dev/null @@ -1,35 +0,0 @@ -// Copyright 2017 Pilosa Corp. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package server - -type Options struct { - Enable bool `toml:"enable"` - IdentityProviderURL string `toml:"identity-provider-url"` - AuthorizeURL string `toml:"authorize-url"` - UserInfoURL string `toml:"user-info-url"` - ClientId string `toml:"client-id"` -} - -func authenticateUser(opt Options) (resp bool) { - - // fmt.Println("IdentityProviderURL", opt.IdentityProviderURL) - // fmt.Println("AuthorizeURL", opt.AuthorizeURL) - // fmt.Println("UserInfoURL", opt.UserInfoURL) - // fmt.Println("ClientId", opt.ClientId) - - resp = false - - return resp -} diff --git a/server/config.go b/server/config.go index c726cb5be..c65ae7f9c 100644 --- a/server/config.go +++ b/server/config.go @@ -19,6 +19,7 @@ import ( "fmt" "log" "net" + "net/url" "runtime" "strconv" "strings" @@ -246,17 +247,20 @@ type Config struct { // Enable AuthZ/AuthN for featurebase server Enable bool `toml:"enable"` - // Base URL for identity provider - IdentityProviderURL string `toml:"identity-provider-url"` + // Application/Client ID + ClientId string `toml:"client-id"` + + // Client Secret + ClientSecret string `toml:"client-secret"` // Authorize URL AuthorizeURL string `toml:"authorize-url"` - // User info URL - UserInfoURL string `toml:"user-info-url"` + // Token URL + TokenURL string `toml:"token-url"` - // Application/Client ID - ClientId string `toml:"client-id"` + // Group Endpoint URL + GroupEndpointURL string `toml:"group-endpoint-url"` } `toml:"auth"` } @@ -291,6 +295,7 @@ func (c *Config) validate() error { "Etcd.ClusterURL", c.Etcd.ClusterURL, "Postgres.Bind", c.Postgres.Bind, } + ports := make(map[int]bool) n := len(hostPort) for i := 0; i < n; i += 2 { @@ -411,12 +416,7 @@ func NewConfig() *Config { c.SchemaDetailsOn = true // AuthZ/AuthN disabled by default - // default identity provider is azure active directory c.Auth.Enable = false - c.Auth.IdentityProviderURL = "http://holder-identity-provider" - c.Auth.AuthorizeURL = "http://holder-authorize-url" - c.Auth.UserInfoURL = "http://holder-user-info-url" - c.Auth.ClientId = "http://holder-client-id" return c } @@ -628,3 +628,37 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin // No IPv4 address, return the first resolved address instead. return addrs[0].String(), nil } + +func (c *Config) ValidateAuth() error { + authURL := []string{ + "ClientId", c.Auth.ClientId, + "ClientSecret", c.Auth.ClientSecret, + "AuthorizeURL", c.Auth.AuthorizeURL, + "TokenURL", c.Auth.TokenURL, + "GroupEndpointURL", c.Auth.GroupEndpointURL, + } + + n := len(authURL) + for i := 0; i < n; i += 2 { + name := authURL[i] + value := authURL[i+1] + if strings.Contains(name, "URL") { + _, err := url.ParseRequestURI(value) + if err != nil { + return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) + } + } else { + if value == "" { + return fmt.Errorf("Empty string for auth config %s", name) + } + } + } + return nil +} + +func (c *Config) MustValidateAuth() { + err := c.ValidateAuth() + if err != nil { + panic(err) + } +} diff --git a/server/config_internal_test.go b/server/config_internal_test.go index f48db1a16..97b1cfa20 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -288,3 +288,86 @@ 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) { + tests := []struct { + expErr string + input params + expected params + }{ + {"Empty string for auth config ClientId", + params{true, "", "", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Empty string for auth config ClientSecret", + params{true, "clientid", "", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Empty string for auth config ClientId", + params{true, "", "clientSecret", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config AuthorizeURL", + params{true, "client", "secret", "", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config TokenURL", + params{true, "client", "secret", "https://url.com/", "", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config GroupEndpointURL", + params{true, "client", "secret", "https://url.com/", "https://url.com/", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config AuthorizeURL", + params{true, "client", "secret", "string", "https://url.com/", "https://url.com/"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config TokenURL", + params{true, "client", "secret", "https://url.com/", "not-a-url", ""}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"Invalid URL for auth config GroupEndpointURL", + params{true, "client", "secret", "https://url.com/", "https://url.com/", "not-valid-url"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + {"", + params{true, "client", "secret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + }, + } + + 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 + + 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()) + } + return + } + }) + } +} diff --git a/server/server.go b/server/server.go index e24c261cb..30b1794ee 100644 --- a/server/server.go +++ b/server/server.go @@ -41,6 +41,7 @@ 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" @@ -235,11 +236,8 @@ func (m *Command) Start() (err error) { } if m.Config.Auth.Enable == true { - // check authentication for user - resp := authenticateUser(m.Config.Auth) - if resp == false { - log.Fatalf("Authentication failed: Unable to access to featurebase server") - } + m.Config.MustValidateAuth() + auth.OptAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) } // Initialize server. From 64b31da4a16e8d76321507d242c63fe75f728eab Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:00:51 -0600 Subject: [PATCH 04/16] resolved duplicated line --- server/config_internal_test.go | 66 +++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 97b1cfa20..98da9c0a9 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -299,50 +299,60 @@ type params struct { } func TestConfig_validateAuth(t *testing.T) { + errorMesgClientID := "Empty string for auth config ClientId" + errorMesgClientSecret := "Empty string for auth config ClientSecret" + errorMesgAuthURL := "Invalid URL for auth config AuthorizeURL" + errorMesgTokenURL := "Invalid URL for auth config TokenURL" + errorMesgGroupEndpointURL := "Invalid URL for auth config GroupEndpointURL" + validTestURL := "https://url.com/" + validClientID := "clientid" + validClientSecret := "clientSecret" + notValidURL := "not-a-url" + tests := []struct { expErr string input params expected params }{ - {"Empty string for auth config ClientId", + {errorMesgClientID, params{true, "", "", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Empty string for auth config ClientSecret", - params{true, "clientid", "", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgClientSecret, + params{true, validClientID, "", "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Empty string for auth config ClientId", - params{true, "", "clientSecret", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgClientID, + params{true, "", validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config AuthorizeURL", - params{true, "client", "secret", "", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgAuthURL, + params{true, validClientID, validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config TokenURL", - params{true, "client", "secret", "https://url.com/", "", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgTokenURL, + params{true, validClientID, validClientSecret, validTestURL, "", ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config GroupEndpointURL", - params{true, "client", "secret", "https://url.com/", "https://url.com/", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgGroupEndpointURL, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config AuthorizeURL", - params{true, "client", "secret", "string", "https://url.com/", "https://url.com/"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgAuthURL, + params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config TokenURL", - params{true, "client", "secret", "https://url.com/", "not-a-url", ""}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgTokenURL, + params{true, validClientID, validClientSecret, validTestURL, notValidURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"Invalid URL for auth config GroupEndpointURL", - params{true, "client", "secret", "https://url.com/", "https://url.com/", "not-valid-url"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + {errorMesgGroupEndpointURL, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {"", - params{true, "client", "secret", "https://url.com/", "https://url.com/", "https://url.com/"}, - params{true, "clientidstring", "clientSecret", "https://url.com/", "https://url.com/", "https://url.com/"}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, } From 91c8bf5e05524b39134c6ff10539c98047fa1801 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:04:21 -0600 Subject: [PATCH 05/16] fixed duplicated empty string --- server/config_internal_test.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 98da9c0a9..60735d45e 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -308,6 +308,7 @@ func TestConfig_validateAuth(t *testing.T) { validClientID := "clientid" validClientSecret := "clientSecret" notValidURL := "not-a-url" + emptyString := "" tests := []struct { expErr string @@ -315,27 +316,27 @@ func TestConfig_validateAuth(t *testing.T) { expected params }{ {errorMesgClientID, - params{true, "", "", "", "", ""}, + params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientSecret, - params{true, validClientID, "", "", "", ""}, + params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientID, - params{true, "", validClientSecret, "", "", ""}, + params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, - params{true, validClientID, validClientSecret, "", "", ""}, + params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, "", ""}, + params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, @@ -343,14 +344,14 @@ func TestConfig_validateAuth(t *testing.T) { params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, notValidURL, ""}, + params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, - {"", + {emptyString, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, From 7de3eaa935f0e243a9621359264b896621b31e17 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:19:59 -0600 Subject: [PATCH 06/16] resolved review's comment --- server/config_internal_test.go | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 60735d45e..b8aede210 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -311,49 +311,38 @@ func TestConfig_validateAuth(t *testing.T) { emptyString := "" tests := []struct { - expErr string - input params - expected params + expErr string + input params }{ {errorMesgClientID, params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientSecret, params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgClientID, params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgAuthURL, params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgTokenURL, params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {errorMesgGroupEndpointURL, params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, {emptyString, params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, }, } From aebd4c4c62f1ee8fc3bcaffebdbf8bcba98ffe30 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:22:08 -0600 Subject: [PATCH 07/16] fixed formatting --- server/config_internal_test.go | 41 +++++++++------------------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index b8aede210..1bd33be84 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -309,41 +309,22 @@ func TestConfig_validateAuth(t *testing.T) { validClientSecret := "clientSecret" notValidURL := "not-a-url" emptyString := "" + enable := true tests := []struct { expErr string input params }{ - {errorMesgClientID, - params{true, emptyString, emptyString, emptyString, emptyString, emptyString}, - }, - {errorMesgClientSecret, - params{true, validClientID, emptyString, emptyString, emptyString, emptyString}, - }, - {errorMesgClientID, - params{true, emptyString, validClientSecret, emptyString, emptyString, emptyString}, - }, - {errorMesgAuthURL, - params{true, validClientID, validClientSecret, emptyString, emptyString, emptyString}, - }, - {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, emptyString, emptyString}, - }, - {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}, - }, - {errorMesgAuthURL, - params{true, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}, - }, - {errorMesgTokenURL, - params{true, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}, - }, - {errorMesgGroupEndpointURL, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}, - }, - {emptyString, - params{true, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}, - }, + {errorMesgClientID, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgClientSecret, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, + {errorMesgClientID, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgAuthURL, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, + {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, + {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, + {errorMesgAuthURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, + {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, + {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, + {emptyString, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, validTestURL}}, } for i, test := range tests { From 50d80f2f1c67f53f00349c5e01bf03d4a4ca3dad Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 14:52:38 -0600 Subject: [PATCH 08/16] fixed arg cli descriptions --- ctl/server.go | 4 ++-- server/config.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ctl/server.go b/ctl/server.go index 66a8c63d4..a368637e3 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -125,9 +125,9 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { // OAuth2.0 identity provider configuration flags.BoolVar(&srv.Config.Auth.Enable, "auth.enable", false, "Enable AuthN/AuthZ of featurebase, disabled by default.") flags.StringVar(&srv.Config.Auth.ClientId, "auth.client-id", srv.Config.Auth.ClientId, "Identity Provider's Application/Client ID.") - flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Application/Client Secret.") + flags.StringVar(&srv.Config.Auth.ClientSecret, "auth.client-secret", srv.Config.Auth.ClientSecret, "Identity Provider's Client Secret.") flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") - flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL for identity provider.") + flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.") flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") } diff --git a/server/config.go b/server/config.go index c65ae7f9c..9f7a24e61 100644 --- a/server/config.go +++ b/server/config.go @@ -630,7 +630,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() error { - authURL := []string{ + authConfig := []string{ "ClientId", c.Auth.ClientId, "ClientSecret", c.Auth.ClientSecret, "AuthorizeURL", c.Auth.AuthorizeURL, @@ -638,10 +638,10 @@ func (c *Config) ValidateAuth() error { "GroupEndpointURL", c.Auth.GroupEndpointURL, } - n := len(authURL) + n := len(authConfig) for i := 0; i < n; i += 2 { - name := authURL[i] - value := authURL[i+1] + name := authConfig[i] + value := authConfig[i+1] if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { From 78e11e0fda2aebdf7760b6eb9418c0b5899b8e0a Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 16:15:15 -0600 Subject: [PATCH 09/16] resolved reviewer's comments --- auth/auth.go | 30 +++++++++--------------------- server/config.go | 3 --- server/server.go | 5 ++++- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 95b222bf4..2abfda28f 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -22,26 +22,14 @@ type AUTH struct { GroupEndpointURL string } -// func (c *config) Init(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) { -// c.ClientId = ClientId -// c.ClientSecret = ClientSecret -// c.AuthorizeURL = AuthorizeURL -// c.TokenURL = TokenURL -// c.GroupEndpointURL = GroupEndpointURL -// } - -// apiOption is a functional option type for pilosa.API -type authOption func(*AUTH) error - -func OptAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) authOption { - return func(a *AUTH) error { - a.ClientId = ClientId - a.ClientSecret = ClientSecret - a.AuthorizeURL = AuthorizeURL - a.TokenURL = TokenURL - a.GroupEndpointURL = GroupEndpointURL - return nil +func NewAuth(ClientId, ClientSecret, AuthorizeURL, TokenURL, GroupEndpointURL string) AUTH { + a := AUTH{ + ClientId: ClientId, + ClientSecret: ClientSecret, + AuthorizeURL: AuthorizeURL, + TokenURL: TokenURL, + GroupEndpointURL: GroupEndpointURL, } -} -// redirectURL + return a +} diff --git a/server/config.go b/server/config.go index 9f7a24e61..f8e411f37 100644 --- a/server/config.go +++ b/server/config.go @@ -415,9 +415,6 @@ func NewConfig() *Config { // Schema Details Toggle c.SchemaDetailsOn = true - // AuthZ/AuthN disabled by default - c.Auth.Enable = false - return c } diff --git a/server/server.go b/server/server.go index 30b1794ee..768f000ed 100644 --- a/server/server.go +++ b/server/server.go @@ -56,6 +56,7 @@ 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" ) @@ -237,7 +238,9 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable == true { m.Config.MustValidateAuth() - auth.OptAuth(m.Config.Auth.ClientId, m.Config.Auth.ClientSecret, m.Config.Auth.AuthorizeURL, m.Config.Auth.TokenURL, m.Config.Auth.GroupEndpointURL) + 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. From 978f236c5948e1c34afd05db30f3a719434e8173 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 2 Dec 2021 16:36:04 -0600 Subject: [PATCH 10/16] resolved additional comments --- server/config.go | 28 ++++++++++++---------------- server/config_internal_test.go | 27 +++++++++++++-------------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/server/config.go b/server/config.go index f8e411f37..e93244dcb 100644 --- a/server/config.go +++ b/server/config.go @@ -627,35 +627,31 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() error { - authConfig := []string{ - "ClientId", c.Auth.ClientId, - "ClientSecret", c.Auth.ClientSecret, - "AuthorizeURL", c.Auth.AuthorizeURL, - "TokenURL", c.Auth.TokenURL, - "GroupEndpointURL", c.Auth.GroupEndpointURL, + authConfig := map[string]string{ + "ClientId": c.Auth.ClientId, + "ClientSecret": c.Auth.ClientSecret, + "AuthorizeURL": c.Auth.AuthorizeURL, + "TokenURL": c.Auth.TokenURL, + "GroupEndpointURL": c.Auth.GroupEndpointURL, } - n := len(authConfig) - for i := 0; i < n; i += 2 { - name := authConfig[i] - value := authConfig[i+1] + for name, value := range authConfig { + if value == "" { + return fmt.Errorf("Empty string for auth config %s", name) + } + if strings.Contains(name, "URL") { _, err := url.ParseRequestURI(value) if err != nil { return fmt.Errorf("Invalid URL for auth config %s: %s", name, err) } - } else { - if value == "" { - return fmt.Errorf("Empty string for auth config %s", name) - } } } return nil } func (c *Config) MustValidateAuth() { - err := c.ValidateAuth() - if err != nil { + if err := c.ValidateAuth(); err != nil { panic(err) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 1bd33be84..5daac4233 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -299,32 +299,31 @@ type params struct { } func TestConfig_validateAuth(t *testing.T) { - errorMesgClientID := "Empty string for auth config ClientId" - errorMesgClientSecret := "Empty string for auth config ClientSecret" - errorMesgAuthURL := "Invalid URL for auth config AuthorizeURL" - errorMesgTokenURL := "Invalid URL for auth config TokenURL" - errorMesgGroupEndpointURL := "Invalid URL for auth config GroupEndpointURL" + errorMesgEmpty := "Empty string" + errorMesgURL := "Invalid URL" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" notValidURL := "not-a-url" emptyString := "" enable := true + disable := false tests := []struct { expErr string input params }{ - {errorMesgClientID, params{enable, emptyString, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgClientSecret, params{enable, validClientID, emptyString, emptyString, emptyString, emptyString}}, - {errorMesgClientID, params{enable, emptyString, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgAuthURL, params{enable, validClientID, validClientSecret, emptyString, emptyString, emptyString}}, - {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, emptyString, emptyString}}, - {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, emptyString}}, - {errorMesgAuthURL, params{enable, validClientID, validClientSecret, notValidURL, validTestURL, validTestURL}}, - {errorMesgTokenURL, params{enable, validClientID, validClientSecret, validTestURL, notValidURL, emptyString}}, - {errorMesgGroupEndpointURL, params{enable, validClientID, validClientSecret, validTestURL, validTestURL, notValidURL}}, + {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}}, } for i, test := range tests { From 06fd65cb31cdc65fc63b0e4f03b95ae138e3faf9 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 10:56:21 -0600 Subject: [PATCH 11/16] 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. From 6425fc50fcf1f29224636317cb514ceef2f6024a Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 11:24:18 -0600 Subject: [PATCH 12/16] added identity provider scope url as parameter --- auth/auth.go | 3 + ctl/server.go | 1 + server/config.go | 1 + server/config_internal_test.go | 100 +++++++++++++++++---------------- 4 files changed, 58 insertions(+), 47 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index de7ed303d..4e617c998 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -32,4 +32,7 @@ type Auth struct { // Group Endpoint URL GroupEndpointURL string `toml:"group-endpoint-url"` + + // Scope URL + ScopeURL string `toml:"scope-url"` } diff --git a/ctl/server.go b/ctl/server.go index a368637e3..c5d43a937 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -129,5 +129,6 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { flags.StringVar(&srv.Config.Auth.AuthorizeURL, "auth.authorize-url", srv.Config.Auth.AuthorizeURL, "Identity Provider's Authorize URL.") flags.StringVar(&srv.Config.Auth.TokenURL, "auth.token-url", srv.Config.Auth.TokenURL, "Identity Provider's Token URL.") flags.StringVar(&srv.Config.Auth.GroupEndpointURL, "auth.group-endpoint-url", srv.Config.Auth.GroupEndpointURL, "Identity Provider's Group endpoint URL.") + flags.StringVar(&srv.Config.Auth.ScopeURL, "auth.scope-url", srv.Config.Auth.ScopeURL, "Identity Provider's Scope URL.") } diff --git a/server/config.go b/server/config.go index 79ae314ad..4d69521a1 100644 --- a/server/config.go +++ b/server/config.go @@ -616,6 +616,7 @@ func (c *Config) ValidateAuth() ([]error, error) { "AuthorizeURL": c.Auth.AuthorizeURL, "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, + "ScopeURL": c.Auth.ScopeURL, } errors := make([]error, 0) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 927f65327..8917d0fc2 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -315,6 +315,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -323,6 +324,45 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: emptyString, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: emptyString, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + ScopeURL: emptyString, + }, + }, + { + // Auth enabled, some configs are set to empty string + []string{ + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + errorMesgEmpty, + }, + auth.Auth{ + Enable: enable, + ClientId: emptyString, + ClientSecret: validClientSecret, + AuthorizeURL: emptyString, + TokenURL: emptyString, + GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { @@ -336,27 +376,11 @@ func TestConfig_validateAuth(t *testing.T) { 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, + ScopeURL: emptyString, }, }, { @@ -366,21 +390,6 @@ func TestConfig_validateAuth(t *testing.T) { 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, @@ -388,12 +397,14 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { // Auth enabled, some configs are set to empty string []string{ errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -402,10 +413,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: validTestURL, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, { - // Auth enabled, + // Auth enabled, some strings are set to invalid URL []string{ errorMesgURL, }, @@ -416,9 +428,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: notValidURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, }, }, { + // Auth enabled, some strings are set to invalid URL []string{ errorMesgURL, errorMesgURL, @@ -430,23 +444,11 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: notValidURL, GroupEndpointURL: notValidURL, + ScopeURL: validTestURL, }, }, { - []string{ - errorMesgEmpty, - errorMesgURL, - }, - auth.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: emptyString, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: notValidURL, - }, - }, - { + // Auth enabled, all configs are set properly []string{}, auth.Auth{ Enable: enable, @@ -455,15 +457,18 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: validTestURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, }, }, { + // Auth disabled, all configs are set to empty string []string{ errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: disable, @@ -472,6 +477,7 @@ func TestConfig_validateAuth(t *testing.T) { AuthorizeURL: emptyString, TokenURL: emptyString, GroupEndpointURL: emptyString, + ScopeURL: emptyString, }, }, } From cebba84beeb6c591dae7b04ee435a7764afe34a1 Mon Sep 17 00:00:00 2001 From: souhailanoor <90720110+souhailanoor@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:38:43 -0600 Subject: [PATCH 13/16] add scope to install/featurebase.conf Co-authored-by: Samir Patel <48686912+54mir@users.noreply.github.com> --- install/featurebase.conf | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/install/featurebase.conf b/install/featurebase.conf index 7807134e7..540a410f4 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -379,4 +379,5 @@ log-path = "/var/log/molecula/featurebase.log" # client-secret = "" # authorize-url = "" # token-url = "" -# group-endpoint-url = "" \ No newline at end of file +# group-endpoint-url = "" +# scope-url = "" \ No newline at end of file From e8f54581007e576746ca52c51ea3f09d9a95d94f Mon Sep 17 00:00:00 2001 From: souhailanoor <90720110+souhailanoor@users.noreply.github.com> Date: Fri, 3 Dec 2021 11:58:08 -0600 Subject: [PATCH 14/16] only validate config when auth is enabled Co-authored-by: reese <45641995+reesporte@users.noreply.github.com> --- server/config.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/config.go b/server/config.go index 4d69521a1..6bf2b22c4 100644 --- a/server/config.go +++ b/server/config.go @@ -610,6 +610,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() ([]error, error) { + if !c.Auth.Enable { + return []error{}, nil + } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret, From 90c3c67ce42048c2c345752c5b79668d12ba61d3 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 12:00:07 -0600 Subject: [PATCH 15/16] fixed test for auth disabled --- server/config_internal_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 8917d0fc2..8d5e1fea0 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -462,14 +462,7 @@ func TestConfig_validateAuth(t *testing.T) { }, { // Auth disabled, all configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, + []string{}, auth.Auth{ Enable: disable, ClientId: emptyString, From 7bcbb5eacaa2c510d3d047bf5697f8e94ebb02cc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 3 Dec 2021 12:10:31 -0600 Subject: [PATCH 16/16] fixed indentation --- server/config.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/config.go b/server/config.go index 6bf2b22c4..0c1989c7a 100644 --- a/server/config.go +++ b/server/config.go @@ -610,9 +610,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin } func (c *Config) ValidateAuth() ([]error, error) { - if !c.Auth.Enable { - return []error{}, nil - } + if !c.Auth.Enable { + return []error{}, nil + } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, "ClientSecret": c.Auth.ClientSecret,