From 44d597cff11237ac841c2235c3bc73761f7ea9e2 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Tue, 22 Dec 2020 15:22:33 +0100 Subject: [PATCH] Fix user data masking on search --- store/rdbms/users.go | 11 ++-- system/service/user.go | 31 ++++++++--- system/service/user_test.go | 103 ++++++++++++++++++++++++++++++++++++ system/types/user.go | 3 ++ tests/system/user_test.go | 8 ++- 5 files changed, 144 insertions(+), 12 deletions(-) create mode 100644 system/service/user_test.go diff --git a/store/rdbms/users.go b/store/rdbms/users.go index 53862e37d..8a4765e75 100644 --- a/store/rdbms/users.go +++ b/store/rdbms/users.go @@ -15,7 +15,7 @@ func (s Store) convertUserFilter(f types.UserFilter) (query squirrel.SelectBuild query = filter.StateCondition(query, "usr.suspended_at", f.Suspended) if len(f.UserID) > 0 { - query = query.Where(squirrel.Eq{"usr.ID": f.UserID}) + query = query.Where(squirrel.Eq{"usr.id": f.UserID}) } if len(f.LabeledIDs) > 0 { @@ -38,11 +38,12 @@ func (s Store) convertUserFilter(f types.UserFilter) (query squirrel.SelectBuild query = query.Where(squirrel.Or{ squirrel.Like{"usr.username": qs}, squirrel.Like{"usr.handle": qs}, - }) - } - if f.Email != "" { - query = query.Where(squirrel.Eq{"usr.email": f.Email}) + // do a lookup on a potentially masked fields + // this should be filtered out in the check function + squirrel.Like{"usr.email": qs}, + squirrel.Like{"usr.name": qs}, + }) } if f.Email != "" { diff --git a/system/service/user.go b/system/service/user.go index b36ba9efd..acd9f0f25 100644 --- a/system/service/user.go +++ b/system/service/user.go @@ -275,7 +275,7 @@ func (svc user) proc(u *types.User, err error) (*types.User, error) { return nil, err } - svc.handlePrivateData(u) + svc.handlePrivateData(svc.ctx, u) return u, nil } @@ -289,12 +289,23 @@ func (svc user) Find(filter types.UserFilter) (uu types.UserSet, f types.UserFil ) // For each fetched item, store backend will check if it is valid or not + filter.MaskedEmailsEnabled = svc.settings.Privacy.Mask.Email + filter.MaskedNamesEnabled = svc.settings.Privacy.Mask.Name filter.Check = func(res *types.User) (bool, error) { if !svc.ac.CanReadUser(svc.ctx, res) { return false, nil } - svc.handlePrivateData(res) + if svc.maskEmail(svc.ctx, res) && ((len(filter.Query) > 0 && strings.HasPrefix(res.Email, filter.Query)) || res.Email == filter.Email) { + // user email matched but it will be masked later on, so exclude it to prevent data probing + return false, nil + } + + if svc.maskName(svc.ctx, res) && (len(filter.Query) > 0 && strings.HasPrefix(res.Name, filter.Query)) { + // user mail matched but it will be masked later on, so exclude it to prevent data probing + return false, nil + } + return true, nil } @@ -338,7 +349,7 @@ func (svc user) Find(filter types.UserFilter) (uu types.UserSet, f types.UserFil } return uu.Walk(func(u *types.User) error { - svc.handlePrivateData(u) + svc.handlePrivateData(svc.ctx, u) return nil }) }() @@ -661,16 +672,24 @@ func (svc user) SetPassword(userID uint64, newPassword string) (err error) { } // Masks (or leaves as-is) private data on user -func (svc user) handlePrivateData(u *types.User) { - if !svc.ac.CanUnmaskEmail(svc.ctx, u) { +func (svc user) handlePrivateData(ctx context.Context, u *types.User) { + if svc.maskEmail(ctx, u) { u.Email = maskPrivateDataEmail } - if !svc.ac.CanUnmaskName(svc.ctx, u) { + if svc.maskName(ctx, u) { u.Name = maskPrivateDataName } } +func (svc user) maskEmail(ctx context.Context, u *types.User) bool { + return svc.settings.Privacy.Mask.Email && !svc.ac.CanUnmaskEmail(ctx, u) +} + +func (svc user) maskName(ctx context.Context, u *types.User) bool { + return svc.settings.Privacy.Mask.Name && !svc.ac.CanUnmaskName(ctx, u) +} + // Preloader collects all ids of users, loads them and sets them back // // diff --git a/system/service/user_test.go b/system/service/user_test.go new file mode 100644 index 000000000..96e01b779 --- /dev/null +++ b/system/service/user_test.go @@ -0,0 +1,103 @@ +package service + +import ( + "context" + a "github.com/cortezaproject/corteza-server/pkg/auth" + "github.com/cortezaproject/corteza-server/pkg/eventbus" + "github.com/cortezaproject/corteza-server/pkg/rbac" + "github.com/cortezaproject/corteza-server/store" + "github.com/cortezaproject/corteza-server/store/sqlite3" + "github.com/cortezaproject/corteza-server/system/types" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "testing" +) + +// Mock auth service with nil for current time, dummy provider validator and mock db +func makeMockUserService() *user { + var ( + ctx = context.Background() + + mem, err = sqlite3.ConnectInMemory(ctx) + + svc = &user{ + settings: &types.AppSettings{}, + ac: AccessControl(rbac.NewService(zap.NewNop(), mem)), + eventbus: eventbus.New(), + } + ) + + if err != nil { + panic(err) + } + + if err = store.Upgrade(ctx, zap.NewNop(), mem); err != nil { + panic(err) + } + + svc.store = mem + + return svc +} + +func TestUser_ProtectedSearch(t *testing.T) { + const testRoleID = 123 + + var ( + req = require.New(t) + ctx = context.Background() + + masked = &types.User{Email: "email.masked@us.er", Name: "Name MSKD", ID: nextID(), CreatedAt: *now()} + unmasked = &types.User{Email: "email.unmasked@us.er", Name: "Name UNMSKD", ID: nextID(), CreatedAt: *now()} + + set types.UserSet + err error + + testUser = &types.User{ID: 42} + ) + + testUser.SetRoles([]uint64{testRoleID}) + ctx = a.SetIdentityToContext(ctx, testUser) + + svc := makeMockUserService() + + svc.ac.(*accessControl).permissions.Grant(ctx, svc.ac.(*accessControl).Whitelist(), + rbac.AllowRule(testRoleID, (&types.User{}).RBACResource().AppendWildcard(), "read"), + rbac.DenyRule(testRoleID, masked.RBACResource(), "unmask.email"), + rbac.AllowRule(testRoleID, unmasked.RBACResource(), "unmask.email"), + rbac.DenyRule(testRoleID, masked.RBACResource(), "unmask.name"), + rbac.AllowRule(testRoleID, unmasked.RBACResource(), "unmask.name"), + ) + req.NoError(store.CreateUser(ctx, svc.store, masked, unmasked)) + + t.Run("with disabled masking", func(t *testing.T) { + // Masking disabled, expecting to fetch both users + svc.settings.Privacy.Mask.Email = false + svc.settings.Privacy.Mask.Name = false + set, _, err = svc.With(ctx).Find(types.UserFilter{Query: "email"}) + req.NoError(err) + req.Len(set, 2) + req.NotEqual(set[0].Email, maskPrivateDataEmail) + req.NotEqual(set[1].Email, maskPrivateDataEmail) + }) + + t.Run("with enabled privacy", func(t *testing.T) { + // Masking enabled, expecting to fetch only unmasked + svc.settings.Privacy.Mask.Email = true + svc.settings.Privacy.Mask.Name = true + + set, _, err = svc.With(ctx).Find(types.UserFilter{Query: "email"}) + req.NoError(err) + req.Len(set, 1) + }) + + t.Run("email search with enabled privacy", func(t *testing.T) { + // Masking enabled, expecting to fetch only unmasked + svc.settings.Privacy.Mask.Email = true + svc.settings.Privacy.Mask.Name = true + + set, _, err = svc.With(ctx).Find(types.UserFilter{Email: "email.masked@us.er"}) + req.NoError(err) + req.Len(set, 0) + }) +} diff --git a/system/types/user.go b/system/types/user.go index d48ae1cf5..873311d0a 100644 --- a/system/types/user.go +++ b/system/types/user.go @@ -64,6 +64,9 @@ type ( // Store then loads additional resources to satisfy the paging parameters Check func(*User) (bool, error) `json:"-"` + MaskedEmailsEnabled bool `json:"-"` + MaskedNamesEnabled bool `json:"-"` + // Standard helpers for paging and sorting filter.Sorting filter.Paging diff --git a/tests/system/user_test.go b/tests/system/user_test.go index 696d4576b..3bbf581d3 100644 --- a/tests/system/user_test.go +++ b/tests/system/user_test.go @@ -45,6 +45,13 @@ func TestUserRead(t *testing.T) { h := newHelper(t) h.clearUsers() + service.CurrentSettings.Privacy.Mask.Email = true + service.CurrentSettings.Privacy.Mask.Name = true + defer func() { + service.CurrentSettings.Privacy.Mask.Email = false + service.CurrentSettings.Privacy.Mask.Name = false + }() + u := h.createUserWithEmail(h.randEmail()) h.apiInit(). @@ -88,7 +95,6 @@ func TestUserListAll(t *testing.T) { Assert(helpers.AssertNoErrors). Assert(jsonpath.Present(`$.response.filter`)). Assert(jsonpath.Present(`$.response.set`)). - Assert(jsonpath.Len(`$.response.set`, seedCount)). End() }