From 179381ef08910a6430e5475c2c7e9a8f201a69bd Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Sun, 3 Oct 2021 17:53:41 +0200 Subject: [PATCH] Improve workflow & system tests, logging A set of changes touching different parts of system all related to initialization of system users and groups --- automation/service/workflow.go | 12 ++++++-- pkg/provision/roles.go | 4 +-- pkg/provision/users.go | 5 +++- pkg/rbac/service.go | 2 ++ system/service/user.go | 28 ------------------- tests/system/role_test.go | 5 ++++ tests/system/user_test.go | 5 ++++ .../invoker_and_runner_in_scope_test.go | 6 +++- tests/workflows/main_test.go | 1 - 9 files changed, 33 insertions(+), 35 deletions(-) diff --git a/automation/service/workflow.go b/automation/service/workflow.go index 7d09e2b90..1925b36ef 100644 --- a/automation/service/workflow.go +++ b/automation/service/workflow.go @@ -593,10 +593,18 @@ func (svc *workflow) Exec(ctx context.Context, workflowID uint64, p types.Workfl ssp.ResourceType = "" } + // Returns context with identity set to service user + // + // Current user (identity in the context) might not have + // sufficient privileges to load info about invoker and runner + sysUserCtx := func() context.Context { + return intAuth.SetIdentityToContext(ctx, intAuth.ServiceUser()) + } + if invokerId := intAuth.GetIdentityFromContext(ctx).Identity(); invokerId > 0 { var is bool if invoker, is = intAuth.GetIdentityFromContext(ctx).(*sysTypes.User); !is { - if invoker, err = DefaultUser.FindByAny(ctx, invokerId); err != nil { + if invoker, err = DefaultUser.FindByAny(sysUserCtx(), invokerId); err != nil { return } } @@ -605,7 +613,7 @@ func (svc *workflow) Exec(ctx context.Context, workflowID uint64, p types.Workfl } if wf.RunAs > 0 { - if runner, err = DefaultUser.FindByAny(ctx, wf.RunAs); err != nil { + if runner, err = DefaultUser.FindByAny(sysUserCtx(), wf.RunAs); err != nil { return } } diff --git a/pkg/provision/roles.go b/pkg/provision/roles.go index 3451f11b3..251489938 100644 --- a/pkg/provision/roles.go +++ b/pkg/provision/roles.go @@ -49,14 +49,13 @@ func SystemRoles(ctx context.Context, log *zap.Logger, s store.Storer) (rr []*ty for i := range rr { r := rr[i] if m[r.Handle] == nil { - log.Info("creating system role", zap.String("handle", r.Handle)) // this is a new role r.ID = id.Next() r.CreatedAt = *now() m[r.Handle] = r + log.Info("creating system role", zap.String("handle", r.Handle), zap.Uint64("ID", r.ID)) } else { - log.Info("updating system role", zap.String("handle", r.Handle)) // use existing role rr[i] = m[r.Handle] @@ -65,6 +64,7 @@ func SystemRoles(ctx context.Context, log *zap.Logger, s store.Storer) (rr []*ty r.DeletedAt = nil r.ArchivedAt = nil + log.Info("updating system role", zap.String("handle", r.Handle), zap.Uint64("ID", r.ID)) } } diff --git a/pkg/provision/users.go b/pkg/provision/users.go index faf215231..209eb3ac5 100644 --- a/pkg/provision/users.go +++ b/pkg/provision/users.go @@ -43,7 +43,6 @@ func SystemUsers(ctx context.Context, log *zap.Logger, s store.Users) (uu []*typ for i := range uu { u := uu[i] if m[u.Handle] == nil { - log.Info("creating user", zap.String("handle", u.Handle)) // this is a new user u.ID = id.Next() u.CreatedAt = *now() @@ -51,7 +50,10 @@ func SystemUsers(ctx context.Context, log *zap.Logger, s store.Users) (uu []*typ if err := store.UpsertUser(ctx, s, u); err != nil { return nil, fmt.Errorf("failed to provision system user %s: %w", u.Handle, err) } + + log.Info("creating system user", zap.String("handle", u.Handle), zap.Uint64("ID", u.ID)) } else { + u.ID = m[u.Handle].ID // There is no need to update system users if they are unchanged @@ -73,6 +75,7 @@ func SystemUsers(ctx context.Context, log *zap.Logger, s store.Users) (uu []*typ return nil, fmt.Errorf("failed to provision system user %s: %w", u.Handle, err) } + log.Info("updating system user", zap.String("handle", u.Handle), zap.Uint64("ID", u.ID)) } } diff --git a/pkg/rbac/service.go b/pkg/rbac/service.go index a24d1bdf1..94af701fe 100644 --- a/pkg/rbac/service.go +++ b/pkg/rbac/service.go @@ -199,6 +199,8 @@ func (svc *service) reloadRules(ctx context.Context) { } // UpdateRoles updates RBAC roles +// +// Warning: this REPLACES all existing roles that are recognized by RBAC subsystem func (svc *service) UpdateRoles(rr ...*Role) { svc.l.Lock() defer svc.l.Unlock() diff --git a/system/service/user.go b/system/service/user.go index ed257c5a3..e4b6dcd42 100644 --- a/system/service/user.go +++ b/system/service/user.go @@ -70,7 +70,6 @@ type ( } UserService interface { - FindByUsername(ctx context.Context, username string) (*types.User, error) FindByEmail(ctx context.Context, email string) (*types.User, error) FindByHandle(ctx context.Context, handle string) (*types.User, error) FindByID(ctx context.Context, id uint64) (*types.User, error) @@ -172,33 +171,6 @@ func (svc user) FindByEmail(ctx context.Context, email string) (u *types.User, e return u, svc.recordAction(ctx, uaProps, UserActionLookup, err) } -func (svc user) FindByUsername(ctx context.Context, username string) (u *types.User, err error) { - var ( - uaProps = &userActionProps{user: &types.User{Username: username}} - ) - - err = func() error { - u, err = store.LookupUserByUsername(ctx, svc.store, username) - if u, err = svc.proc(ctx, u, err); err != nil { - return err - } - - uaProps.setUser(u) - - if !svc.ac.CanReadUser(ctx, u) { - return UserErrNotAllowedToRead() - } - - if err = label.Load(ctx, svc.store, u); err != nil { - return err - } - - return nil - }() - - return u, svc.recordAction(ctx, uaProps, UserActionLookup, err) -} - func (svc user) FindByHandle(ctx context.Context, handle string) (u *types.User, err error) { var ( uaProps = &userActionProps{user: &types.User{Handle: handle}} diff --git a/tests/system/role_test.go b/tests/system/role_test.go index c927a69fa..9e8932798 100644 --- a/tests/system/role_test.go +++ b/tests/system/role_test.go @@ -479,6 +479,7 @@ func TestMemberList(t *testing.T) { h.apiInit(). Get(fmt.Sprintf("/roles/%d/members", r.ID)). + Header("Accept", "application/json"). Expect(t). Status(http.StatusOK). Assert(helpers.AssertNoErrors). @@ -488,6 +489,7 @@ func TestMemberList(t *testing.T) { func TestMemberAdd(t *testing.T) { h := newHelper(t) + helpers.AllowMe(h, types.UserRbacResource(0), "read") helpers.AllowMe(h, types.RoleRbacResource(0), "members.manage") r := h.repoMakeRole(h.randEmail()) @@ -495,6 +497,7 @@ func TestMemberAdd(t *testing.T) { h.apiInit(). Post(fmt.Sprintf("/roles/%d/member/%d", r.ID, u.ID)). + Header("Accept", "application/json"). Expect(t). Status(http.StatusOK). Assert(helpers.AssertNoErrors). @@ -503,6 +506,7 @@ func TestMemberAdd(t *testing.T) { func TestMemberRemove(t *testing.T) { h := newHelper(t) + helpers.AllowMe(h, types.UserRbacResource(0), "read") helpers.AllowMe(h, types.RoleRbacResource(0), "members.manage") r := h.repoMakeRole(h.randEmail()) @@ -510,6 +514,7 @@ func TestMemberRemove(t *testing.T) { h.apiInit(). Post(fmt.Sprintf("/roles/%d/member/%d", r.ID, u.ID)). + Header("Accept", "application/json"). Expect(t). Status(http.StatusOK). Assert(helpers.AssertNoErrors). diff --git a/tests/system/user_test.go b/tests/system/user_test.go index d2ce74b97..de64fed39 100644 --- a/tests/system/user_test.go +++ b/tests/system/user_test.go @@ -52,6 +52,8 @@ func TestUserRead(t *testing.T) { h := newHelper(t) h.clearUsers() + helpers.AllowMe(h, types.UserRbacResource(0), "read") + service.CurrentSettings.Privacy.Mask.Email = true service.CurrentSettings.Privacy.Mask.Name = true defer func() { @@ -602,6 +604,7 @@ func TestUserLabels(t *testing.T) { func TestUserMemberList(t *testing.T) { h := newHelper(t) h.clearUsers() + helpers.AllowMe(h, types.UserRbacResource(0), "read") helpers.AllowMe(h, types.RoleRbacResource(0), "read") u := h.createUserWithEmail(h.randEmail()) @@ -623,6 +626,7 @@ func TestUserMemberList(t *testing.T) { func TestUserMemberAdd(t *testing.T) { h := newHelper(t) h.clearUsers() + helpers.AllowMe(h, types.UserRbacResource(0), "read") helpers.AllowMe(h, types.RoleRbacResource(0), "members.manage") u := h.createUserWithEmail(h.randEmail()) @@ -641,6 +645,7 @@ func TestUserMemberAdd(t *testing.T) { func TestUserMemberRemove(t *testing.T) { h := newHelper(t) h.clearUsers() + helpers.AllowMe(h, types.UserRbacResource(0), "read") helpers.AllowMe(h, types.RoleRbacResource(0), "members.manage") u := h.createUserWithEmail(h.randEmail()) diff --git a/tests/workflows/invoker_and_runner_in_scope_test.go b/tests/workflows/invoker_and_runner_in_scope_test.go index aa5a4de4d..aa08314a2 100644 --- a/tests/workflows/invoker_and_runner_in_scope_test.go +++ b/tests/workflows/invoker_and_runner_in_scope_test.go @@ -8,6 +8,7 @@ import ( "github.com/cortezaproject/corteza-server/pkg/auth" "github.com/cortezaproject/corteza-server/pkg/rbac" sysTypes "github.com/cortezaproject/corteza-server/system/types" + "github.com/cortezaproject/corteza-server/tests/helpers" "github.com/stretchr/testify/require" ) @@ -41,7 +42,10 @@ func Test_invoker_and_runner_in_scope(t *testing.T) { wfInvoker.SetRoles(wfInvokers.ID) ctx = auth.SetIdentityToContext(ctx, wfInvoker) - rbac.Global().UpdateRoles(rbac.CommonRole.Make(wfInvokers.ID, wfInvokers.Handle)) + helpers.UpdateRBAC( + wfInvokers.ID, + ) + rbac.Global().Reload(ctx) t.Run("invoker set in scope", func(t *testing.T) { diff --git a/tests/workflows/main_test.go b/tests/workflows/main_test.go index 4de14047e..b8c1b4cf4 100644 --- a/tests/workflows/main_test.go +++ b/tests/workflows/main_test.go @@ -122,7 +122,6 @@ func bypassRBAC(ctx context.Context) context.Context { } u.SetRoles(auth.BypassRoles().IDs()...) - return auth.SetIdentityToContext(ctx, u) }