From 01e4baab04398fe4296afd4bd2316fc4332e1b22 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 10 Dec 2021 14:07:24 -0600 Subject: [PATCH 01/10] determine permission for user access to index --- auth/auth.go | 114 +++++++++++++++++++ auth/auth_test.go | 231 +++++++++++++++++++++++++++++++++++++++ ctl/server.go | 2 +- install/featurebase.conf | 3 +- server/config.go | 10 ++ 5 files changed, 358 insertions(+), 2 deletions(-) create mode 100644 auth/auth_test.go diff --git a/auth/auth.go b/auth/auth.go index 4e617c998..344055337 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -14,6 +14,15 @@ package auth +import ( + "fmt" + "io/ioutil" + "log" + "path/filepath" + + "gopkg.in/yaml.v2" +) + type Auth struct { // Enable AuthZ/AuthN for featurebase server Enable bool `toml:"enable"` @@ -35,4 +44,109 @@ type Auth struct { // Scope URL ScopeURL string `toml:"scope-url"` + + // Permissions file for groups + PermissionsFile string `toml:"permissions"` +} + +type GroupPermissions struct { + Permissions []Permissions `yaml:"group_permissions"` +} + +type Permissions struct { + GroupId string `yaml:"groupId"` + Index string `yaml:"index"` + Permission string `yaml:"permission"` +} + +func ReadPermissionsFile(filePath string) (yamlData []byte) { + filePathAbs, _ := filepath.Abs(filePath) + yamlData, err := ioutil.ReadFile(filePathAbs) + if err != nil { + panic(err) + } + return yamlData +} + +func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { + err := yaml.Unmarshal([]byte(data), &p) + if err != nil { + log.Fatalf("Error %s", err) + } +} + +func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) { + // read yaml permissions file + yamlData := ReadPermissionsFile(Auth.PermissionsFile) + + // get group permissions + var p GroupPermissions + p.CreatePermissionsStruct(yamlData) + + // check permissions for all groups and index, and return most permissive + return p.ResolvePermissions(groups, index) +} + +func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) { + + // get union of groups the user is part of obtained from identity provider and groups in permissions file + var groupMatch []Permissions + for _, group := range groups { + for i := range p.Permissions { + if group["id"] == p.Permissions[i].GroupId { + groupMatch = append(groupMatch, p.Permissions[i]) + } + } + } + + if len(groupMatch) == 0 { + return "", fmt.Errorf("User is NOT allowed access to FeatureBase") + } + + // check that user's groups have access to the index user want to access + var indexMatch []Permissions + indexCheck := map[string]bool{} + for _, g := range groupMatch { + for _, idx := range index { + if idx == g.Index { + indexMatch = append(indexMatch, g) + indexCheck[idx] = true + } + } + } + + // check that user has access to every index + indexCount := 0 + for _, value := range indexCheck { + if value { + indexCount += 1 + } + } + + if indexCount != len(index) { + return "", fmt.Errorf("User is not allowed access to index: %s", index) + } + + // check permissions for index user has access to + allPermissions := map[string]bool{ + "admin": false, + "write": false, + "read": false, + } + + for _, g := range indexMatch { + if !allPermissions[g.Permission] { + allPermissions[g.Permission] = true + } + } + + if allPermissions["admin"] { + return "admin", error(nil) + } else if allPermissions["write"] { + return "write", error(nil) + } else if allPermissions["read"] { + return "read", error(nil) + } else { + return "", fmt.Errorf("No permissions found") + } } diff --git a/auth/auth_test.go b/auth/auth_test.go new file mode 100644 index 000000000..ddd081e80 --- /dev/null +++ b/auth/auth_test.go @@ -0,0 +1,231 @@ +// 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_test + +import ( + "fmt" + "reflect" + "strings" + "testing" + + "github.com/molecula/featurebase/v2/auth" +) + +func createStruct(inputs [][]string) (permissions auth.GroupPermissions) { + var sliceStruct []auth.Permissions + for _, i := range inputs { + groupId := i[0] + index := i[1] + permission := i[2] + p := auth.Permissions{groupId, index, permission} + sliceStruct = append(sliceStruct, p) + } + permissions = auth.GroupPermissions{Permissions: sliceStruct} + return permissions +} + +func TestAuth_CreatePermissionsStruct(t *testing.T) { + var singleInput = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read"`) + + var emptyInput = []byte(`group_permissions: + - group: + groupId: "" + index: "" + permission: ""`) + + var multiInput = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + var slice1 [][]string + var slice2 [][]string + var slice3 [][]string + var subslice1 []string + var subslice2 []string + var subslice3 []string + subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read") + subslice2 = append(subslice2, "", "", "") + subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin") + slice1 = append(slice1, subslice1) + slice2 = append(slice2, subslice2) + slice3 = append(slice3, subslice1, subslice3) + singleStruct := createStruct(slice1) + emptyStruct := createStruct(slice2) + multiStruct := createStruct(slice3) + + tests := []struct { + input []byte + output auth.GroupPermissions + }{ + {singleInput, singleStruct}, + {emptyInput, emptyStruct}, + {multiInput, multiStruct}, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + var p auth.GroupPermissions + p.CreatePermissionsStruct(test.input) + + if !reflect.DeepEqual(p, test.output) { + t.Fatalf("Expected output %s, but got %s", test.output, p) + } + }, + ) + } +} + +func createGroupMaps(groups []string) []map[string]string { + + var group1 []map[string]string + for _, i := range groups { + map1 := map[string]string{} + map1["id"] = i + group1 = append(group1, map1) + } + return group1 +} + +func TestAuth_ResolvePermissions(t *testing.T) { + // initializes different example of permissions file in yaml + var permissions1 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "read"`) + + var permissions2 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "write"`) + + var permissions3 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "read" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + var permissions4 = []byte(`group_permissions: + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee55906b" + index: "test" + permission: "" + - group: + groupId: "dca35310-ecda-4f23-86cd-876aee559900" + index: "test" + permission: "admin"`) + + // initializes groups that are returned from identity provider + groupsList1 := []string{} + groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"} + groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"} + + tests := []struct { + permissions []byte + groups []map[string]string + index []string + userAccess string + err string + }{ + { + permissions1, + createGroupMaps(groupsList1), + []string{"test"}, + "", + "User is NOT allowed access to FeatureBase", + }, + { + permissions1, + createGroupMaps(groupsList2), + []string{"test1"}, + "", + "User is not allowed access to index", + }, + { + permissions1, + createGroupMaps(groupsList2), + []string{"test"}, + "read", + "", + }, + { + permissions2, + createGroupMaps(groupsList3), + []string{"test"}, + "write", + "", + }, + { + permissions3, + createGroupMaps(groupsList3), + []string{"test"}, + "admin", + "", + }, + { + permissions2, + createGroupMaps(groupsList3), + []string{"test"}, + "write", + "", + }, + { + permissions4, + createGroupMaps(groupsList2), + []string{"test"}, + "", + "No permissions found", + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + var p auth.GroupPermissions + p.CreatePermissionsStruct(test.permissions) + + p1, err := p.ResolvePermissions(test.groups, test.index) + + if p1 != test.userAccess { + t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1) + } + + if err != nil { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error()) + } + } + + }) + } +} diff --git a/ctl/server.go b/ctl/server.go index c5d43a937..06f7d1bb2 100644 --- a/ctl/server.go +++ b/ctl/server.go @@ -130,5 +130,5 @@ func BuildServerFlags(cmd *cobra.Command, srv *server.Command) { 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.") - + flags.StringVar(&srv.Config.Auth.PermissionsFile, "auth.permissions", srv.Config.Auth.PermissionsFile, "Permissions' file with group authorization.") } diff --git a/install/featurebase.conf b/install/featurebase.conf index 540a410f4..5b1e73345 100644 --- a/install/featurebase.conf +++ b/install/featurebase.conf @@ -380,4 +380,5 @@ log-path = "/var/log/molecula/featurebase.log" # authorize-url = "" # token-url = "" # group-endpoint-url = "" -# scope-url = "" \ No newline at end of file +# scope-url = "" +# permissions = "" \ No newline at end of file diff --git a/server/config.go b/server/config.go index 0c1989c7a..6f74d4f25 100644 --- a/server/config.go +++ b/server/config.go @@ -620,6 +620,7 @@ func (c *Config) ValidateAuth() ([]error, error) { "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, "ScopeURL": c.Auth.ScopeURL, + "PermissionsFile": c.Auth.PermissionsFile, } errors := make([]error, 0) @@ -636,6 +637,15 @@ func (c *Config) ValidateAuth() ([]error, error) { continue } } + + if strings.Contains(name, "File") { + yamlData := auth.ReadPermissionsFile(value) + var p auth.GroupPermissions + p.CreatePermissionsStruct(yamlData) + if len(p.Permissions) == 0 { + errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value)) + } + } } if len(errors) > 0 { return errors, fmt.Errorf("there were errors validating config") From 9e2cf81127a32e918453e920582ee02afa4984bc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 10:35:56 -0600 Subject: [PATCH 02/10] added unit tests --- server/config.go | 35 +++++++++++++++++-------- server/config_internal_test.go | 47 +++++++++++++++++++++++++++++----- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/server/config.go b/server/config.go index 52f798abf..1f3752577 100644 --- a/server/config.go +++ b/server/config.go @@ -7,6 +7,7 @@ import ( "log" "net" "net/url" + "path/filepath" "runtime" "strconv" "strings" @@ -613,37 +614,51 @@ func (c *Config) ValidateAuth() ([]error, error) { errors := make([]error, 0) for name, value := range authConfig { if value == "" { - errors = append(errors, 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 { - errors = append(errors, 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 } } if strings.Contains(name, "File") { - yamlData := auth.ReadPermissionsFile(value) - var p auth.GroupPermissions - p.CreatePermissionsStruct(yamlData) - if len(p.Permissions) == 0 { - errors = append(errors, fmt.Errorf("No group permissions found in permissions file: %s", value)) + fileExt := filepath.Ext(value) + if (fileExt != ".yaml") && (fileExt != ".yml") { + errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value)) + continue } } } + if len(errors) > 0 { - return errors, fmt.Errorf("there were errors validating config") + return errors, fmt.Errorf("There were errors validating config") } return errors, nil } +func (c *Config) ValidatePermissions() (err error) { + + yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile) + var p auth.GroupPermissions + p.CreatePermissionsStruct(yamlData) + if len(p.Permissions) == 0 { + return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile) + } + return nil +} + func (c *Config) MustValidateAuth() { if errors, err := c.ValidateAuth(); err != nil { - for _, e := range errors { - log.Println(e) + for _, e1 := range errors { + log.Println(e1) + } + if e2 := c.ValidatePermissions(); e2 != nil { + log.Println(e2) } log.Fatal(err) } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 7c762b23e..8003a1f2f 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -279,12 +279,15 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } func TestConfig_validateAuth(t *testing.T) { - errorMesgEmpty := "empty string" - errorMesgURL := "invalid URL" + errorMesgEmpty := "Empty string" + errorMesgURL := "Invalid URL" + errorMesgPermissions := "Invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" - notValidURL := "not-a-url" + validFilename := "permissions.yaml" + invalidFilename := "permissions.txt" + invalidURL := "not-a-url" emptyString := "" enable := true disable := false @@ -303,6 +306,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -312,6 +316,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -322,6 +327,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -331,6 +337,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -341,6 +348,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -350,6 +358,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -359,6 +368,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -368,6 +378,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -376,6 +387,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -385,6 +397,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -392,6 +405,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgEmpty, errorMesgEmpty, + errorMesgEmpty, }, auth.Auth{ Enable: enable, @@ -401,6 +415,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, { @@ -412,10 +427,11 @@ func TestConfig_validateAuth(t *testing.T) { Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, - AuthorizeURL: notValidURL, + AuthorizeURL: invalidURL, TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, }, }, { @@ -429,9 +445,26 @@ func TestConfig_validateAuth(t *testing.T) { ClientId: validClientID, ClientSecret: validClientSecret, AuthorizeURL: validTestURL, - TokenURL: notValidURL, - GroupEndpointURL: notValidURL, + TokenURL: invalidURL, + GroupEndpointURL: invalidURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, + }, + }, + { + // Auth enabled, permissions file is set to invalid string + []string{ + errorMesgPermissions, + }, + auth.Auth{ + Enable: enable, + ClientId: validClientID, + ClientSecret: validClientSecret, + AuthorizeURL: validTestURL, + TokenURL: validTestURL, + GroupEndpointURL: validTestURL, + ScopeURL: validTestURL, + PermissionsFile: invalidFilename, }, }, { @@ -445,6 +478,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, + PermissionsFile: validFilename, }, }, { @@ -458,6 +492,7 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, + PermissionsFile: emptyString, }, }, } From 32228bb1344a0cc55aa33fdbb1d7ea74cdd259d0 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 10:48:40 -0600 Subject: [PATCH 03/10] updated go.mod --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index add6082e3..b66980688 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( golang.org/x/net v0.0.0-20210805182204-aaa1db679c0d // indirect golang.org/x/sync v0.0.0-20210220032951-036812b2e83c google.golang.org/grpc v1.28.0 - gopkg.in/yaml.v2 v2.3.0 // indirect + gopkg.in/yaml.v2 v2.3.0 modernc.org/mathutil v1.0.0 modernc.org/strutil v1.0.0 sigs.k8s.io/yaml v1.2.0 // indirect From 3c8a7384baae950bffc54ea8c52241ea279c96aa Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 13:05:35 -0600 Subject: [PATCH 04/10] added reviewer's suggestions --- auth/auth.go | 35 ++++++++++----------- auth/auth_test.go | 56 +++++++++++++--------------------- server/config.go | 10 +++--- server/config_internal_test.go | 6 ++-- 4 files changed, 46 insertions(+), 61 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 0d19f9461..7376e2551 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -56,9 +56,9 @@ func ReadPermissionsFile(filePath string) (yamlData []byte) { } func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { - err := yaml.Unmarshal([]byte(data), &p) + err := yaml.Unmarshal([]byte(data), p) if err != nil { - log.Fatalf("Error %s", err) + log.Fatalf("error %s", err) } } @@ -87,7 +87,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } if len(groupMatch) == 0 { - return "", fmt.Errorf("User is NOT allowed access to FeatureBase") + return "", fmt.Errorf("user is NOT allowed access to FeatureBase") } // check that user's groups have access to the index user want to access @@ -102,16 +102,15 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } } - // check that user has access to every index - indexCount := 0 - for _, value := range indexCheck { - if value { - indexCount += 1 + // check which index user does NOT have access to, and return in error mesg + if len(indexCheck) != len(index) { + var indexNotFound []string + for _, idx := range index { + if !indexCheck[idx] { + indexNotFound = append(indexNotFound, idx) + } } - } - - if indexCount != len(index) { - return "", fmt.Errorf("User is not allowed access to index: %s", index) + return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound) } // check permissions for index user has access to @@ -122,18 +121,16 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index } for _, g := range indexMatch { - if !allPermissions[g.Permission] { - allPermissions[g.Permission] = true - } + allPermissions[g.Permission] = true } if allPermissions["admin"] { - return "admin", error(nil) + return "admin", nil } else if allPermissions["write"] { - return "write", error(nil) + return "write", nil } else if allPermissions["read"] { - return "read", error(nil) + return "read", nil } else { - return "", fmt.Errorf("No permissions found") + return "", fmt.Errorf("no permissions found") } } diff --git a/auth/auth_test.go b/auth/auth_test.go index ddd081e80..e27463c59 100644 --- a/auth/auth_test.go +++ b/auth/auth_test.go @@ -22,19 +22,6 @@ import ( "github.com/molecula/featurebase/v2/auth" ) -func createStruct(inputs [][]string) (permissions auth.GroupPermissions) { - var sliceStruct []auth.Permissions - for _, i := range inputs { - groupId := i[0] - index := i[1] - permission := i[2] - p := auth.Permissions{groupId, index, permission} - sliceStruct = append(sliceStruct, p) - } - permissions = auth.GroupPermissions{Permissions: sliceStruct} - return permissions -} - func TestAuth_CreatePermissionsStruct(t *testing.T) { var singleInput = []byte(`group_permissions: - group: @@ -58,21 +45,22 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { index: "test" permission: "admin"`) - var slice1 [][]string - var slice2 [][]string - var slice3 [][]string - var subslice1 []string - var subslice2 []string - var subslice3 []string - subslice1 = append(subslice1, "dca35310-ecda-4f23-86cd-876aee55906b", "test", "read") - subslice2 = append(subslice2, "", "", "") - subslice3 = append(subslice3, "dca35310-ecda-4f23-86cd-876aee559900", "test", "admin") - slice1 = append(slice1, subslice1) - slice2 = append(slice2, subslice2) - slice3 = append(slice3, subslice1, subslice3) - singleStruct := createStruct(slice1) - emptyStruct := createStruct(slice2) - multiStruct := createStruct(slice3) + singleStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, + }, + } + emptyStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"", "", ""}, + }, + } + multiStruct := auth.GroupPermissions{ + Permissions: []auth.Permissions{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, + {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, + }, + } tests := []struct { input []byte @@ -89,7 +77,7 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { p.CreatePermissionsStruct(test.input) if !reflect.DeepEqual(p, test.output) { - t.Fatalf("Expected output %s, but got %s", test.output, p) + t.Fatalf("expected output %s, but got %s", test.output, p) } }, ) @@ -162,14 +150,14 @@ func TestAuth_ResolvePermissions(t *testing.T) { createGroupMaps(groupsList1), []string{"test"}, "", - "User is NOT allowed access to FeatureBase", + "user is NOT allowed access to FeatureBase", }, { permissions1, createGroupMaps(groupsList2), []string{"test1"}, "", - "User is not allowed access to index", + "user is not allowed access to index", }, { permissions1, @@ -204,7 +192,7 @@ func TestAuth_ResolvePermissions(t *testing.T) { createGroupMaps(groupsList2), []string{"test"}, "", - "No permissions found", + "no permissions found", }, } @@ -217,12 +205,12 @@ func TestAuth_ResolvePermissions(t *testing.T) { p1, err := p.ResolvePermissions(test.groups, test.index) if p1 != test.userAccess { - t.Errorf("Expected permission to be %s, but got %s", test.userAccess, p1) + t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1) } if err != nil { if !strings.Contains(err.Error(), test.err) { - t.Errorf("Expected error to contain %s, but got %s", test.err, err.Error()) + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) } } diff --git a/server/config.go b/server/config.go index 1f3752577..595697067 100644 --- a/server/config.go +++ b/server/config.go @@ -614,14 +614,14 @@ func (c *Config) ValidateAuth() ([]error, error) { errors := make([]error, 0) for name, value := range authConfig { if value == "" { - errors = append(errors, 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 { - errors = append(errors, 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 } } @@ -629,14 +629,14 @@ func (c *Config) ValidateAuth() ([]error, error) { if strings.Contains(name, "File") { fileExt := filepath.Ext(value) if (fileExt != ".yaml") && (fileExt != ".yml") { - errors = append(errors, fmt.Errorf("Invalid file extension for auth config %s: %s", name, value)) + errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value)) continue } } } if len(errors) > 0 { - return errors, fmt.Errorf("There were errors validating config") + return errors, fmt.Errorf("there were errors validating config") } return errors, nil } @@ -647,7 +647,7 @@ func (c *Config) ValidatePermissions() (err error) { var p auth.GroupPermissions p.CreatePermissionsStruct(yamlData) if len(p.Permissions) == 0 { - return fmt.Errorf("No group permissions found in permissions file: %s", c.Auth.PermissionsFile) + return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) } return nil } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 8003a1f2f..7f387172e 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -279,9 +279,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { } func TestConfig_validateAuth(t *testing.T) { - errorMesgEmpty := "Empty string" - errorMesgURL := "Invalid URL" - errorMesgPermissions := "Invalid file extension" + errorMesgEmpty := "empty string" + errorMesgURL := "invalid URL" + errorMesgPermissions := "invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" From 48913caafdbb3a8ae37ed356eceb29831732be26 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Wed, 15 Dec 2021 13:35:30 -0600 Subject: [PATCH 05/10] renamed package to authz, inmplemented reviewer's feedback --- auth/auth.go => authz/authz.go | 61 ++++++------ auth/auth_test.go => authz/authz_test.go | 112 +++++++++++------------ server/config.go | 20 +++- server/config_internal_test.go | 26 +++--- server/server.go | 14 +++ 5 files changed, 128 insertions(+), 105 deletions(-) rename auth/auth.go => authz/authz.go (62%) rename auth/auth_test.go => authz/authz_test.go (61%) diff --git a/auth/auth.go b/authz/authz.go similarity index 62% rename from auth/auth.go rename to authz/authz.go index 7376e2551..f48943c04 100644 --- a/auth/auth.go +++ b/authz/authz.go @@ -1,11 +1,23 @@ -// Copyright 2021 Molecula Corp. All rights reserved. -package auth +// 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 authz import ( "fmt" + "io" "io/ioutil" - "log" - "path/filepath" "gopkg.in/yaml.v2" ) @@ -46,48 +58,39 @@ type Permissions struct { Permission string `yaml:"permission"` } -func ReadPermissionsFile(filePath string) (yamlData []byte) { - filePathAbs, _ := filepath.Abs(filePath) - yamlData, err := ioutil.ReadFile(filePathAbs) +type Group struct { + ID string `json:"id"` + Name string `json:"displayName"` +} + +func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) { + permsData, err := ioutil.ReadAll(permsFile) if err != nil { - panic(err) + return fmt.Errorf("reading permissions failed with error: %s", err) } - return yamlData -} -func (p *GroupPermissions) CreatePermissionsStruct(data []byte) { - err := yaml.Unmarshal([]byte(data), p) + err = yaml.Unmarshal(permsData, p) if err != nil { - log.Fatalf("error %s", err) + return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } + + return nil } -func GetPermissions(Auth *Auth, groups []map[string]string, index []string) (permission string, err error) { - // read yaml permissions file - yamlData := ReadPermissionsFile(Auth.PermissionsFile) - - // get group permissions - var p GroupPermissions - p.CreatePermissionsStruct(yamlData) - - // check permissions for all groups and index, and return most permissive - return p.ResolvePermissions(groups, index) -} - -func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index []string) (permission string, err error) { +func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) { // get union of groups the user is part of obtained from identity provider and groups in permissions file var groupMatch []Permissions for _, group := range groups { for i := range p.Permissions { - if group["id"] == p.Permissions[i].GroupId { + if group.ID == p.Permissions[i].GroupId { groupMatch = append(groupMatch, p.Permissions[i]) } } } if len(groupMatch) == 0 { - return "", fmt.Errorf("user is NOT allowed access to FeatureBase") + return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups) } // check that user's groups have access to the index user want to access @@ -110,7 +113,7 @@ func (p *GroupPermissions) ResolvePermissions(groups []map[string]string, index indexNotFound = append(indexNotFound, idx) } } - return "", fmt.Errorf("user is not allowed access to index: %s", indexNotFound) + return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound) } // check permissions for index user has access to diff --git a/auth/auth_test.go b/authz/authz_test.go similarity index 61% rename from auth/auth_test.go rename to authz/authz_test.go index e27463c59..eaec5df13 100644 --- a/auth/auth_test.go +++ b/authz/authz_test.go @@ -11,7 +11,7 @@ // 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_test +package authz_test import ( "fmt" @@ -19,23 +19,23 @@ import ( "strings" "testing" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" ) -func TestAuth_CreatePermissionsStruct(t *testing.T) { - var singleInput = []byte(`group_permissions: +func TestAuth_ReadPermissionsFile(t *testing.T) { + var singleInput = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" - permission: "read"`) + permission: "read"` - var emptyInput = []byte(`group_permissions: + var emptyInput = `group_permissions: - group: groupId: "" index: "" - permission: ""`) + permission: ""` - var multiInput = []byte(`group_permissions: + var multiInput = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" @@ -43,28 +43,28 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` - singleStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + singleStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, }, } - emptyStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + emptyStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"", "", ""}, }, } - multiStruct := auth.GroupPermissions{ - Permissions: []auth.Permissions{ + multiStruct := authz.GroupPermissions{ + Permissions: []authz.Permissions{ {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, }, } tests := []struct { - input []byte - output auth.GroupPermissions + input string + output authz.GroupPermissions }{ {singleInput, singleStruct}, {emptyInput, emptyStruct}, @@ -73,8 +73,11 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - var p auth.GroupPermissions - p.CreatePermissionsStruct(test.input) + permFile := strings.NewReader(test.input) + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permFile); err != nil { + t.Fatalf("readPermissionsFile error: %s", err) + } if !reflect.DeepEqual(p, test.output) { t.Fatalf("expected output %s, but got %s", test.output, p) @@ -84,26 +87,15 @@ func TestAuth_CreatePermissionsStruct(t *testing.T) { } } -func createGroupMaps(groups []string) []map[string]string { - - var group1 []map[string]string - for _, i := range groups { - map1 := map[string]string{} - map1["id"] = i - group1 = append(group1, map1) - } - return group1 -} - func TestAuth_ResolvePermissions(t *testing.T) { // initializes different example of permissions file in yaml - var permissions1 = []byte(`group_permissions: + var permissions1 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" - permission: "read"`) + permission: "read"` - var permissions2 = []byte(`group_permissions: + var permissions2 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" @@ -111,9 +103,9 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "write"`) + permission: "write"` - var permissions3 = []byte(`group_permissions: + var permissions3 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" @@ -121,9 +113,9 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` - var permissions4 = []byte(`group_permissions: + var permissions4 = `group_permissions: - group: groupId: "dca35310-ecda-4f23-86cd-876aee55906b" index: "test" @@ -131,65 +123,68 @@ func TestAuth_ResolvePermissions(t *testing.T) { - group: groupId: "dca35310-ecda-4f23-86cd-876aee559900" index: "test" - permission: "admin"`) + permission: "admin"` // initializes groups that are returned from identity provider - groupsList1 := []string{} - groupsList2 := []string{"dca35310-ecda-4f23-86cd-876aee55906b"} - groupsList3 := []string{"dca35310-ecda-4f23-86cd-876aee55906b", "dca35310-ecda-4f23-86cd-876aee559900"} + groupsList1 := []authz.Group{} + groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}} + groupsList3 := []authz.Group{ + {"dca35310-ecda-4f23-86cd-876aee55906b", "name"}, + {"dca35310-ecda-4f23-86cd-876aee559900", "name"}, + } tests := []struct { - permissions []byte - groups []map[string]string - index []string - userAccess string - err string + yamlData string + groups []authz.Group + index []string + userAccess string + err string }{ { permissions1, - createGroupMaps(groupsList1), + groupsList1, []string{"test"}, "", - "user is NOT allowed access to FeatureBase", + "NOT allowed access to FeatureBase", }, { permissions1, - createGroupMaps(groupsList2), + groupsList2, []string{"test1"}, "", - "user is not allowed access to index", + "NOT allowed access to index", }, { permissions1, - createGroupMaps(groupsList2), + groupsList2, []string{"test"}, "read", "", }, { permissions2, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "write", "", }, { permissions3, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "admin", "", }, { permissions2, - createGroupMaps(groupsList3), + groupsList3, []string{"test"}, "write", "", }, { permissions4, - createGroupMaps(groupsList2), + groupsList2, []string{"test"}, "", "no permissions found", @@ -199,10 +194,11 @@ func TestAuth_ResolvePermissions(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - var p auth.GroupPermissions - p.CreatePermissionsStruct(test.permissions) + permFile := strings.NewReader(test.yamlData) + var p authz.GroupPermissions + p.ReadPermissionsFile(permFile) - p1, err := p.ResolvePermissions(test.groups, test.index) + p1, err := p.GetPermissions(test.groups, test.index) if p1 != test.userAccess { t.Errorf("expected permission to be %s, but got %s", test.userAccess, p1) diff --git a/server/config.go b/server/config.go index 595697067..1f3b62b4a 100644 --- a/server/config.go +++ b/server/config.go @@ -7,13 +7,14 @@ import ( "log" "net" "net/url" + "os" "path/filepath" "runtime" "strconv" "strings" "time" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" petcd "github.com/molecula/featurebase/v2/etcd" rbfcfg "github.com/molecula/featurebase/v2/rbf/cfg" "github.com/molecula/featurebase/v2/storage" @@ -232,7 +233,7 @@ type Config struct { SchemaDetailsOn bool `toml:"schema-details-on"` // Enable AuthZ/AuthN - Auth auth.Auth `toml:"auth"` + Auth authz.Auth `toml:"auth"` } // Namespace returns the namespace to use based on the Future flag. @@ -642,13 +643,22 @@ func (c *Config) ValidateAuth() ([]error, error) { } func (c *Config) ValidatePermissions() (err error) { + permsFile, err := os.Open(c.Auth.PermissionsFile) + if err != nil { + return err + } + + var p *authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return err + } - yamlData := auth.ReadPermissionsFile(c.Auth.PermissionsFile) - var p auth.GroupPermissions - p.CreatePermissionsStruct(yamlData) if len(p.Permissions) == 0 { return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) } + + defer permsFile.Close() + return nil } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 7f387172e..4d6dcc71c 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -9,7 +9,7 @@ import ( "strings" "testing" - "github.com/molecula/featurebase/v2/auth" + "github.com/molecula/featurebase/v2/authz" ) type addrs struct{ bind, advertise string } @@ -294,7 +294,7 @@ func TestConfig_validateAuth(t *testing.T) { tests := []struct { expErrs []string - input auth.Auth + input authz.Auth }{ { @@ -308,7 +308,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: emptyString, ClientSecret: emptyString, @@ -329,7 +329,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: emptyString, @@ -350,7 +350,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: emptyString, ClientSecret: validClientSecret, @@ -370,7 +370,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -389,7 +389,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -407,7 +407,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -423,7 +423,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgURL, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -440,7 +440,7 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgURL, errorMesgURL, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -456,7 +456,7 @@ func TestConfig_validateAuth(t *testing.T) { []string{ errorMesgPermissions, }, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -470,7 +470,7 @@ func TestConfig_validateAuth(t *testing.T) { { // Auth enabled, all configs are set properly []string{}, - auth.Auth{ + authz.Auth{ Enable: enable, ClientId: validClientID, ClientSecret: validClientSecret, @@ -484,7 +484,7 @@ func TestConfig_validateAuth(t *testing.T) { { // Auth disabled, all configs are set to empty string []string{}, - auth.Auth{ + authz.Auth{ Enable: disable, ClientId: emptyString, ClientSecret: emptyString, diff --git a/server/server.go b/server/server.go index a6d0049ae..b876f3eb6 100644 --- a/server/server.go +++ b/server/server.go @@ -29,6 +29,7 @@ import ( "golang.org/x/sync/errgroup" pilosa "github.com/molecula/featurebase/v2" + "github.com/molecula/featurebase/v2/authz" "github.com/molecula/featurebase/v2/boltdb" "github.com/molecula/featurebase/v2/encoding/proto" petcd "github.com/molecula/featurebase/v2/etcd" @@ -224,6 +225,19 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable { m.Config.MustValidateAuth() + + // Read permissions file + permsFile, err := os.Open(m.Config.Auth.PermissionsFile) + if err != nil { + return err + } + + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return err + } + + defer permsFile.Close() } // Initialize server. From 484cbbcd0943bb5bc5d453d9248188904a90a5f2 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Wed, 15 Dec 2021 14:19:12 -0600 Subject: [PATCH 06/10] fixed func name --- authz/authz_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/authz_test.go b/authz/authz_test.go index eaec5df13..b1fbc4d6a 100644 --- a/authz/authz_test.go +++ b/authz/authz_test.go @@ -87,7 +87,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { } } -func TestAuth_ResolvePermissions(t *testing.T) { +func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml var permissions1 = `group_permissions: - group: From 2ca29e6018621b203462296f72be3675e7405ffb Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 11:45:35 -0600 Subject: [PATCH 07/10] addressed reviewer's comments and added more tests --- authz/authz.go | 77 ++++------- authz/authz_test.go | 157 +++++++++------------- server/config.go | 118 +++++++++++----- server/config_internal_test.go | 238 +++++++++++++-------------------- server/server.go | 7 +- 5 files changed, 268 insertions(+), 329 deletions(-) diff --git a/authz/authz.go b/authz/authz.go index f48943c04..42c3bba2f 100644 --- a/authz/authz.go +++ b/authz/authz.go @@ -49,27 +49,23 @@ type Auth struct { } type GroupPermissions struct { - Permissions []Permissions `yaml:"group_permissions"` -} - -type Permissions struct { - GroupId string `yaml:"groupId"` - Index string `yaml:"index"` - Permission string `yaml:"permission"` + Permissions map[string]map[string]string } type Group struct { - ID string `json:"id"` - Name string `json:"displayName"` + UserID string + GroupID string `json:"id"` + GroupName string `json:"displayName"` } func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) { permsData, err := ioutil.ReadAll(permsFile) + if err != nil { return fmt.Errorf("reading permissions failed with error: %s", err) } - err = yaml.Unmarshal(permsData, p) + err = yaml.UnmarshalStrict(permsData, &p.Permissions) if err != nil { return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } @@ -77,54 +73,33 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return nil } -func (p *GroupPermissions) GetPermissions(groups []Group, index []string) (permission string, err error) { +func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { - // get union of groups the user is part of obtained from identity provider and groups in permissions file - var groupMatch []Permissions - for _, group := range groups { - for i := range p.Permissions { - if group.ID == p.Permissions[i].GroupId { - groupMatch = append(groupMatch, p.Permissions[i]) - } - } - } - - if len(groupMatch) == 0 { - return "", fmt.Errorf("the user's groups %s are NOT allowed access to FeatureBase", groups) - } - - // check that user's groups have access to the index user want to access - var indexMatch []Permissions - indexCheck := map[string]bool{} - for _, g := range groupMatch { - for _, idx := range index { - if idx == g.Index { - indexMatch = append(indexMatch, g) - indexCheck[idx] = true - } - } - } - - // check which index user does NOT have access to, and return in error mesg - if len(indexCheck) != len(index) { - var indexNotFound []string - for _, idx := range index { - if !indexCheck[idx] { - indexNotFound = append(indexNotFound, idx) - } - } - return "", fmt.Errorf("user is NOT allowed access to index: %s", indexNotFound) - } - - // check permissions for index user has access to allPermissions := map[string]bool{ "admin": false, "write": false, "read": false, } - for _, g := range indexMatch { - allPermissions[g.Permission] = true + if len(groups) == 0 { + return "", fmt.Errorf("user is not part of any groups in identity provider") + } + + var groupsDenied []string + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + if perm, ok := p.Permissions[group.GroupID][index]; ok { + allPermissions[perm] = true + } else { + return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index) + } + } else { + groupsDenied = append(groupsDenied, group.GroupID) + } + } + + if len(groupsDenied) == len(groups) { + return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied) } if allPermissions["admin"] { diff --git a/authz/authz_test.go b/authz/authz_test.go index b1fbc4d6a..fcec43185 100644 --- a/authz/authz_test.go +++ b/authz/authz_test.go @@ -23,64 +23,43 @@ import ( ) func TestAuth_ReadPermissionsFile(t *testing.T) { - var singleInput = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read"` + singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read"` - var emptyInput = `group_permissions: - - group: - groupId: "" - index: "" - permission: ""` + multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +"dca35310-ecda-4f23-86cd-876aee559900": + "test": "admin"` - var multiInput = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` - - singleStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, - }, + singleStruct := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, } - emptyStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"", "", ""}, - }, - } - multiStruct := authz.GroupPermissions{ - Permissions: []authz.Permissions{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "test", "read"}, - {"dca35310-ecda-4f23-86cd-876aee559900", "test", "admin"}, - }, + + multiStruct := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + "dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"}, } tests := []struct { input string - output authz.GroupPermissions + output map[string]map[string]string }{ {singleInput, singleStruct}, - {emptyInput, emptyStruct}, {multiInput, multiStruct}, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { permFile := strings.NewReader(test.input) + var p authz.GroupPermissions - if err := p.ReadPermissionsFile(permFile); err != nil { + err := p.ReadPermissionsFile(permFile) + if err != nil { t.Fatalf("readPermissionsFile error: %s", err) } - if !reflect.DeepEqual(p, test.output) { - t.Fatalf("expected output %s, but got %s", test.output, p) + if !reflect.DeepEqual(p.Permissions, test.output) { + t.Fatalf("expected output %s, but got %s", test.output, p.Permissions) } }, ) @@ -89,103 +68,84 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml - var permissions1 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "read"` + permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read"` - var permissions2 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "write"` + permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": "write"` - var permissions3 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "read" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` + permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b": + "test": "write" + "test2": "read" +"dca35310-ecda-4f23-86cd-876aee559900": + "test": "admin"` - var permissions4 = `group_permissions: - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee55906b" - index: "test" - permission: "" - - group: - groupId: "dca35310-ecda-4f23-86cd-876aee559900" - index: "test" - permission: "admin"` + permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": ""` // initializes groups that are returned from identity provider + groupName := "name" + userId := "user-id" groupsList1 := []authz.Group{} - groupsList2 := []authz.Group{{"dca35310-ecda-4f23-86cd-876aee55906b", "name"}} + groupsList2 := []authz.Group{{userId, "fake-group", groupName}} groupsList3 := []authz.Group{ - {"dca35310-ecda-4f23-86cd-876aee55906b", "name"}, - {"dca35310-ecda-4f23-86cd-876aee559900", "name"}, + {userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName}, + {userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName}, } tests := []struct { yamlData string groups []authz.Group - index []string + index string userAccess string err string }{ { permissions1, groupsList1, - []string{"test"}, + "test", + "", + "user is not part of any groups in identity provider", + }, + { + permissions1, + groupsList3, + "test1", + "", + "NOT allowed access to index", + }, + { + permissions2, + groupsList2, + "test", "", "NOT allowed access to FeatureBase", }, { permissions1, - groupsList2, - []string{"test1"}, - "", - "NOT allowed access to index", - }, - { - permissions1, - groupsList2, - []string{"test"}, + groupsList3, + "test", "read", "", }, { permissions2, groupsList3, - []string{"test"}, + "test", "write", "", }, { permissions3, groupsList3, - []string{"test"}, + "test", "admin", "", }, - { - permissions2, - groupsList3, - []string{"test"}, - "write", - "", - }, { permissions4, - groupsList2, - []string{"test"}, + groupsList3, + "test", "", "no permissions found", }, @@ -195,8 +155,11 @@ func TestAuth_GetPermissions(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { permFile := strings.NewReader(test.yamlData) + var p authz.GroupPermissions - p.ReadPermissionsFile(permFile) + if err := p.ReadPermissionsFile(permFile); err != nil { + t.Errorf("Error: %s", err) + } p1, err := p.GetPermissions(test.groups, test.index) diff --git a/server/config.go b/server/config.go index 1f3b62b4a..866ffb3ff 100644 --- a/server/config.go +++ b/server/config.go @@ -4,6 +4,7 @@ package server import ( "context" "fmt" + "io" "log" "net" "net/url" @@ -598,9 +599,9 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin return addrs[0].String(), nil } -func (c *Config) ValidateAuth() ([]error, error) { +func (c *Config) ValidateAuth() (errors []error) { if !c.Auth.Enable { - return []error{}, nil + return errors } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, @@ -609,10 +610,8 @@ func (c *Config) ValidateAuth() ([]error, error) { "TokenURL": c.Auth.TokenURL, "GroupEndpointURL": c.Auth.GroupEndpointURL, "ScopeURL": c.Auth.ScopeURL, - "PermissionsFile": c.Auth.PermissionsFile, } - errors := make([]error, 0) for name, value := range authConfig { if value == "" { errors = append(errors, fmt.Errorf("empty string for auth config %s", name)) @@ -626,50 +625,99 @@ func (c *Config) ValidateAuth() ([]error, error) { continue } } + } - if strings.Contains(name, "File") { - fileExt := filepath.Ext(value) - if (fileExt != ".yaml") && (fileExt != ".yml") { - errors = append(errors, fmt.Errorf("invalid file extension for auth config %s: %s", name, value)) + if len(errors) > 0 { + return errors + } + return nil +} + +func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { + + var p authz.GroupPermissions + if err := p.ReadPermissionsFile(permsFile); err != nil { + return append(errors, err) + } + + if len(p.Permissions) == 0 { + return append(errors, fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile)) + } + + for groupId, indexPerm := range p.Permissions { + if groupId == "" { + errors = append(errors, fmt.Errorf("empty string for group id in permissions file %s", c.Auth.PermissionsFile)) + continue + } + + for index, perm := range indexPerm { + if index == "" { + errors = append(errors, fmt.Errorf("empty string for index for group id %s in permissions file %s ", groupId, c.Auth.PermissionsFile)) + continue + } + + if perm == "" { + errors = append(errors, fmt.Errorf("empty string for permission for group id %s and index %s in permissions file %s", groupId, index, c.Auth.PermissionsFile)) + continue + } + + if !((perm == "admin") || (perm == "write") || (perm == "read")) { + errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile)) continue } } } - if len(errors) > 0 { - return errors, fmt.Errorf("there were errors validating config") + return errors } - return errors, nil -} - -func (c *Config) ValidatePermissions() (err error) { - permsFile, err := os.Open(c.Auth.PermissionsFile) - if err != nil { - return err - } - - var p *authz.GroupPermissions - if err := p.ReadPermissionsFile(permsFile); err != nil { - return err - } - - if len(p.Permissions) == 0 { - return fmt.Errorf("no group permissions found in permissions file: %s", c.Auth.PermissionsFile) - } - - defer permsFile.Close() return nil } +func (c *Config) ValidatePermissionsFile() (err error) { + + if c.Auth.PermissionsFile == "" { + return fmt.Errorf("empty string for auth config permissions file") + } + + fileExt := filepath.Ext(c.Auth.PermissionsFile) + if (fileExt != ".yaml") && (fileExt != ".yml") { + return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile) + } + return nil +} + func (c *Config) MustValidateAuth() { - if errors, err := c.ValidateAuth(); err != nil { - for _, e1 := range errors { - log.Println(e1) + + errorsAuth := c.ValidateAuth() + if len(errorsAuth) > 0 { + for _, e := range errorsAuth { + log.Println(e) } - if e2 := c.ValidatePermissions(); e2 != nil { - log.Println(e2) + } + + var errorsPerm []error + errorsPermFile := c.ValidatePermissionsFile() + if errorsPermFile == nil { + permsFile, err := os.Open(c.Auth.PermissionsFile) + if err != nil { + log.Println(err) } - log.Fatal(err) + + defer permsFile.Close() + + errorsPerm = c.ValidatePermissions(permsFile) + if len(errorsPerm) > 0 { + for _, e := range errorsPerm { + log.Println(e) + } + } + + } else { + log.Println(errorsPermFile) + } + + if len(errorsAuth) > 0 || len(errorsPerm) > 0 || errorsPermFile != nil { + log.Fatal(fmt.Errorf("there were errors validating authN/authZ config and/or permissions")) } } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 4d6dcc71c..48ba081db 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -281,12 +281,9 @@ func TestConfig_validateAddrsGRPC(t *testing.T) { func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty := "empty string" errorMesgURL := "invalid URL" - errorMesgPermissions := "invalid file extension" validTestURL := "https://url.com/" validClientID := "clientid" validClientSecret := "clientSecret" - validFilename := "permissions.yaml" - invalidFilename := "permissions.txt" invalidURL := "not-a-url" emptyString := "" enable := true @@ -306,7 +303,6 @@ func TestConfig_validateAuth(t *testing.T) { errorMesgEmpty, errorMesgEmpty, errorMesgEmpty, - errorMesgEmpty, }, authz.Auth{ Enable: enable, @@ -316,106 +312,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: emptyString, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: emptyString, - ClientSecret: validClientSecret, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: emptyString, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: emptyString, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, - }, - }, - { - // Auth enabled, some configs are set to empty string - []string{ - errorMesgEmpty, - errorMesgEmpty, - errorMesgEmpty, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: emptyString, - ScopeURL: emptyString, - PermissionsFile: emptyString, }, }, { @@ -431,40 +327,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, - PermissionsFile: validFilename, - }, - }, - { - // Auth enabled, some strings are set to invalid URL - []string{ - errorMesgURL, - errorMesgURL, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: invalidURL, - GroupEndpointURL: invalidURL, - ScopeURL: validTestURL, - PermissionsFile: validFilename, - }, - }, - { - // Auth enabled, permissions file is set to invalid string - []string{ - errorMesgPermissions, - }, - authz.Auth{ - Enable: enable, - ClientId: validClientID, - ClientSecret: validClientSecret, - AuthorizeURL: validTestURL, - TokenURL: validTestURL, - GroupEndpointURL: validTestURL, - ScopeURL: validTestURL, - PermissionsFile: invalidFilename, }, }, { @@ -478,7 +340,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: validTestURL, GroupEndpointURL: validTestURL, ScopeURL: validTestURL, - PermissionsFile: validFilename, }, }, { @@ -492,7 +353,6 @@ func TestConfig_validateAuth(t *testing.T) { TokenURL: emptyString, GroupEndpointURL: emptyString, ScopeURL: emptyString, - PermissionsFile: emptyString, }, }, } @@ -502,9 +362,9 @@ func TestConfig_validateAuth(t *testing.T) { c := NewConfig() c.Auth = test.input - errors, err := c.ValidateAuth() + errors := c.ValidateAuth() if len(test.expErrs) > 0 { - if err == nil { + if errors == nil { t.Fatal("expected errors, but none were found") } } @@ -522,3 +382,97 @@ func TestConfig_validateAuth(t *testing.T) { }) } } + +func TestConfig_validatePermissions(t *testing.T) { + permissions0 := `` + + permissions1 := `"": + "test": "read"` + + permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": + "": "write"` + + permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": ""` + + permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": + "test": "readwrite"` + + tests := []struct { + err string + input string + }{ + { + "no group permissions found in permissions file", + permissions0, + }, + { + "empty string for group id", + permissions1, + }, + { + "empty string for index", + permissions2, + }, + { + "empty string for permission", + permissions3, + }, + { + "not a valid permission", + permissions4, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + c := NewConfig() + c.Auth.PermissionsFile = "test.yaml" + + permFile := strings.NewReader(test.input) + errors := c.ValidatePermissions(permFile) + + if errors == nil { + t.Fatal("expected errors, but none were found") + } + + for _, err := range errors { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) + + } + } + }) + } +} + +func TestConfig_validatePermissionsFilename(t *testing.T) { + + tests := []struct { + err string + input string + }{ + { + "empty string for auth config permissions file", + "", + }, + { + "invalid file extension for auth config permissions file", + "permissions.txt", + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + c := NewConfig() + c.Auth.PermissionsFile = test.input + + if err := c.ValidatePermissionsFile(); err != nil { + if !strings.Contains(err.Error(), test.err) { + t.Errorf("expected error to contain %s, but got %s", test.err, err.Error()) + } + } + }) + } +} diff --git a/server/server.go b/server/server.go index b876f3eb6..c2418630b 100644 --- a/server/server.go +++ b/server/server.go @@ -226,18 +226,17 @@ func (m *Command) Start() (err error) { if m.Config.Auth.Enable { m.Config.MustValidateAuth() - // Read permissions file permsFile, err := os.Open(m.Config.Auth.PermissionsFile) if err != nil { return err } + defer permsFile.Close() + var p authz.GroupPermissions - if err := p.ReadPermissionsFile(permsFile); err != nil { + if err = p.ReadPermissionsFile(permsFile); err != nil { return err } - - defer permsFile.Close() } // Initialize server. From f05f1d0de27bae969d31d4364b7bedd85e007593 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 15:51:30 -0600 Subject: [PATCH 08/10] added more authz functionality --- authz/{authz.go => authorization.go} | 31 ++++++- .../{authz_test.go => authorization_test.go} | 93 ++++++++++++++++++- server/server.go | 31 +++++++ 3 files changed, 151 insertions(+), 4 deletions(-) rename authz/{authz.go => authorization.go} (75%) rename authz/{authz_test.go => authorization_test.go} (68%) diff --git a/authz/authz.go b/authz/authorization.go similarity index 75% rename from authz/authz.go rename to authz/authorization.go index 42c3bba2f..7f311899d 100644 --- a/authz/authz.go +++ b/authz/authorization.go @@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss if perm, ok := p.Permissions[group.GroupID][index]; ok { allPermissions[perm] = true } else { - return "", fmt.Errorf("User %s is NOT allowed access to index %s", group.UserID, index) + return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index) } } else { groupsDenied = append(groupsDenied, group.GroupID) @@ -99,7 +99,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss } if len(groupsDenied) == len(groups) { - return "", fmt.Errorf("group(s) %s are NOT allowed access to FeatureBase", groupsDenied) + return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied) } if allPermissions["admin"] { @@ -112,3 +112,30 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss return "", fmt.Errorf("no permissions found") } } + +func (p *GroupPermissions) IsAdmin(groups []Group) bool { + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + for _, permission := range p.Permissions[group.GroupID] { + if permission == "admin" { + return true + } + } + } + } + return false +} + +func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) { + + for _, group := range groups { + if _, ok := p.Permissions[group.GroupID]; ok { + for index, permission := range p.Permissions[group.GroupID] { + if permission == desiredPermission { + indexList = append(indexList, index) + } + } + } + } + return indexList +} diff --git a/authz/authz_test.go b/authz/authorization_test.go similarity index 68% rename from authz/authz_test.go rename to authz/authorization_test.go index fcec43185..84db21ab5 100644 --- a/authz/authz_test.go +++ b/authz/authorization_test.go @@ -23,6 +23,7 @@ import ( ) func TestAuth_ReadPermissionsFile(t *testing.T) { + singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": "test": "read"` @@ -67,6 +68,7 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { } func TestAuth_GetPermissions(t *testing.T) { + // initializes different example of permissions file in yaml permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": "test": "read"` @@ -112,14 +114,14 @@ func TestAuth_GetPermissions(t *testing.T) { groupsList3, "test1", "", - "NOT allowed access to index", + "does not have permission to index", }, { permissions2, groupsList2, "test", "", - "NOT allowed access to FeatureBase", + "does not have permission to FeatureBase", }, { permissions1, @@ -176,3 +178,90 @@ func TestAuth_GetPermissions(t *testing.T) { }) } } + +func TestAuth_IsAdmin(t *testing.T) { + + group := []authz.Group{ + {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + } + + groupPermissions1 := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"}, + } + + groupPermissions2 := map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + } + + tests := []struct { + groups []authz.Group + groupPermissions map[string]map[string]string + output bool + }{ + { + group, groupPermissions1, true, + }, + { + group, groupPermissions2, false, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + p := authz.GroupPermissions{test.groupPermissions} + resp := p.IsAdmin(test.groups) + if resp != test.output { + t.Errorf("expected %t, but got %t", test.output, resp) + } + }) + } +} + +func TestAuth_GetAuthorizedIndexList(t *testing.T) { + + group := []authz.Group{ + {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + } + + p := authz.GroupPermissions{map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": { + "test1": "admin", + "test2": "read", + "test3": "read", + }, + }} + + tests := []struct { + groups []authz.Group + permission string + output []string + }{ + { + group, + "read", + []string{"test2", "test3"}, + }, + { + group, + "admin", + []string{"test1"}, + }, + { + group, + "write", + nil, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + + indexList := p.GetAuthorizedIndexList(test.groups, test.permission) + + if !reflect.DeepEqual(indexList, test.output) { + t.Errorf("expected %s, but got %s", test.output, indexList) + } + }) + } + +} diff --git a/server/server.go b/server/server.go index c2418630b..17ea28933 100644 --- a/server/server.go +++ b/server/server.go @@ -237,6 +237,37 @@ func (m *Command) Start() (err error) { if err = p.ReadPermissionsFile(permsFile); err != nil { return err } + + groups := []authz.Group{ + { + UserID: "user-id", + GroupID: "dca35310-ecda-4f23-86cd-876aee55906b", + GroupName: "group-name", + }, + // { + // UserID: "user-id", + // GroupID: "dca35310-ecda-4f23-86cd-876aee559900", + // GroupName: "group-name", + // }, + } + + index := "test" + + perm, err := p.GetPermissions(groups, index) + fmt.Printf("\nuser has %s access to index %s\n", perm, index) + if err != nil { + fmt.Printf("\np: %s, err: %s\n", perm, err.Error()) + } + + adminAccess := p.IsAdmin(groups) + fmt.Printf("\nAdminAccess: %t\n", adminAccess) + + accessList := []string{"read", "write", "admin"} + for _, a := range accessList { + indexList := p.GetAuthorizedIndexList(groups, a) + fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList) + } + } // Initialize server. From a606bd030a09ac65cbc8cedba80533650f710260 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Fri, 17 Dec 2021 16:46:07 -0600 Subject: [PATCH 09/10] addressed reviewer's feedback --- authz/authorization.go | 8 ++++++-- authz/authorization_test.go | 8 +++++--- server/config.go | 16 ++++------------ server/server.go | 31 ------------------------------- 4 files changed, 15 insertions(+), 48 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index 7f311899d..f89a52ea6 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -70,7 +70,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } - return nil + return } func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { @@ -91,7 +91,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss if perm, ok := p.Permissions[group.GroupID][index]; ok { allPermissions[perm] = true } else { - return "", fmt.Errorf("User %s does not have permission to index %s", group.UserID, index) + return "", fmt.Errorf("user %s does not have permission to index %s", group.UserID, index) } } else { groupsDenied = append(groupsDenied, group.GroupID) @@ -133,6 +133,10 @@ func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermiss for index, permission := range p.Permissions[group.GroupID] { if permission == desiredPermission { indexList = append(indexList, index) + } else if permission == "admin" { + indexList = append(indexList, index) + } else if permission == "write" && desiredPermission == "read" { + indexList = append(indexList, index) } } } diff --git a/authz/authorization_test.go b/authz/authorization_test.go index 84db21ab5..cd985f973 100644 --- a/authz/authorization_test.go +++ b/authz/authorization_test.go @@ -16,6 +16,7 @@ package authz_test import ( "fmt" "reflect" + "sort" "strings" "testing" @@ -227,7 +228,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { "dca35310-ecda-4f23-86cd-876aee55906b": { "test1": "admin", "test2": "read", - "test3": "read", + "test3": "write", }, }} @@ -239,7 +240,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { { group, "read", - []string{"test2", "test3"}, + []string{"test1", "test2", "test3"}, }, { group, @@ -249,7 +250,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { { group, "write", - nil, + []string{"test1", "test3"}, }, } @@ -257,6 +258,7 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { indexList := p.GetAuthorizedIndexList(test.groups, test.permission) + sort.Strings(indexList) if !reflect.DeepEqual(indexList, test.output) { t.Errorf("expected %s, but got %s", test.output, indexList) diff --git a/server/config.go b/server/config.go index 866ffb3ff..5f4f0573c 100644 --- a/server/config.go +++ b/server/config.go @@ -601,7 +601,7 @@ func lookupAddr(ctx context.Context, resolver *net.Resolver, host string) (strin func (c *Config) ValidateAuth() (errors []error) { if !c.Auth.Enable { - return errors + return } authConfig := map[string]string{ "ClientId": c.Auth.ClientId, @@ -626,11 +626,7 @@ func (c *Config) ValidateAuth() (errors []error) { } } } - - if len(errors) > 0 { - return errors - } - return nil + return errors } func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { @@ -667,11 +663,7 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { } } } - if len(errors) > 0 { - return errors - } - - return nil + return errors } func (c *Config) ValidatePermissionsFile() (err error) { @@ -684,7 +676,7 @@ func (c *Config) ValidatePermissionsFile() (err error) { if (fileExt != ".yaml") && (fileExt != ".yml") { return fmt.Errorf("invalid file extension for auth config permissions file: %s", c.Auth.PermissionsFile) } - return nil + return } func (c *Config) MustValidateAuth() { diff --git a/server/server.go b/server/server.go index 17ea28933..c2418630b 100644 --- a/server/server.go +++ b/server/server.go @@ -237,37 +237,6 @@ func (m *Command) Start() (err error) { if err = p.ReadPermissionsFile(permsFile); err != nil { return err } - - groups := []authz.Group{ - { - UserID: "user-id", - GroupID: "dca35310-ecda-4f23-86cd-876aee55906b", - GroupName: "group-name", - }, - // { - // UserID: "user-id", - // GroupID: "dca35310-ecda-4f23-86cd-876aee559900", - // GroupName: "group-name", - // }, - } - - index := "test" - - perm, err := p.GetPermissions(groups, index) - fmt.Printf("\nuser has %s access to index %s\n", perm, index) - if err != nil { - fmt.Printf("\np: %s, err: %s\n", perm, err.Error()) - } - - adminAccess := p.IsAdmin(groups) - fmt.Printf("\nAdminAccess: %t\n", adminAccess) - - accessList := []string{"read", "write", "admin"} - for _, a := range accessList { - indexList := p.GetAuthorizedIndexList(groups, a) - fmt.Printf("\nPermission requested: %s, Index List: %s\n", a, indexList) - } - } // Initialize server. From c14bd08213477c3fc6bc355b24774c39be0bd4cc Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Sun, 19 Dec 2021 11:37:41 -0600 Subject: [PATCH 10/10] updated admin to be at the cluster level --- authz/authorization.go | 33 ++++--- authz/authorization_test.go | 153 +++++++++++++++++++++------------ server/config.go | 9 +- server/config_internal_test.go | 32 +++++-- 4 files changed, 149 insertions(+), 78 deletions(-) diff --git a/authz/authorization.go b/authz/authorization.go index f89a52ea6..77bd67ade 100644 --- a/authz/authorization.go +++ b/authz/authorization.go @@ -49,7 +49,8 @@ type Auth struct { } type GroupPermissions struct { - Permissions map[string]map[string]string + Permissions map[string]map[string]string `yaml:"user-groups"` + Admin string `yaml:"admin"` } type Group struct { @@ -65,7 +66,7 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) return fmt.Errorf("reading permissions failed with error: %s", err) } - err = yaml.UnmarshalStrict(permsData, &p.Permissions) + err = yaml.UnmarshalStrict(permsData, &p) if err != nil { return fmt.Errorf("unmarshalling permissions failed with error: %s", err) } @@ -75,8 +76,11 @@ func (p *GroupPermissions) ReadPermissionsFile(permsFile io.Reader) (err error) func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permission string, errors error) { + if admin := p.IsAdmin(groups); admin { + return "admin", nil + } + allPermissions := map[string]bool{ - "admin": false, "write": false, "read": false, } @@ -102,9 +106,7 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss return "", fmt.Errorf("group(s) %s does not have permission to FeatureBase", groupsDenied) } - if allPermissions["admin"] { - return "admin", nil - } else if allPermissions["write"] { + if allPermissions["write"] { return "write", nil } else if allPermissions["read"] { return "read", nil @@ -115,26 +117,29 @@ func (p *GroupPermissions) GetPermissions(groups []Group, index string) (permiss func (p *GroupPermissions) IsAdmin(groups []Group) bool { for _, group := range groups { - if _, ok := p.Permissions[group.GroupID]; ok { - for _, permission := range p.Permissions[group.GroupID] { - if permission == "admin" { - return true - } - } + if p.Admin == group.GroupID { + return true } } return false } func (p *GroupPermissions) GetAuthorizedIndexList(groups []Group, desiredPermission string) (indexList []string) { + // if user is admin, find all indexes in permissions file and return them + if admin := p.IsAdmin(groups); admin { + for groupId := range p.Permissions { + for index := range p.Permissions[groupId] { + indexList = append(indexList, index) + } + } + return indexList + } for _, group := range groups { if _, ok := p.Permissions[group.GroupID]; ok { for index, permission := range p.Permissions[group.GroupID] { if permission == desiredPermission { indexList = append(indexList, index) - } else if permission == "admin" { - indexList = append(indexList, index) } else if permission == "write" && desiredPermission == "read" { indexList = append(indexList, index) } diff --git a/authz/authorization_test.go b/authz/authorization_test.go index cd985f973..dab33dbe1 100644 --- a/authz/authorization_test.go +++ b/authz/authorization_test.go @@ -25,29 +25,39 @@ import ( func TestAuth_ReadPermissionsFile(t *testing.T) { - singleInput := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read"` + singleInput := `user-groups: + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - multiInput := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read" -"dca35310-ecda-4f23-86cd-876aee559900": - "test": "admin"` + multiInput := `user-groups: + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" + "test2": "write" + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - singleStruct := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + singlePermission := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + }, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } - multiStruct := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, - "dca35310-ecda-4f23-86cd-876aee559900": {"test": "admin"}, + multiPermission := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read", "test2": "write"}, + "dca35310-ecda-4f23-86cd-876aee559900": {"test": "write"}}, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } tests := []struct { input string - output map[string]map[string]string + output authz.GroupPermissions }{ - {singleInput, singleStruct}, - {multiInput, multiStruct}, + {singleInput, singlePermission}, + {multiInput, multiPermission}, } for i, test := range tests { @@ -60,8 +70,8 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { t.Fatalf("readPermissionsFile error: %s", err) } - if !reflect.DeepEqual(p.Permissions, test.output) { - t.Fatalf("expected output %s, but got %s", test.output, p.Permissions) + if !reflect.DeepEqual(p, test.output) { + t.Fatalf("expected output %s, but got %s", test.output, p) } }, ) @@ -71,20 +81,28 @@ func TestAuth_ReadPermissionsFile(t *testing.T) { func TestAuth_GetPermissions(t *testing.T) { // initializes different example of permissions file in yaml - permissions1 := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "read"` + permissions1 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": "write"` + permissions2 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions3 := `"dca35310-ecda-4f23-86cd-876aee55906b": - "test": "write" - "test2": "read" -"dca35310-ecda-4f23-86cd-876aee559900": - "test": "admin"` + permissions3 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee55906b": + "test": "write" + "test2": "read" + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": ""` + permissions4 := `"user-groups": + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` // initializes groups that are returned from identity provider groupName := "name" @@ -95,6 +113,7 @@ func TestAuth_GetPermissions(t *testing.T) { {userId, "dca35310-ecda-4f23-86cd-876aee55906b", groupName}, {userId, "dca35310-ecda-4f23-86cd-876aee559900", groupName}, } + groupsList4 := []authz.Group{{userId, "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", groupName}} tests := []struct { yamlData string @@ -140,7 +159,7 @@ func TestAuth_GetPermissions(t *testing.T) { }, { permissions3, - groupsList3, + groupsList4, "test", "admin", "", @@ -182,34 +201,37 @@ func TestAuth_GetPermissions(t *testing.T) { func TestAuth_IsAdmin(t *testing.T) { - group := []authz.Group{ - {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + group1 := []authz.Group{ + {"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"}, } - groupPermissions1 := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "admin"}, + group2 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, } - groupPermissions2 := map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "read"}, + groupPermissions := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": {"test": "write"}, + }, + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", } tests := []struct { groups []authz.Group - groupPermissions map[string]map[string]string + groupPermissions authz.GroupPermissions output bool }{ { - group, groupPermissions1, true, + group1, groupPermissions, true, }, { - group, groupPermissions2, false, + group2, groupPermissions, false, }, } for i, test := range tests { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - p := authz.GroupPermissions{test.groupPermissions} + p := test.groupPermissions resp := p.IsAdmin(test.groups) if resp != test.output { t.Errorf("expected %t, but got %t", test.output, resp) @@ -220,17 +242,30 @@ func TestAuth_IsAdmin(t *testing.T) { func TestAuth_GetAuthorizedIndexList(t *testing.T) { - group := []authz.Group{ - {"user-is", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, + group1 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee55906b", "group-name"}, } - p := authz.GroupPermissions{map[string]map[string]string{ - "dca35310-ecda-4f23-86cd-876aee55906b": { - "test1": "admin", - "test2": "read", - "test3": "write", + group2 := []authz.Group{ + {"admin-user-id", "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", "admin-group"}, + } + + group3 := []authz.Group{ + {"user-id", "dca35310-ecda-4f23-86cd-876aee559900", "group-name"}, + } + + p := authz.GroupPermissions{ + Permissions: map[string]map[string]string{ + "dca35310-ecda-4f23-86cd-876aee55906b": { + "test1": "read", + "test2": "write", + }, + "dca35310-ecda-4f23-86cd-876aee559900": { + "test3": "read", + }, }, - }} + Admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe", + } tests := []struct { groups []authz.Group @@ -238,19 +273,29 @@ func TestAuth_GetAuthorizedIndexList(t *testing.T) { output []string }{ { - group, + group1, + "read", + []string{"test1", "test2"}, + }, + { + group1, + "write", + []string{"test2"}, + }, + { + group3, + "write", + nil, + }, + { + group2, "read", []string{"test1", "test2", "test3"}, }, { - group, - "admin", - []string{"test1"}, - }, - { - group, + group2, "write", - []string{"test1", "test3"}, + []string{"test1", "test2", "test3"}, }, } diff --git a/server/config.go b/server/config.go index 5f4f0573c..807e19b7f 100644 --- a/server/config.go +++ b/server/config.go @@ -657,12 +657,17 @@ func (c *Config) ValidatePermissions(permsFile io.Reader) (errors []error) { continue } - if !((perm == "admin") || (perm == "write") || (perm == "read")) { - errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s", perm, groupId, index, c.Auth.PermissionsFile)) + if !((perm == "write") || (perm == "read")) { + errors = append(errors, fmt.Errorf("not a valid permission %s for group id %s and index %s in permissions file %s; expected permissions are read or write", perm, groupId, index, c.Auth.PermissionsFile)) continue } } } + + if p.Admin == "" { + errors = append(errors, fmt.Errorf("empty string for admin in permissions file: %s", c.Auth.PermissionsFile)) + } + return errors } diff --git a/server/config_internal_test.go b/server/config_internal_test.go index 48ba081db..11148dfad 100644 --- a/server/config_internal_test.go +++ b/server/config_internal_test.go @@ -386,17 +386,29 @@ func TestConfig_validateAuth(t *testing.T) { func TestConfig_validatePermissions(t *testing.T) { permissions0 := `` - permissions1 := `"": - "test": "read"` + permissions1 := `user-groups: + "": + "test": "read" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions2 := `"dca35310-ecda-4f23-86cd-876aee559900": - "": "write"` + permissions2 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "": "write" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions3 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": ""` + permissions3 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` - permissions4 := `"dca35310-ecda-4f23-86cd-876aee559900": - "test": "readwrite"` + permissions4 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "readwrite" +admin: "ac97c9e2-346b-42a2-b6da-18bcb61a32fe"` + + permissions5 := `user-groups: + "dca35310-ecda-4f23-86cd-876aee559900": + "test": "read"` tests := []struct { err string @@ -422,6 +434,10 @@ func TestConfig_validatePermissions(t *testing.T) { "not a valid permission", permissions4, }, + { + "empty string for admin in permissions file", + permissions5, + }, } for i, test := range tests {