Skip to content

Commit

Permalink
ROX-18391: cleanup permission checker
Browse files Browse the repository at this point in the history 
## Description

When we started PG migration project we were unsure in what direction it will go with permission checker. This resulted in a safe approach to support everything we could think of. After time we can cleanup permission checker interface and limit it to only two methods.

## Checklist
- [ ] Investigated and inspected CI test results
- [ ] Unit test and regression tests added
- [ ] Evaluated and added CHANGELOG entry if required
- [ ] Determined and documented upgrade steps
- [ ] Documented user facing changes (create PR based on [openshift/openshift-docs](https://github.com/openshift/openshift-docs) and merge into [rhacs-docs](https://github.com/openshift/openshift-docs/tree/rhacs-docs))

If any of these don't apply, please comment below.

## Testing Performed

CI
  • Loading branch information
@janisz
janisz committed Jul 14, 2023
1 parent a33e7d6 commit 286a940
Show file tree
Hide file tree
Showing 9 changed files with 27 additions and 234 deletions.
22 changes: 0 additions & 22 deletions central/clusterinit/store/postgres/permission_checker.go
View file

This file was deleted.

42 changes: 5 additions & 37 deletions central/clusterinit/store/postgres/permission_checker_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,18 @@ import (
accessPkg "github.com/stackrox/rox/central/clusterinit/backend/access"
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/sac"
pgSearch "github.com/stackrox/rox/pkg/search/postgres"
"github.com/stackrox/rox/pkg/sync"
)

type permissionChecker struct{}

var (
once sync.Once
instance PermissionChecker
instance pgSearch.PermissionChecker
)

func permissionCheckerSingleton() PermissionChecker {
func permissionCheckerSingleton() pgSearch.PermissionChecker {
once.Do(func() {
instance = permissionChecker{}
})
Expand All @@ -33,42 +33,10 @@ func checkAccess(ctx context.Context, access storage.Access) (bool, error) {
return err == nil, err
}

func (permissionChecker) CountAllowed(ctx context.Context) (bool, error) {
func (permissionChecker) ReadAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}

func (permissionChecker) ExistsAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}

func (permissionChecker) GetAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}

func (permissionChecker) UpsertAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return checkAccess(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionChecker) UpsertManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return checkAccess(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionChecker) DeleteAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
func (permissionChecker) WriteAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionChecker) GetIDsAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}

func (permissionChecker) GetManyAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}

func (permissionChecker) DeleteManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return checkAccess(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionChecker) WalkAllowed(ctx context.Context) (bool, error) {
return checkAccess(ctx, storage.Access_READ_ACCESS)
}
22 changes: 0 additions & 22 deletions central/networkgraph/entity/datastore/internal/store/postgres/permission_checker.go
View file

This file was deleted.

41 changes: 5 additions & 36 deletions central/networkgraph/entity/datastore/internal/store/postgres/permission_checker_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,20 @@ import (
"github.com/stackrox/rox/pkg/auth/permissions"
"github.com/stackrox/rox/pkg/sac"
"github.com/stackrox/rox/pkg/sac/effectiveaccessscope"
pgSearch "github.com/stackrox/rox/pkg/search/postgres"
"github.com/stackrox/rox/pkg/sync"
)

type permissionCheckerImpl struct{}

var (
once sync.Once
instance PermissionChecker
instance pgSearch.PermissionChecker

networkGraphSAC = sac.ForResource(resources.NetworkGraph)
)

func permissionCheckerSingleton() PermissionChecker {
func permissionCheckerSingleton() pgSearch.PermissionChecker {
once.Do(func() {
instance = permissionCheckerImpl{}
})
Expand All @@ -45,42 +46,10 @@ func genericPassThrough(ctx context.Context, access storage.Access) (bool, error
return true, nil
}

func (permissionCheckerImpl) CountAllowed(ctx context.Context) (bool, error) {
func (permissionCheckerImpl) ReadAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) ExistsAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) GetAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) WalkAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) UpsertAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionCheckerImpl) UpsertManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionCheckerImpl) DeleteAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_WRITE_ACCESS)
}

func (permissionCheckerImpl) GetIDsAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) GetManyAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_ACCESS)
}

func (permissionCheckerImpl) DeleteManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
func (permissionCheckerImpl) WriteAllowed(ctx context.Context) (bool, error) {
return genericPassThrough(ctx, storage.Access_READ_WRITE_ACCESS)
}
22 changes: 0 additions & 22 deletions central/vulnerabilityrequest/datastore/internal/store/postgres/permission_checker.go
View file

This file was deleted.

41 changes: 5 additions & 36 deletions central/vulnerabilityrequest/datastore/internal/store/postgres/permission_checker_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/stackrox/rox/central/role/resources"
"github.com/stackrox/rox/pkg/sac"
pgSearch "github.com/stackrox/rox/pkg/search/postgres"
"github.com/stackrox/rox/pkg/sync"
)

Expand All @@ -13,54 +14,22 @@ type permissionChecker struct{}

var (
once sync.Once
instance PermissionChecker
instance pgSearch.PermissionChecker

requesterOrApproverSAC = sac.ForResources(sac.ForResource(resources.VulnerabilityManagementRequests), sac.ForResource(resources.VulnerabilityManagementApprovals))
)

