From 9a4a08e35f0d74bf2e7e643b89090ec1084ad307 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 10:22:20 +0200 Subject: [PATCH 1/9] remove debug code --- pkg/proto/v0/accounts.pb.micro_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/proto/v0/accounts.pb.micro_test.go b/pkg/proto/v0/accounts.pb.micro_test.go index 56c6e331d3..5764452c85 100644 --- a/pkg/proto/v0/accounts.pb.micro_test.go +++ b/pkg/proto/v0/accounts.pb.micro_test.go @@ -189,8 +189,6 @@ func buildRoleServiceMock() settings.RoleService { return settings.MockRoleService{ AssignRoleToUserFunc: func(ctx context.Context, req *settings.AssignRoleToUserRequest, opts ...client.CallOption) (res *settings.AssignRoleToUserResponse, err error) { mockedRoleAssignment[req.AccountUuid] = req.RoleId - fmt.Println(mockedRoleAssignment) - fmt.Println("asdf blablabla") return &settings.AssignRoleToUserResponse{ Assignment: &settings.UserRoleAssignment{ AccountUuid: req.AccountUuid, From d6db5c9f640b26195742830ebc8c9935e561393a Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:36:27 +0200 Subject: [PATCH 2/9] Test setup for accounts handler and ListAccounts permission checks --- pkg/service/v0/accounts_test.go | 164 ++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 pkg/service/v0/accounts_test.go diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_test.go new file mode 100644 index 0000000000..a2c224b042 --- /dev/null +++ b/pkg/service/v0/accounts_test.go @@ -0,0 +1,164 @@ +package service + +import ( + "context" + "encoding/json" + "log" + "net/http" + "os" + "testing" + "time" + + "github.com/micro/go-micro/v2/client" + merrors "github.com/micro/go-micro/v2/errors" + "github.com/micro/go-micro/v2/metadata" + "github.com/owncloud/ocis-accounts/pkg/config" + "github.com/owncloud/ocis-accounts/pkg/proto/v0" + olog "github.com/owncloud/ocis-pkg/v2/log" + "github.com/owncloud/ocis-pkg/v2/middleware" + "github.com/owncloud/ocis-pkg/v2/roles" + settings "github.com/owncloud/ocis-settings/pkg/proto/v0" + ssvc "github.com/owncloud/ocis-settings/pkg/service/v0" + "github.com/stretchr/testify/assert" +) + +const dataPath = "/var/tmp/ocis-accounts-tests" + +var ( + roleServiceMock settings.RoleService + s *Service +) + +func init() { + cfg := config.New() + cfg.Server.Name = "accounts" + cfg.Server.AccountsDataPath = dataPath + logger := olog.NewLogger(olog.Color(true), olog.Pretty(true)) + roleServiceMock = buildRoleServiceMock() + roleManager := roles.NewManager( + roles.Logger(logger), + roles.RoleService(roleServiceMock), + roles.CacheTTL(time.Hour), + roles.CacheSize(1024), + ) + s, _ = New( + Logger(logger), + Config(cfg), + RoleService(roleServiceMock), + RoleManager(&roleManager), + ) +} + +func setup() (teardown func()) { + return func() { + if err := os.RemoveAll(dataPath); err != nil { + log.Printf("could not delete data root: %s", dataPath) + } else { + log.Println("data root deleted") + } + } +} + +// TestPermissionsListAccounts checks permission handling on ListAccounts +func TestPermissionsListAccounts(t *testing.T) { + var scenarios = []struct { + name string + roleIDs []string + query string + permissionError error + }{ + // TODO: remove this test when https://github.com/owncloud/ocis-accounts/pull/111 is merged + // replace with two tests: + // 1: "ListAccounts fails with 403 when no roleIDs in context" + // 2: "ListAccounts fails with 403 when no admin role in context and query empty" + { + "ListAccounts succeeds when no roleIDs in context", + nil, + "", + nil, + }, + { + "ListAccounts fails when no admin roleID in context", + []string{ssvc.BundleUUIDRoleUser, ssvc.BundleUUIDRoleGuest}, + "", + merrors.Forbidden(s.id, "no permission for ListAccounts"), + }, + { + "ListAccounts succeeds when admin roleID in context", + []string{ssvc.BundleUUIDRoleAdmin}, + "", + nil, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + teardown := setup() + defer teardown() + + ctx := context.Background() + if scenario.roleIDs != nil { + roleIDs, err := json.Marshal(scenario.roleIDs) + assert.NoError(t, err) + ctx = metadata.Set(ctx, middleware.RoleIDs, string(roleIDs)) + } + + request := &proto.ListAccountsRequest{ + Query: scenario.query, + } + response := &proto.ListAccountsResponse{} + err := s.ListAccounts(ctx, request, response) + if scenario.permissionError != nil { + assert.Equal(t, scenario.permissionError, err) + } else if err != nil { + // we are only checking permissions here, so just check that the error code is not 403 + merr := merrors.FromError(err) + assert.NotEqual(t, http.StatusForbidden, merr.GetCode()) + } + }) + } +} + +func buildRoleServiceMock() settings.RoleService { + defaultRoles := map[string]*settings.Bundle{ + ssvc.BundleUUIDRoleAdmin: { + Id: ssvc.BundleUUIDRoleAdmin, + Type: settings.Bundle_TYPE_ROLE, + Resource: &settings.Resource{ + Type: settings.Resource_TYPE_SYSTEM, + }, + Settings: []*settings.Setting{ + { + Id: AccountManagementPermissionID, + }, + }, + }, + ssvc.BundleUUIDRoleUser: { + Id: ssvc.BundleUUIDRoleUser, + Type: settings.Bundle_TYPE_ROLE, + Resource: &settings.Resource{ + Type: settings.Resource_TYPE_SYSTEM, + }, + Settings: []*settings.Setting{}, + }, + } + return settings.MockRoleService{ + ListRolesFunc: func(ctx context.Context, req *settings.ListBundlesRequest, opts ...client.CallOption) (res *settings.ListBundlesResponse, err error) { + payload := make([]*settings.Bundle, 0) + for _, roleID := range req.BundleIds { + if defaultRoles[roleID] != nil { + payload = append(payload, defaultRoles[roleID]) + } + } + return &settings.ListBundlesResponse{ + Bundles: payload, + }, nil + }, + AssignRoleToUserFunc: func(ctx context.Context, req *settings.AssignRoleToUserRequest, opts ...client.CallOption) (res *settings.AssignRoleToUserResponse, err error) { + // mock can be empty. function is called during service start. actual role assignments not needed for the tests. + return &settings.AssignRoleToUserResponse{ + Assignment: &settings.UserRoleAssignment{}, + }, nil + }, + } +} From a0cbbaaf71c114db1840280354aebb9c7e5adaae Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:43:28 +0200 Subject: [PATCH 3/9] Add test for GetAccount permission checks --- pkg/service/v0/accounts_test.go | 63 +++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_test.go index a2c224b042..014455e0a7 100644 --- a/pkg/service/v0/accounts_test.go +++ b/pkg/service/v0/accounts_test.go @@ -96,13 +96,7 @@ func TestPermissionsListAccounts(t *testing.T) { teardown := setup() defer teardown() - ctx := context.Background() - if scenario.roleIDs != nil { - roleIDs, err := json.Marshal(scenario.roleIDs) - assert.NoError(t, err) - ctx = metadata.Set(ctx, middleware.RoleIDs, string(roleIDs)) - } - + ctx := buildTestCtx(t, scenario.roleIDs) request := &proto.ListAccountsRequest{ Query: scenario.query, } @@ -119,6 +113,61 @@ func TestPermissionsListAccounts(t *testing.T) { } } +// TestPermissionsGetAccount checks permission handling on GetAccount +// TODO: remove this test function entirely, when https://github.com/owncloud/ocis-accounts/pull/111 is merged. GetAccount will not have permission checks for the time being. +func TestPermissionsGetAccount(t *testing.T) { + var scenarios = []struct { + name string + roleIDs []string + permissionError error + }{ + { + "GetAccount succeeds when no role IDs in context", + nil, + nil, + }, + { + "GetAccount fails when no admin roleID in context", + []string{ssvc.BundleUUIDRoleUser, ssvc.BundleUUIDRoleGuest}, + merrors.Forbidden(s.id, "no permission for GetAccount"), + }, + { + "GetAccount succeeds when admin roleID in context", + []string{ssvc.BundleUUIDRoleAdmin}, + nil, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + teardown := setup() + defer teardown() + + ctx := buildTestCtx(t, scenario.roleIDs) + request := &proto.GetAccountRequest{} + response := &proto.Account{} + err := s.GetAccount(ctx, request, response) + if scenario.permissionError != nil { + assert.Equal(t, scenario.permissionError, err) + } else if err != nil { + // we are only checking permissions here, so just check that the error code is not 403 + merr := merrors.FromError(err) + assert.NotEqual(t, http.StatusForbidden, merr.GetCode()) + } + }) + } +} + +func buildTestCtx(t *testing.T, roleIDs []string) context.Context { + ctx := context.Background() + if roleIDs != nil { + roleIDs, err := json.Marshal(roleIDs) + assert.NoError(t, err) + ctx = metadata.Set(ctx, middleware.RoleIDs, string(roleIDs)) + } + return ctx +} + func buildRoleServiceMock() settings.RoleService { defaultRoles := map[string]*settings.Bundle{ ssvc.BundleUUIDRoleAdmin: { From dc2484d5845283213a62ae1eaff77c0be34cff27 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:47:29 +0200 Subject: [PATCH 4/9] Add tests for UpdateAccount --- pkg/service/v0/accounts_test.go | 52 +++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_test.go index 014455e0a7..7f4d259f70 100644 --- a/pkg/service/v0/accounts_test.go +++ b/pkg/service/v0/accounts_test.go @@ -69,8 +69,8 @@ func TestPermissionsListAccounts(t *testing.T) { }{ // TODO: remove this test when https://github.com/owncloud/ocis-accounts/pull/111 is merged // replace with two tests: - // 1: "ListAccounts fails with 403 when no roleIDs in context" - // 2: "ListAccounts fails with 403 when no admin role in context and query empty" + // 1: "ListAccounts fails with 403 when roleIDs don't exist in context" + // 2: "ListAccounts fails with 403 when ('no admin role in context' AND 'empty query')" { "ListAccounts succeeds when no roleIDs in context", nil, @@ -158,6 +158,54 @@ func TestPermissionsGetAccount(t *testing.T) { } } +// TestPermissionsUpdateAccount checks permission handling on UpdateAccount +func TestPermissionsUpdateAccount(t *testing.T) { + var scenarios = []struct { + name string + roleIDs []string + permissionError error + }{ + // TODO: remove this test when https://github.com/owncloud/ocis-accounts/pull/111 is merged + // replace with two tests: + // 1: "UpdateAccount fails with 403 when roleIDs don't exist in context" + // 2: "UpdateAccount fails with 403 when no admin role in context" + { + "UpdateAccount succeeds when no role IDs in context", + nil, + nil, + }, + { + "UpdateAccount fails when no admin roleID in context", + []string{ssvc.BundleUUIDRoleUser, ssvc.BundleUUIDRoleGuest}, + merrors.Forbidden(s.id, "no permission for UpdateAccount"), + }, + { + "UpdateAccount succeeds when admin roleID in context", + []string{ssvc.BundleUUIDRoleAdmin}, + nil, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + teardown := setup() + defer teardown() + + ctx := buildTestCtx(t, scenario.roleIDs) + request := &proto.UpdateAccountRequest{} + response := &proto.Account{} + err := s.UpdateAccount(ctx, request, response) + if scenario.permissionError != nil { + assert.Equal(t, scenario.permissionError, err) + } else if err != nil { + // we are only checking permissions here, so just check that the error code is not 403 + merr := merrors.FromError(err) + assert.NotEqual(t, http.StatusForbidden, merr.GetCode()) + } + }) + } +} + func buildTestCtx(t *testing.T, roleIDs []string) context.Context { ctx := context.Background() if roleIDs != nil { From c4edd4828d7699316e328f013399637fe01a82ed Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:50:08 +0200 Subject: [PATCH 5/9] Add test for permission checks on DeleteAccount --- pkg/service/v0/accounts_test.go | 49 +++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_test.go index 7f4d259f70..db65e51180 100644 --- a/pkg/service/v0/accounts_test.go +++ b/pkg/service/v0/accounts_test.go @@ -3,6 +3,7 @@ package service import ( "context" "encoding/json" + "github.com/golang/protobuf/ptypes/empty" "log" "net/http" "os" @@ -206,6 +207,54 @@ func TestPermissionsUpdateAccount(t *testing.T) { } } +// TestPermissionsDeleteAccount checks permission handling on DeleteAccount +func TestPermissionsDeleteAccount(t *testing.T) { + var scenarios = []struct { + name string + roleIDs []string + permissionError error + }{ + // TODO: remove this test when https://github.com/owncloud/ocis-accounts/pull/111 is merged + // replace with two tests: + // 1: "DeleteAccount fails with 403 when roleIDs don't exist in context" + // 2: "DeleteAccount fails with 403 when no admin role in context" + { + "DeleteAccount succeeds when no role IDs in context", + nil, + nil, + }, + { + "DeleteAccount fails when no admin roleID in context", + []string{ssvc.BundleUUIDRoleUser, ssvc.BundleUUIDRoleGuest}, + merrors.Forbidden(s.id, "no permission for DeleteAccount"), + }, + { + "DeleteAccount succeeds when admin roleID in context", + []string{ssvc.BundleUUIDRoleAdmin}, + nil, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + teardown := setup() + defer teardown() + + ctx := buildTestCtx(t, scenario.roleIDs) + request := &proto.DeleteAccountRequest{} + response := &empty.Empty{} + err := s.DeleteAccount(ctx, request, response) + if scenario.permissionError != nil { + assert.Equal(t, scenario.permissionError, err) + } else if err != nil { + // we are only checking permissions here, so just check that the error code is not 403 + merr := merrors.FromError(err) + assert.NotEqual(t, http.StatusForbidden, merr.GetCode()) + } + }) + } +} + func buildTestCtx(t *testing.T, roleIDs []string) context.Context { ctx := context.Background() if roleIDs != nil { From b1a4b48167eacfb8ff68f96bb39450dfb01cb9db Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:51:52 +0200 Subject: [PATCH 6/9] Add test for permission checks on CreateAccount --- pkg/service/v0/accounts_test.go | 48 +++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_test.go index db65e51180..40695288d3 100644 --- a/pkg/service/v0/accounts_test.go +++ b/pkg/service/v0/accounts_test.go @@ -159,6 +159,54 @@ func TestPermissionsGetAccount(t *testing.T) { } } +// TestPermissionsCreateAccount checks permission handling on CreateAccount +func TestPermissionsCreateAccount(t *testing.T) { + var scenarios = []struct { + name string + roleIDs []string + permissionError error + }{ + // TODO: remove this test when https://github.com/owncloud/ocis-accounts/pull/111 is merged + // replace with two tests: + // 1: "CreateAccount fails with 403 when roleIDs don't exist in context" + // 2: "CreateAccount fails with 403 when no admin role in context" + { + "CreateAccount succeeds when no role IDs in context", + nil, + nil, + }, + { + "CreateAccount fails when no admin roleID in context", + []string{ssvc.BundleUUIDRoleUser, ssvc.BundleUUIDRoleGuest}, + merrors.Forbidden(s.id, "no permission for CreateAccount"), + }, + { + "CreateAccount succeeds when admin roleID in context", + []string{ssvc.BundleUUIDRoleAdmin}, + nil, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + teardown := setup() + defer teardown() + + ctx := buildTestCtx(t, scenario.roleIDs) + request := &proto.CreateAccountRequest{} + response := &proto.Account{} + err := s.CreateAccount(ctx, request, response) + if scenario.permissionError != nil { + assert.Equal(t, scenario.permissionError, err) + } else if err != nil { + // we are only checking permissions here, so just check that the error code is not 403 + merr := merrors.FromError(err) + assert.NotEqual(t, http.StatusForbidden, merr.GetCode()) + } + }) + } +} + // TestPermissionsUpdateAccount checks permission handling on UpdateAccount func TestPermissionsUpdateAccount(t *testing.T) { var scenarios = []struct { From 232e84e1318c5469f7fd19ddad389f9c90d87097 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:52:50 +0200 Subject: [PATCH 7/9] Rename test file to reflect that it's only about permission checks --- pkg/service/v0/{accounts_test.go => accounts_permission_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename pkg/service/v0/{accounts_test.go => accounts_permission_test.go} (100%) diff --git a/pkg/service/v0/accounts_test.go b/pkg/service/v0/accounts_permission_test.go similarity index 100% rename from pkg/service/v0/accounts_test.go rename to pkg/service/v0/accounts_permission_test.go From 7c315fe0015e760476a61cb1787f5be4963de357 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:57:57 +0200 Subject: [PATCH 8/9] Sort imports --- pkg/service/v0/accounts_permission_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/service/v0/accounts_permission_test.go b/pkg/service/v0/accounts_permission_test.go index 40695288d3..cd61e41aa9 100644 --- a/pkg/service/v0/accounts_permission_test.go +++ b/pkg/service/v0/accounts_permission_test.go @@ -3,13 +3,13 @@ package service import ( "context" "encoding/json" - "github.com/golang/protobuf/ptypes/empty" "log" "net/http" "os" "testing" "time" + "github.com/golang/protobuf/ptypes/empty" "github.com/micro/go-micro/v2/client" merrors "github.com/micro/go-micro/v2/errors" "github.com/micro/go-micro/v2/metadata" From 804e9914f1526abc14cbc0b77d5b8513af087e0b Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 4 Sep 2020 11:59:34 +0200 Subject: [PATCH 9/9] Add mock guest role --- pkg/service/v0/accounts_permission_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/service/v0/accounts_permission_test.go b/pkg/service/v0/accounts_permission_test.go index cd61e41aa9..f93303eae4 100644 --- a/pkg/service/v0/accounts_permission_test.go +++ b/pkg/service/v0/accounts_permission_test.go @@ -335,6 +335,14 @@ func buildRoleServiceMock() settings.RoleService { }, Settings: []*settings.Setting{}, }, + ssvc.BundleUUIDRoleGuest: { + Id: ssvc.BundleUUIDRoleGuest, + Type: settings.Bundle_TYPE_ROLE, + Resource: &settings.Resource{ + Type: settings.Resource_TYPE_SYSTEM, + }, + Settings: []*settings.Setting{}, + }, } return settings.MockRoleService{ ListRolesFunc: func(ctx context.Context, req *settings.ListBundlesRequest, opts ...client.CallOption) (res *settings.ListBundlesResponse, err error) {