Skip to content

ROX-11070: Update group service and datastore to use ID instead of composite key. #2063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dhaus67 merged 10 commits into from
Jul 4, 2022
Merged

ROX-11070: Update group service and datastore to use ID instead of composite key. #2063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
57370c7
Update service and datastore
dhaus67 Jun 14, 2022
f430934
Add ID to groups resolver.
dhaus67 Jun 14, 2022
d134841
Do not fail early when auth provider ID is not set, only require the …
dhaus67 Jun 15, 2022
1e7a804
Address first review comments.
dhaus67 Jun 22, 2022
91519fd
Be more defensive with potential nil pointer, ensure validation is do…
dhaus67 Jun 23, 2022
f38c050
Ensure that the UI E2E tests intercept the groups batch request.
dhaus67 Jun 23, 2022
3787040
Address review comments (error class usage, splitting functions in st…
dhaus67 Jun 30, 2022
f7e0e79
Address minor review comments.
dhaus67 Jul 4, 2022
c5828e5
Fix unit tests after rebase.
dhaus67 Jul 4, 2022
d2b50ae
Add missing error case.
dhaus67 Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions central/graphql/resolvers/groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ func init() {
schema := getBuilder()
utils.Must(
schema.AddQuery("groups: [Group!]!"),
schema.AddQuery("group(authProviderId: String, key: String, value: String): Group"),
schema.AddQuery("group(authProviderId: String, key: String, value: String, id: String): Group"),
)
}

Expand All @@ -30,7 +30,7 @@ func (resolver *Resolver) Groups(ctx context.Context) ([]*groupResolver, error)
}

// Group returns a GraphQL resolver for the matching group, if it exists
func (resolver *Resolver) Group(ctx context.Context, args struct{ AuthProviderID, Key, Value *string }) (*groupResolver, error) {
func (resolver *Resolver) Group(ctx context.Context, args struct{ AuthProviderID, Key, Value, ID *string }) (*groupResolver, error) {
defer metrics.SetGraphQLOperationDurationTime(time.Now(), pkgMetrics.Root, "Group")

err := readGroups(ctx)
Expand All @@ -47,6 +47,9 @@ func (resolver *Resolver) Group(ctx context.Context, args struct{ AuthProviderID
if args.Value != nil {
props.Value = *args.Value
}
if args.ID != nil {
props.Id = *args.ID
}
grp, err := resolver.GroupDataStore.Get(ctx, props)
return resolver.wrapGroup(grp, grp != nil, err)
}
1 change: 0 additions & 1 deletion central/group/datastore/datastore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ type DataStore interface {

Add(ctx context.Context, group *storage.Group) error
Update(ctx context.Context, group *storage.Group) error
Upsert(ctx context.Context, group *storage.Group) error
Mutate(ctx context.Context, remove, update, add []*storage.Group) error
Remove(ctx context.Context, props *storage.GroupProperties) error
RemoveAllWithAuthProviderID(ctx context.Context, authProviderID string) error
Expand Down
52 changes: 44 additions & 8 deletions central/group/datastore/datastore_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/stackrox/rox/central/group/datastore/internal/store"
"github.com/stackrox/rox/central/role/resources"
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/sac"
)

Expand Down Expand Up @@ -65,6 +66,21 @@ func (ds *dataStoreImpl) Add(ctx context.Context, group *storage.Group) error {
return sac.ErrResourceAccessDenied
}

if err := ValidateGroup(group); err != nil {
return errox.InvalidArgs.CausedBy(err)
}

if group.GetProps().GetId() != "" {
return errox.InvalidArgs.Newf("id should be empty but %q was provided", group.GetProps().GetId())
}

if group.GetProps() != nil {
group.GetProps().Id = GenerateGroupID()
} else {
// Theoretically should never happen, as the auth provider ID is required to be set.
group.Props = &storage.GroupProperties{Id: GenerateGroupID()}
}

return ds.storage.Add(group)
}

Expand All @@ -75,24 +91,40 @@ func (ds *dataStoreImpl) Update(ctx context.Context, group *storage.Group) error
return sac.ErrResourceAccessDenied
}

if err := ValidateGroup(group); err != nil {
return errox.InvalidArgs.CausedBy(err)
}

return ds.storage.Update(group)
}