func permissionCheckerSingleton() PermissionChecker {
func permissionCheckerSingleton() pgSearch.PermissionChecker {
once.Do(func() {
instance = permissionChecker{}
})
return instance
}

func (permissionChecker) CountAllowed(ctx context.Context) (bool, error) {
func (permissionChecker) ReadAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}

func (permissionChecker) ExistsAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}

func (permissionChecker) GetAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}

func (permissionChecker) UpsertAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return requesterOrApproverSAC.WriteAllowedToAny(ctx)
}

func (permissionChecker) UpsertManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return requesterOrApproverSAC.WriteAllowedToAny(ctx)
}

func (permissionChecker) DeleteAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
func (permissionChecker) WriteAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.WriteAllowedToAny(ctx)
}

func (permissionChecker) GetIDsAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}

func (permissionChecker) GetManyAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}

func (permissionChecker) DeleteManyAllowed(ctx context.Context, _ ...sac.ScopeKey) (bool, error) {
return requesterOrApproverSAC.WriteAllowedToAny(ctx)
}

func (permissionChecker) WalkAllowed(ctx context.Context) (bool, error) {
return requesterOrApproverSAC.ReadAllowedToAny(ctx)
}
29 changes: 12 additions & 17 deletions pkg/search/postgres/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,8 @@ type Deleter interface {

// PermissionChecker is a permission checker that could be used by GenericStore
type PermissionChecker interface {
CountAllowed(ctx context.Context) (bool, error)
DeleteAllowed(ctx context.Context, keys ...sac.ScopeKey) (bool, error)
DeleteManyAllowed(ctx context.Context, keys ...sac.ScopeKey) (bool, error)
ExistsAllowed(ctx context.Context) (bool, error)
GetAllowed(ctx context.Context) (bool, error)
UpsertAllowed(ctx context.Context, keys ...sac.ScopeKey) (bool, error)
WalkAllowed(ctx context.Context) (bool, error)
ReadAllowed(ctx context.Context) (bool, error)
WriteAllowed(ctx context.Context) (bool, error)
}

type primaryKeyGetter[T any, PT unmarshaler[T]] func(obj PT) string
Expand Down Expand Up @@ -123,7 +118,7 @@ func (s *GenericStore[T, PT]) Exists(ctx context.Context, id string) (bool, erro

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.ExistsAllowed(ctx); err != nil {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil {
return false, err
} else if !ok {
return false, nil
Expand Down Expand Up @@ -153,7 +148,7 @@ func (s *GenericStore[T, PT]) Count(ctx context.Context) (int, error) {

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.CountAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return 0, err
}
} else {
Expand All @@ -171,7 +166,7 @@ func (s *GenericStore[T, PT]) Count(ctx context.Context) (int, error) {
func (s *GenericStore[T, PT]) Walk(ctx context.Context, fn func(obj PT) error) error {
var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.WalkAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return err
}
} else {
Expand Down Expand Up @@ -221,7 +216,7 @@ func (s *GenericStore[T, PT]) Get(ctx context.Context, id string) (PT, bool, err

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.GetAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return nil, false, err
}
} else {
Expand Down Expand Up @@ -251,7 +246,7 @@ func (s *GenericStore[T, PT]) GetByQuery(ctx context.Context, query *v1.Query) (

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.GetAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return nil, err
}
} else {
Expand Down Expand Up @@ -284,7 +279,7 @@ func (s *GenericStore[T, PT]) GetIDs(ctx context.Context) ([]string, error) {
defer s.setPostgresOperationDurationTime(time.Now(), ops.GetAll)
var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.GetAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return nil, err
}
} else {
Expand Down Expand Up @@ -317,7 +312,7 @@ func (s *GenericStore[T, PT]) GetMany(ctx context.Context, identifiers []string)

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.GetAllowed(ctx); err != nil || !ok {
if ok, err := s.permissionChecker.ReadAllowed(ctx); err != nil || !ok {
return nil, nil, err
}
} else {
Expand Down Expand Up @@ -367,7 +362,7 @@ func (s *GenericStore[T, PT]) DeleteByQuery(ctx context.Context, query *v1.Query

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.DeleteAllowed(ctx); err != nil {
if ok, err := s.permissionChecker.WriteAllowed(ctx); err != nil {
return err
} else if !ok {
return sac.ErrResourceAccessDenied
Expand Down Expand Up @@ -400,7 +395,7 @@ func (s *GenericStore[T, PT]) DeleteMany(ctx context.Context, identifiers []stri

var sacQueryFilter *v1.Query
if s.hasPermissionsChecker() {
if ok, err := s.permissionChecker.DeleteManyAllowed(ctx); err != nil {
if ok, err := s.permissionChecker.WriteAllowed(ctx); err != nil {
return err
} else if !ok {
return sac.ErrResourceAccessDenied
Expand Down Expand Up @@ -513,7 +508,7 @@ func (s *GenericStore[T, PT]) permissionCheckerAllowsUpsert(ctx context.Context)
if !s.hasPermissionsChecker() {
return utils.ShouldErr(errInvalidOperation)
}
allowed, err := s.permissionChecker.UpsertAllowed(ctx)
allowed, err := s.permissionChecker.WriteAllowed(ctx)
if err != nil {
return err
}
Expand Down

0 comments on commit 286a940

Please sign in to comment.