addressed reviewer's comments and added more tests

This commit is contained in:
Souhaila Noor 2021-12-17 11:45:35 -06:00
parent 484cbbcd09
commit 2ca29e6018
5 changed files with 268 additions and 329 deletions

View file

@ -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"] {

View file

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

View file

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

View file

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

View file

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