From 3c8a7384baae950bffc54ea8c52241ea279c96aa Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Mon, 13 Dec 2021 13:05:35 -0600 Subject: [PATCH] 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"