func (ds *dataStoreImpl) Upsert(ctx context.Context, group *storage.Group) error {
func (ds *dataStoreImpl) Mutate(ctx context.Context, remove, update, add []*storage.Group) error {
if ok, err := groupSAC.WriteAllowed(ctx); err != nil {
return err
} else if !ok {
return sac.ErrResourceAccessDenied
}

return ds.storage.Upsert(group)
}
for _, grp := range append(remove, update...) {
if err := ValidateGroup(grp); err != nil {
return errox.InvalidArgs.CausedBy(err)
}
}

func (ds *dataStoreImpl) Mutate(ctx context.Context, remove, update, add []*storage.Group) error {
if ok, err := groupSAC.WriteAllowed(ctx); err != nil {
return err
} else if !ok {
return sac.ErrResourceAccessDenied
for _, grp := range add {
if err := ValidateGroup(grp); err != nil {
return errox.InvalidArgs.CausedBy(err)
}

if grp.GetProps().GetId() != "" {
return errox.InvalidArgs.Newf("id should be empty but %q was provided", grp.GetProps().GetId())
}
if grp.GetProps() != nil {
grp.GetProps().Id = GenerateGroupID()
} else {
// Theoretically should never happen, as the auth provider ID is required to be set.
grp.Props = &storage.GroupProperties{Id: GenerateGroupID()}
}
}

return ds.storage.Mutate(remove, update, add)
Expand All @@ -105,6 +137,10 @@ func (ds *dataStoreImpl) Remove(ctx context.Context, props *storage.GroupPropert
return sac.ErrResourceAccessDenied
}

if err := ValidateProps(props); err != nil {
return errox.InvalidArgs.CausedBy(err)
}

return ds.storage.Remove(props)
}

Expand Down
125 changes: 89 additions & 36 deletions central/group/datastore/datastore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ func TestGroupDataStore(t *testing.T) {
suite.Run(t, new(groupDataStoreTestSuite))
}

var (
groupWithID = &storage.Group{Props: &storage.GroupProperties{
Id: "123",
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
)

type groupDataStoreTestSuite struct {
suite.Suite

Expand Down Expand Up @@ -127,99 +136,143 @@ func (s *groupDataStoreTestSuite) TestAllowsWalk() {
func (s *groupDataStoreTestSuite) TestEnforcesAdd() {
s.storage.EXPECT().Add(gomock.Any()).Times(0)

err := s.dataStore.Add(s.hasNoneCtx, &storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Add(s.hasNoneCtx, grp)
s.Error(err, "expected an error trying to write without permissions")

err = s.dataStore.Add(s.hasReadCtx, &storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Add(s.hasReadCtx, grp)
s.Error(err, "expected an error trying to write without permissions")
}

func (s *groupDataStoreTestSuite) TestAllowsAdd() {
s.storage.EXPECT().Add(gomock.Any()).Return(nil).Times(2)

err := s.dataStore.Add(s.hasWriteCtx, &storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Add(s.hasWriteCtx, grp)
s.NoError(err, "expected no error trying to write with permissions")

err = s.dataStore.Add(s.hasWriteAccessCtx, &storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Add(s.hasWriteAccessCtx, grp)
s.NoError(err, "expected no error trying to write with Access permissions")
}

func (s *groupDataStoreTestSuite) TestEnforcesUpdate() {
s.storage.EXPECT().Update(gomock.Any()).Times(0)

err := s.dataStore.Update(s.hasNoneCtx, &storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Update(s.hasNoneCtx, grp)
s.Error(err, "expected an error trying to write without permissions")

err = s.dataStore.Update(s.hasReadCtx, &storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Update(s.hasReadCtx, grp)
s.Error(err, "expected an error trying to write without permissions")
}

func (s *groupDataStoreTestSuite) TestAllowsUpdate() {
s.storage.EXPECT().Update(gomock.Any()).Return(nil).Times(2)

err := s.dataStore.Update(s.hasWriteCtx, &storage.Group{})
s.NoError(err, "expected no error trying to write with permissions")

err = s.dataStore.Update(s.hasWriteAccessCtx, &storage.Group{})
s.NoError(err, "expected no error trying to write with Access permissions")
}

func (s *groupDataStoreTestSuite) TestEnforcesUpsert() {
s.storage.EXPECT().Upsert(gomock.Any()).Times(0)

err := s.dataStore.Upsert(s.hasNoneCtx, &storage.Group{})
s.Error(err, "expected an error trying to write without permissions")

err = s.dataStore.Upsert(s.hasReadCtx, &storage.Group{})
s.Error(err, "expected an error trying to write without permissions")
}

func (s *groupDataStoreTestSuite) TestAllowsUpsert() {
s.storage.EXPECT().Upsert(gomock.Any()).Return(nil).Times(2)

err := s.dataStore.Upsert(s.hasWriteCtx, &storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Update(s.hasWriteCtx, grp)
s.NoError(err, "expected no error trying to write with permissions")

err = s.dataStore.Upsert(s.hasWriteAccessCtx, &storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Update(s.hasWriteAccessCtx, grp)
s.NoError(err, "expected no error trying to write with Access permissions")
}

func (s *groupDataStoreTestSuite) TestEnforcesMutate() {
s.storage.EXPECT().Mutate(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)

err := s.dataStore.Mutate(s.hasNoneCtx, []*storage.Group{}, []*storage.Group{}, []*storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Mutate(s.hasNoneCtx, []*storage.Group{groupWithID}, []*storage.Group{groupWithID},
[]*storage.Group{grp})
s.Error(err, "expected an error trying to write without permissions")

err = s.dataStore.Mutate(s.hasReadCtx, []*storage.Group{}, []*storage.Group{}, []*storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Mutate(s.hasReadCtx, []*storage.Group{groupWithID}, []*storage.Group{groupWithID},
[]*storage.Group{grp})
s.Error(err, "expected an error trying to write without permissions")
}

func (s *groupDataStoreTestSuite) TestAllowsMutate() {
s.storage.EXPECT().Mutate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(2)

err := s.dataStore.Mutate(s.hasWriteCtx, []*storage.Group{}, []*storage.Group{}, []*storage.Group{})
grp := &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err := s.dataStore.Mutate(s.hasWriteCtx, []*storage.Group{groupWithID}, []*storage.Group{groupWithID},
[]*storage.Group{grp})
s.NoError(err, "expected no error trying to write with permissions")

err = s.dataStore.Mutate(s.hasWriteAccessCtx, []*storage.Group{}, []*storage.Group{}, []*storage.Group{})
grp = &storage.Group{Props: &storage.GroupProperties{
AuthProviderId: "123",
Key: "123",
Value: "123",
}, RoleName: "123"}
err = s.dataStore.Mutate(s.hasWriteAccessCtx, []*storage.Group{groupWithID}, []*storage.Group{groupWithID},
[]*storage.Group{grp})
s.NoError(err, "expected no error trying to write with Access permissions")
}

func (s *groupDataStoreTestSuite) TestEnforcesRemove() {
s.storage.EXPECT().Remove(gomock.Any()).Times(0)

err := s.dataStore.Remove(s.hasNoneCtx, &storage.GroupProperties{})
err := s.dataStore.Remove(s.hasNoneCtx, groupWithID.GetProps())
s.Error(err, "expected an error trying to write without permissions")

err = s.dataStore.Remove(s.hasReadCtx, &storage.GroupProperties{})
err = s.dataStore.Remove(s.hasReadCtx, groupWithID.GetProps())
s.Error(err, "expected an error trying to write without permissions")
}

func (s *groupDataStoreTestSuite) TestAllowsRemove() {
s.storage.EXPECT().Remove(gomock.Any()).Return(nil).Times(2)

err := s.dataStore.Remove(s.hasWriteCtx, &storage.GroupProperties{})
err := s.dataStore.Remove(s.hasWriteCtx, groupWithID.GetProps())
s.NoError(err, "expected no error trying to write with permissions")

err = s.dataStore.Remove(s.hasWriteAccessCtx, &storage.GroupProperties{})
err = s.dataStore.Remove(s.hasWriteAccessCtx, groupWithID.GetProps())
s.NoError(err, "expected no error trying to write with Access permissions")
}
14 changes: 0 additions & 14 deletions central/group/datastore/internal/store/mocks/store.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 14 additions & 4 deletions central/group/datastore/internal/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,19 @@ package store
import (
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/bolthelper"
"github.com/stackrox/rox/pkg/utils"
bolt "go.etcd.io/bbolt"
)

var groupsBucket = []byte("groups2")

var isEmptyGroupPropertiesF = func(props *storage.GroupProperties) bool {
if props.GetAuthProviderId() == "" && props.GetKey() == "" && props.GetValue() == "" {
return true
}
return false
}

// Store updates and utilizes groups, which are attribute to role mappings.
//go:generate mockgen-wrapper
type Store interface {
Expand All @@ -19,7 +27,6 @@ type Store interface {

Add(*storage.Group) error
Update(*storage.Group) error
Upsert(*storage.Group) error
Mutate(remove, update, add []*storage.Group) error
Remove(props *storage.GroupProperties) error
}
Expand All @@ -31,9 +38,12 @@ func New(db *bolt.DB) Store {
store := &storeImpl{
db: db,
}

allEmptyGroupProperty := storage.GroupProperties{AuthProviderId: "", Key: "", Value: ""}
_ = store.Remove(&allEmptyGroupProperty) // ignore error to suppress warning
grps, err := store.GetFiltered(isEmptyGroupPropertiesF)
utils.Should(err)
for _, grp := range grps {
err = store.Remove(grp.GetProps())
utils.Should(err)
}

return store
}
Loading