From b380eef81b147fde92a88dafa2922a033cfde7b3 Mon Sep 17 00:00:00 2001 From: Mitja Zivkovic Date: Thu, 21 Feb 2019 23:58:17 +0100 Subject: [PATCH] upd(system): add permission validation --- Makefile | 2 +- internal/rules/main_test.go | 10 ++ internal/rules/resources.go | 50 +++----- internal/rules/resources_test.go | 116 +++++++++--------- system/service/main_test.go | 50 ++++++++ system/service/permission.go | 6 + system/service/permission_test.go | 50 ++++++-- system/service/permissions.go | 41 ------- system/service/validation.go | 110 +++++++++++++++++ ...permissions_test.go => validation_test.go} | 0 10 files changed, 292 insertions(+), 143 deletions(-) delete mode 100644 system/service/permissions.go create mode 100644 system/service/validation.go rename system/service/{permissions_test.go => validation_test.go} (100%) diff --git a/Makefile b/Makefile index bc31899e3..739ad1d8d 100644 --- a/Makefile +++ b/Makefile @@ -100,7 +100,7 @@ test.crm.db: $(GOTEST) $(GO) tool cover -func=.cover.out | grep --color "^\|[^0-9]0.0%" test.system: $(GOTEST) - $(GOTEST) -covermode count -coverprofile .cover.out -v ./system/repository/... + $(GOTEST) -covermode count -coverprofile .cover.out -v ./system/repository/... ./system/service/... $(GO) tool cover -func=.cover.out | grep --color "^\|[^0-9]0.0%" test.system.db: $(GOTEST) diff --git a/internal/rules/main_test.go b/internal/rules/main_test.go index a8c3162fa..e63e9f4dd 100644 --- a/internal/rules/main_test.go +++ b/internal/rules/main_test.go @@ -41,5 +41,15 @@ func TestMain(m *testing.M) { return } + // clean up tables + { + for _, name := range []string{"sys_user", "sys_role", "sys_role_member", "sys_organisation", "sys_rules"} { + _, err := db.Exec("truncate " + name) + if err != nil { + panic("Error when clearing " + name + ": " + err.Error()) + } + } + } + os.Exit(m.Run()) } diff --git a/internal/rules/resources.go b/internal/rules/resources.go index 54a03e9de..555a22a40 100644 --- a/internal/rules/resources.go +++ b/internal/rules/resources.go @@ -9,6 +9,10 @@ import ( "github.com/crusttech/crust/internal/auth" ) +const ( + delimiter = ":" +) + type resources struct { ctx context.Context db *factory.DB @@ -29,43 +33,27 @@ func (r *resources) identity() uint64 { return auth.GetIdentityFromContext(r.ctx).Identity() } +// IsAllowed function checks granted permission for specific resource and operation. Permission checks on +// global level are not allowed and will always return Deny. func (r *resources) IsAllowed(resource string, operation string) Access { - if strings.Contains(resource, "*") { - return r.checkAccessMulti(resource, operation) - } - return r.checkAccess(resource, operation) -} + parts := strings.Split(resource, delimiter) -func (r *resources) checkAccessMulti(resource string, operation string) Access { - user := r.identity() - result := []Access{} - query := []string{ - // select rules - "select r.value from sys_rules r", - // join members - "inner join sys_role_member m on (m.rel_role = r.rel_role and m.rel_user=?)", - // add conditions - "where r.resource LIKE ? and r.operation=?", - } - resource = strings.Replace(resource, "*", "%", -1) - queryString := strings.Join(query, " ") - if err := r.db.Select(&result, queryString, user, resource, operation); err != nil { - // @todo: log error + // Permission check on global level is not allowed. + if parts[len(parts)-1] == "*" { return Deny } - // order by deny, allow - for _, val := range result { - if val == Deny { - return Deny - } + access := r.checkAccess(resource, operation) + if access == Inherit { + // Create resource definition for global level. + parts[len(parts)-1] = "*" + + resource = strings.Join(parts, delimiter) + + // If rule for specific resource is not set we check on global level. + return r.checkAccess(resource, operation) } - for _, val := range result { - if val == Allow { - return Allow - } - } - return Inherit + return access } func (r *resources) checkAccess(resource string, operation string) Access { diff --git a/internal/rules/resources_test.go b/internal/rules/resources_test.go index bdf8808da..c3e7b06df 100644 --- a/internal/rules/resources_test.go +++ b/internal/rules/resources_test.go @@ -20,14 +20,11 @@ func TestRules(t *testing.T) { db := factory.Database.MustGet() + roleID := uint64(123456) + db.Insert("sys_user", user) - var i uint64 = 0 - for i < 5 { - db.Insert("sys_role", types.Role{ID: i, Name: fmt.Sprintf("Role %d", i)}) - i++ - } - db.Insert("sys_role_member", types.RoleMember{RoleID: 1, UserID: user.ID}) - db.Insert("sys_role_member", types.RoleMember{RoleID: 2, UserID: user.ID}) + db.Insert("sys_role", types.Role{ID: roleID, Name: fmt.Sprintf("Role %d", roleID)}) + db.Insert("sys_role_member", types.RoleMember{RoleID: roleID, UserID: user.ID}) Expect := func(expected rules.Access, actual rules.Access, format string, params ...interface{}) { Assert(t, expected == actual, format, params...) @@ -35,99 +32,104 @@ func TestRules(t *testing.T) { resources := rules.NewResources(ctx, db) - // default (unset=deny) + // delete all for test roleID = 123456 { - Expect(rules.Inherit, resources.IsAllowed("channel:1", "update"), "expected inherit") - Expect(rules.Inherit, resources.IsAllowed("channel:*", "update"), "expected inherit") + err := resources.Delete(roleID) + NoError(t, err, "expected no error") } - // allow channel:2 group:2 (default deny, multi=allow) + // default (unset=deny), forbidden check ...:* + { + Expect(rules.Inherit, resources.IsAllowed("messaging:channel:1", "update"), "messaging:channel:1 update - Inherit") + Expect(rules.Deny, resources.IsAllowed("messaging:channel:*", "update"), "messaging:channel:* update - Deny") + } + + // allow messaging:channel:2 update,delete { list := []rules.Rule{ - rules.Rule{Resource: "channel:2", Operation: "update", Value: rules.Allow}, - rules.Rule{Resource: "channel:2", Operation: "delete", Value: rules.Allow}, + rules.Rule{Resource: "messaging:channel:2", Operation: "update", Value: rules.Allow}, + rules.Rule{Resource: "messaging:channel:2", Operation: "delete", Value: rules.Allow}, } + err := resources.Grant(roleID, list) + NoError(t, err, "expect no error") - resources.Grant(2, list) - Expect(rules.Inherit, resources.IsAllowed("channel:1", "update"), "expected error, got nil") - Expect(rules.Allow, resources.IsAllowed("channel:2", "update"), "channel:2 update, expected no error") - Expect(rules.Allow, resources.IsAllowed("channel:*", "update"), "channel:* update, expected no error") + Expect(rules.Inherit, resources.IsAllowed("messaging:channel:1", "update"), "messaging:channel:1 update - Inherit") + Expect(rules.Allow, resources.IsAllowed("messaging:channel:2", "update"), "messaging:channel:2 update - Allow") + Expect(rules.Deny, resources.IsAllowed("messaging:channel:*", "update"), "messaging:channel:* update - Deny") } - // list grants for role 2 + // list grants for test role { - grants, err := resources.List(2) + grants, err := resources.List(roleID) NoError(t, err, "expect no error") Assert(t, len(grants) == 2, "expected 2 grants") for _, grant := range grants { - Assert(t, grant.RoleID == 2, "expected RoleID == 2, got %v", grant.RoleID) - Assert(t, grant.Resource == "channel:2", "expected Resource == channel:2, got %s", grant.Resource) - // Assert(t, grant.Operation == "delete", "expected Operation == delete, got %s", grant.Operation) + Assert(t, grant.RoleID == roleID, "expected RoleID == 123456, got %v", grant.RoleID) + Assert(t, grant.Resource == "messaging:channel:2", "expected Resource == messaging:channel:2, got %s", grant.Resource) Assert(t, grant.Value == rules.Allow, "expected Value == Allow, got %s", grant.Value) } } - // deny channel:1 group:1 (explicit deny, multi=deny) + // deny messaging:channel:1 update { list := []rules.Rule{ - rules.Rule{Resource: "channel:1", Operation: "update", Value: rules.Deny}, + rules.Rule{Resource: "messaging:channel:1", Operation: "update", Value: rules.Deny}, } - resources.Grant(1, list) - Expect(rules.Deny, resources.IsAllowed("channel:1", "update"), "expected error, got nil") - Expect(rules.Allow, resources.IsAllowed("channel:2", "update"), "channel:2 update, expected no error") - Expect(rules.Deny, resources.IsAllowed("channel:*", "update"), "expected error, got nil") + err := resources.Grant(roleID, list) + NoError(t, err, "expect no error") + + Expect(rules.Deny, resources.IsAllowed("messaging:channel:1", "update"), "messaging:channel:1 update - Deny") + Expect(rules.Allow, resources.IsAllowed("messaging:channel:2", "update"), "messaging:channel:2 update - Allow") + Expect(rules.Deny, resources.IsAllowed("messaging:channel:*", "update"), "messaging:channel:* update - Deny") } - // reset (unset=deny) - { - list1 := []rules.Rule{ - rules.Rule{Resource: "channel:1", Operation: "update", Value: rules.Inherit}, - rules.Rule{Resource: "channel:1", Operation: "delete", Value: rules.Inherit}, - } - resources.Grant(1, list1) - - list2 := []rules.Rule{ - rules.Rule{Resource: "channel:2", Operation: "update", Value: rules.Inherit}, - rules.Rule{Resource: "channel:2", Operation: "delete", Value: rules.Inherit}, - } - resources.Grant(2, list2) - - Expect(rules.Inherit, resources.IsAllowed("channel:1", "update"), "expected error, got nil") - Expect(rules.Inherit, resources.IsAllowed("channel:*", "update"), "expected error, got nil") - } - - // Grant by roleID + // reset messaging:channel:1, messaging:channel:2 { list := []rules.Rule{ - rules.Rule{Resource: "channel:*", Operation: "update", Value: rules.Allow}, - rules.Rule{Resource: "channel:1", Operation: "update", Value: rules.Deny}, - rules.Rule{Resource: "channel:2", Operation: "update"}, + rules.Rule{Resource: "messaging:channel:1", Operation: "update", Value: rules.Inherit}, + rules.Rule{Resource: "messaging:channel:1", Operation: "delete", Value: rules.Inherit}, + rules.Rule{Resource: "messaging:channel:2", Operation: "update", Value: rules.Inherit}, + rules.Rule{Resource: "messaging:channel:2", Operation: "delete", Value: rules.Inherit}, + } + err := resources.Grant(roleID, list) + NoError(t, err, "expect no error") + + Expect(rules.Inherit, resources.IsAllowed("messaging:channel:1", "update"), "messaging:channel:1 update - Inherit") + Expect(rules.Inherit, resources.IsAllowed("messaging:channel:2", "update"), "messaging:channel:2 update - Inherit") + } + + // [messaging:channel:*,update] - allow, [messaging:channel:1, deny] + { + list := []rules.Rule{ + rules.Rule{Resource: "messaging:channel:*", Operation: "update", Value: rules.Allow}, + rules.Rule{Resource: "messaging:channel:1", Operation: "update", Value: rules.Deny}, + rules.Rule{Resource: "messaging:channel:2", Operation: "update"}, rules.Rule{Resource: "system", Operation: "organisation.create", Value: rules.Allow}, } - err := resources.Grant(2, list) + err := resources.Grant(roleID, list) NoError(t, err, "expected no error") + + Expect(rules.Deny, resources.IsAllowed("messaging:channel:1", "update"), "messaging:channel:1 update - Deny") + Expect(rules.Allow, resources.IsAllowed("messaging:channel:2", "update"), "messaging:channel:2 update - Allow") } // list all by roleID { - grants, err := resources.List(2) - - fmt.Println(grants) - + grants, err := resources.List(roleID) NoError(t, err, "expected no error") Assert(t, len(grants) == 3, "expected grants == 3, got %v", len(grants)) } // delete all by roleID { - err := resources.Delete(2) + err := resources.Delete(roleID) NoError(t, err, "expected no error") } // list all by roleID { - grants, err := resources.List(2) + grants, err := resources.List(roleID) NoError(t, err, "expected no error") Assert(t, len(grants) == 0, "expected grants == 0, got %v", len(grants)) } diff --git a/system/service/main_test.go b/system/service/main_test.go index 44db976fe..b84b0c449 100644 --- a/system/service/main_test.go +++ b/system/service/main_test.go @@ -2,14 +2,64 @@ package service import ( "fmt" + "log" + "os" "runtime" "testing" + + "github.com/joho/godotenv" + "github.com/namsral/flag" + "github.com/titpetric/factory" + + systemMigrate "github.com/crusttech/crust/system/db" ) type mockDB struct{} func (mockDB) Transaction(callback func() error) error { return callback() } +func TestMain(m *testing.M) { + // @todo this is a very optimistic initialization, make it more robust + godotenv.Load("../../.env") + + prefix := "system" + dsn := "" + + p := func(s string) string { + return prefix + "-" + s + } + + flag.StringVar(&dsn, p("db-dsn"), "crust:crust@tcp(db1:3306)/crust?collation=utf8mb4_general_ci", "DSN for database connection") + flag.Parse() + + if testing.Short() { + return + } + + factory.Database.Add("default", dsn) + + db := factory.Database.MustGet() + db.Profiler = &factory.Database.ProfilerStdout + + // migrate database schema + if err := systemMigrate.Migrate(db); err != nil { + log.Printf("Error running migrations: %+v\n", err) + return + } + + // clean up tables + { + for _, name := range []string{"sys_user", "sys_role", "sys_role_member", "sys_organisation", "sys_rules"} { + _, err := db.Exec("truncate " + name) + if err != nil { + panic("Error when clearing " + name + ": " + err.Error()) + } + } + } + + os.Exit(m.Run()) +} + func assert(t *testing.T, ok bool, format string, args ...interface{}) bool { if !ok { _, file, line, _ := runtime.Caller(1) diff --git a/system/service/permission.go b/system/service/permission.go index 6d1007278..b7b5f3c61 100644 --- a/system/service/permission.go +++ b/system/service/permission.go @@ -43,6 +43,12 @@ func (p *permission) Get(roleID uint64) (interface{}, error) { } func (p *permission) Update(roleID uint64, rules []rules.Rule) (interface{}, error) { + for _, rule := range rules { + err := validatePermission(rule.Resource, rule.Operation) + if err != nil { + return nil, err + } + } return nil, p.resources.Grant(roleID, rules) } diff --git a/system/service/permission_test.go b/system/service/permission_test.go index 7e88c299e..ade3022fc 100644 --- a/system/service/permission_test.go +++ b/system/service/permission_test.go @@ -13,7 +13,7 @@ import ( systemTypes "github.com/crusttech/crust/system/types" ) -func TestPermissions(t *testing.T) { +func TestPermission(t *testing.T) { if testing.Short() { t.Skip("skipping test in short mode.") return @@ -50,23 +50,47 @@ func TestPermissions(t *testing.T) { // Create permission service. permissionSvc := Permission().With(ctx) - // Setup rules for test role. + // Update rules for test role. { - rules := []rules.Rule{ - rules.Rule{Resource: "messaging:channel:*", Operation: "update", Value: rules.Allow}, - rules.Rule{Resource: "messaging:channel:1", Operation: "update", Value: rules.Deny}, - rules.Rule{Resource: "messaging:channel:2", Operation: "update"}, + list := []rules.Rule{ + rules.Rule{Resource: "messaging:channel:*", Operation: "message.update.all", Value: rules.Allow}, + rules.Rule{Resource: "messaging:channel:1", Operation: "message.update.all", Value: rules.Deny}, + rules.Rule{Resource: "messaging:channel:2", Operation: "message.update.all"}, rules.Rule{Resource: "system", Operation: "organisation.create", Value: rules.Allow}, - rules.Rule{Resource: "system:organisation:*", Operation: "update", Value: rules.Allow}, - rules.Rule{Resource: "system:organisation:*", Operation: "delete", Value: rules.Allow}, - rules.Rule{Resource: "messaging:channel:*", Operation: "update.name", Value: rules.Allow}, - rules.Rule{Resource: "messaging:channel:*", Operation: "update.topic", Value: rules.Allow}, - rules.Rule{Resource: "messaging:channel:*", Operation: "members.manage", Value: rules.Allow}, + rules.Rule{Resource: "system:organisation:*", Operation: "access", Value: rules.Allow}, + rules.Rule{Resource: "messaging:channel", Operation: "message.update.all", Value: rules.Allow}, } - _, err := permissionSvc.Update(role.ID, rules) + _, err := permissionSvc.Update(role.ID, list) NoError(t, err, "expected no error, setting rules") } + // Update with invalid roles + { + list := []rules.Rule{ + rules.Rule{Resource: "nosystem:channel:*", Operation: "message.update.all", Value: rules.Allow}, + } + _, err := permissionSvc.Update(role.ID, list) + Error(t, err, "expected error") + + list = []rules.Rule{ + rules.Rule{Resource: "messaging:noresource:1", Operation: "message.update.all", Value: rules.Deny}, + } + _, err = permissionSvc.Update(role.ID, list) + Error(t, err, "expected error") + + list = []rules.Rule{ + rules.Rule{Resource: "messaging:channel:", Operation: "message.update.all"}, + } + _, err = permissionSvc.Update(role.ID, list) + Error(t, err, "expected error") + + list = []rules.Rule{ + rules.Rule{Resource: "system:organisation:*", Operation: "invalid", Value: rules.Allow}, + } + _, err = permissionSvc.Update(role.ID, list) + Error(t, err, "expected error") + } + // List rules for test role. { ret, err := permissionSvc.Get(role.ID) @@ -74,7 +98,7 @@ func TestPermissions(t *testing.T) { rules := ret.([]rules.Rule) - Assert(t, len(rules) == 8, "expected len(rules) == 8, got %v", len(rules)) + Assert(t, len(rules) == 5, "expected len(rules) == 5, got %v", len(rules)) } // Delete rules for test role. diff --git a/system/service/permissions.go b/system/service/permissions.go deleted file mode 100644 index a9e866e74..000000000 --- a/system/service/permissions.go +++ /dev/null @@ -1,41 +0,0 @@ -package service - -import ( - "strings" - - "github.com/pkg/errors" -) - -func validatePermission(resource string, operation string) error { - var services = map[string]map[string]bool{ - "messaging:channel": map[string]bool{ - "manage.webhooks": false, - "message.send": true, - "message.embed": true, - "message.attach": true, - "message.update_own": true, - "message.update_all": true, - "message.react": true, - }, - } - - delimiter := ":" - resourceParts := strings.Split(resource, delimiter) - if len(resourceParts) < 2 { - return errors.Errorf("Invalid resource format, expected >= 2, got %d", len(resourceParts)) - } - - resourceName := resourceParts[0] + delimiter + resourceParts[1] - if service, ok := services[resourceName]; ok { - if op := service[operation]; op { - if len(resourceParts) == 3 { - if val := resourceParts[2]; val != "" { - return nil - } - } - return errors.Errorf("Invalid resource format, missing resource ID") - } - return errors.Errorf("Unknown operation: '%s'", operation) - } - return errors.Errorf("Unknown resource name: '%s'", resourceName) -} diff --git a/system/service/validation.go b/system/service/validation.go new file mode 100644 index 000000000..9d0509b39 --- /dev/null +++ b/system/service/validation.go @@ -0,0 +1,110 @@ +package service + +import ( + "strings" + + "github.com/pkg/errors" +) + +func validatePermission(resource string, operation string) error { + var services = map[string]map[string]bool{ + "system": map[string]bool{ + "access": true, + "organisation.create": true, + "role.create": true, + }, + "system:organisation": map[string]bool{ + "access": true, + }, + "system:role": map[string]bool{ + "read": true, + "update": true, + "delete": true, + "members.manage": true, + }, + "messaging": map[string]bool{ + "access": true, + "channel.public.create": true, + "channel.private.create": true, + "channel.direct.create": true, + }, + "messaging:channel": map[string]bool{ + "update": true, + "read": true, + "join": true, + "leave": true, + "members.manage": true, + "webhooks.manage": true, + "attachments.manage": true, + "message.send": true, + "message.reply": true, + "message.embed": true, + "message.attach": true, + "message.update.own": true, + "message.update.all": true, + "message.react": true, + }, + "compose": map[string]bool{ + "access": true, + "namespace.create": true, + }, + "compose:namespace": map[string]bool{ + "read": true, + "update": true, + "delete": true, + "module.create": true, + "chart.create": true, + "trigger.create": true, + "page.create": true, + }, + "compose:module": map[string]bool{ + "read": true, + "update": true, + "delete": true, + "record.create": true, + "record.read": true, + "record.update": true, + "record.delete": true, + }, + "compose:chart": map[string]bool{ + "read": true, + "update": true, + "delete": true, + }, + "compose:trigger": map[string]bool{ + "read": true, + "update": true, + "delete": true, + }, + "compose:page": map[string]bool{ + "read": true, + "update": true, + "delete": true, + }, + } + + delimiter := ":" + resourceParts := strings.Split(resource, delimiter) + if len(resourceParts) < 1 { + return errors.Errorf("Invalid resource format, expected >= 1, got %d", len(resourceParts)) + } + + resourceName := resourceParts[0] + if len(resourceParts) > 1 { + resourceName = resourceParts[0] + delimiter + resourceParts[1] + } + + if service, ok := services[resourceName]; ok { + if op := service[operation]; op { + if len(resourceParts) == 3 { + if val := resourceParts[2]; val != "" { + return nil + } + return errors.Errorf("Invalid resource format, missing resource ID") + } + return nil + } + return errors.Errorf("Unknown operation: '%s'", operation) + } + return errors.Errorf("Unknown resource name: '%s'", resourceName) +} diff --git a/system/service/permissions_test.go b/system/service/validation_test.go similarity index 100% rename from system/service/permissions_test.go rename to system/service/validation_test.go