From 4a61f4111f1d7bc38c2c6366db0575452daa631b Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Fri, 20 May 2022 10:26:57 +0200 Subject: [PATCH] Remove unused error from evaluator Evaluate (#49305) --- pkg/services/accesscontrol/evaluator.go | 32 ++++++++----------- pkg/services/accesscontrol/evaluator_test.go | 19 ++++------- pkg/services/accesscontrol/mock/mock.go | 2 +- .../ossaccesscontrol/ossaccesscontrol.go | 2 +- .../ngalert/api/authorization_test.go | 12 +++---- 5 files changed, 27 insertions(+), 40 deletions(-) diff --git a/pkg/services/accesscontrol/evaluator.go b/pkg/services/accesscontrol/evaluator.go index ac80a364a7b..67f43fc0495 100644 --- a/pkg/services/accesscontrol/evaluator.go +++ b/pkg/services/accesscontrol/evaluator.go @@ -12,7 +12,7 @@ var logger = log.New("accesscontrol.evaluator") type Evaluator interface { // Evaluate permissions that are grouped by action - Evaluate(permissions map[string][]string) (bool, error) + Evaluate(permissions map[string][]string) bool // MutateScopes executes a sequence of ScopeModifier functions on all embedded scopes of an evaluator and returns a new Evaluator MutateScopes(ctx context.Context, mutate ScopeAttributeMutator) (Evaluator, error) // String returns a string representation of permission required by the evaluator @@ -32,25 +32,25 @@ type permissionEvaluator struct { Scopes []string } -func (p permissionEvaluator) Evaluate(permissions map[string][]string) (bool, error) { +func (p permissionEvaluator) Evaluate(permissions map[string][]string) bool { userScopes, ok := permissions[p.Action] if !ok { - return false, nil + return false } if len(p.Scopes) == 0 { - return true, nil + return true } for _, target := range p.Scopes { for _, scope := range userScopes { if match(scope, target) { - return true, nil + return true } } } - return false, nil + return false } func match(scope, target string) bool { @@ -114,13 +114,13 @@ type allEvaluator struct { allOf []Evaluator } -func (a allEvaluator) Evaluate(permissions map[string][]string) (bool, error) { +func (a allEvaluator) Evaluate(permissions map[string][]string) bool { for _, e := range a.allOf { - if ok, err := e.Evaluate(permissions); !ok || err != nil { - return false, err + if !e.Evaluate(permissions) { + return false } } - return true, nil + return true } func (a allEvaluator) MutateScopes(ctx context.Context, mutate ScopeAttributeMutator) (Evaluator, error) { @@ -164,17 +164,13 @@ type anyEvaluator struct { anyOf []Evaluator } -func (a anyEvaluator) Evaluate(permissions map[string][]string) (bool, error) { +func (a anyEvaluator) Evaluate(permissions map[string][]string) bool { for _, e := range a.anyOf { - ok, err := e.Evaluate(permissions) - if err != nil { - return false, err - } - if ok { - return true, nil + if e.Evaluate(permissions) { + return true } } - return false, nil + return false } func (a anyEvaluator) MutateScopes(ctx context.Context, mutate ScopeAttributeMutator) (Evaluator, error) { diff --git a/pkg/services/accesscontrol/evaluator_test.go b/pkg/services/accesscontrol/evaluator_test.go index b15c260e14b..d37e2d5838e 100644 --- a/pkg/services/accesscontrol/evaluator_test.go +++ b/pkg/services/accesscontrol/evaluator_test.go @@ -52,8 +52,7 @@ func TestPermission_Evaluate(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - ok, err := test.evaluator.Evaluate(test.permissions) - assert.NoError(t, err) + ok := test.evaluator.Evaluate(test.permissions) assert.Equal(t, test.expected, ok) }) } @@ -123,8 +122,7 @@ func TestPermission_Inject(t *testing.T) { t.Run(test.desc, func(t *testing.T) { injected, err := test.evaluator.MutateScopes(context.TODO(), ScopeInjector(test.params)) assert.NoError(t, err) - ok, err := injected.Evaluate(test.permissions) - assert.NoError(t, err) + ok := injected.Evaluate(test.permissions) assert.Equal(t, test.expected, ok) }) } @@ -172,8 +170,7 @@ func TestAll_Evaluate(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - ok, err := test.evaluator.Evaluate(test.permissions) - assert.NoError(t, err) + ok := test.evaluator.Evaluate(test.permissions) assert.Equal(t, test.expected, ok) }) } @@ -236,7 +233,7 @@ func TestAll_Inject(t *testing.T) { t.Run(test.desc, func(t *testing.T) { injected, err := test.evaluator.MutateScopes(context.TODO(), ScopeInjector(test.params)) assert.NoError(t, err) - ok, err := injected.Evaluate(test.permissions) + ok := injected.Evaluate(test.permissions) assert.NoError(t, err) assert.Equal(t, test.expected, ok) }) @@ -283,8 +280,7 @@ func TestAny_Evaluate(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - ok, err := test.evaluator.Evaluate(test.permissions) - assert.NoError(t, err) + ok := test.evaluator.Evaluate(test.permissions) assert.Equal(t, test.expected, ok) }) } @@ -347,7 +343,7 @@ func TestAny_Inject(t *testing.T) { t.Run(test.desc, func(t *testing.T) { injected, err := test.evaluator.MutateScopes(context.TODO(), ScopeInjector(test.params)) assert.NoError(t, err) - ok, err := injected.Evaluate(test.permissions) + ok := injected.Evaluate(test.permissions) assert.NoError(t, err) assert.Equal(t, test.expected, ok) }) @@ -409,8 +405,7 @@ func TestEval(t *testing.T) { for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - ok, err := test.evaluator.Evaluate(test.permissions) - assert.NoError(t, err) + ok := test.evaluator.Evaluate(test.permissions) assert.Equal(t, test.expected, ok) }) } diff --git a/pkg/services/accesscontrol/mock/mock.go b/pkg/services/accesscontrol/mock/mock.go index 9e5b42f79ec..abfedb651d9 100644 --- a/pkg/services/accesscontrol/mock/mock.go +++ b/pkg/services/accesscontrol/mock/mock.go @@ -103,7 +103,7 @@ func (m *Mock) Evaluate(ctx context.Context, user *models.SignedInUser, evaluato if err != nil { return false, err } - return resolvedEvaluator.Evaluate(accesscontrol.GroupScopesByAction(permissions)) + return resolvedEvaluator.Evaluate(accesscontrol.GroupScopesByAction(permissions)), nil } // GetUserPermissions returns user permissions. diff --git a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go index 49b4d04de5b..cba70501b1e 100644 --- a/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go +++ b/pkg/services/accesscontrol/ossaccesscontrol/ossaccesscontrol.go @@ -98,7 +98,7 @@ func (ac *OSSAccessControlService) Evaluate(ctx context.Context, user *models.Si if err != nil { return false, err } - return resolvedEvaluator.Evaluate(user.Permissions[user.OrgId]) + return resolvedEvaluator.Evaluate(user.Permissions[user.OrgId]), nil } // GetUserRoles returns user permissions based on built-in roles diff --git a/pkg/services/ngalert/api/authorization_test.go b/pkg/services/ngalert/api/authorization_test.go index b7d0b869206..8ad6cc55fa8 100644 --- a/pkg/services/ngalert/api/authorization_test.go +++ b/pkg/services/ngalert/api/authorization_test.go @@ -189,9 +189,8 @@ func TestAuthorizeRuleChanges(t *testing.T) { groupChanges := testCase.changes() result, err := authorizeRuleChanges(groupChanges, func(evaluator ac.Evaluator) bool { - response, err := evaluator.Evaluate(make(map[string][]string)) + response := evaluator.Evaluate(make(map[string][]string)) require.False(t, response) - require.NoError(t, err) executed = true return false }) @@ -202,9 +201,8 @@ func TestAuthorizeRuleChanges(t *testing.T) { permissions := testCase.permissions(groupChanges) executed = false result, err = authorizeRuleChanges(groupChanges, func(evaluator ac.Evaluator) bool { - response, err := evaluator.Evaluate(permissions) + response := evaluator.Evaluate(permissions) require.Truef(t, response, "provided permissions [%v] is not enough for requested permissions [%s]", testCase.permissions, evaluator.GoString()) - require.NoError(t, err) executed = true return true }) @@ -359,8 +357,7 @@ func TestAuthorizeRuleDelete(t *testing.T) { groupChanges := testCase.changes() permissions := testCase.permissions(groupChanges) result, err := authorizeRuleChanges(groupChanges, func(evaluator ac.Evaluator) bool { - response, err := evaluator.Evaluate(permissions) - require.NoError(t, err) + response := evaluator.Evaluate(permissions) return response }) @@ -401,9 +398,8 @@ func TestCheckDatasourcePermissionsForRule(t *testing.T) { executed := 0 eval := authorizeDatasourceAccessForRule(rule, func(evaluator ac.Evaluator) bool { - response, err := evaluator.Evaluate(permissions) + response := evaluator.Evaluate(permissions) require.Truef(t, response, "provided permissions [%v] is not enough for requested permissions [%s]", permissions, evaluator.GoString()) - require.NoError(t, err) executed++ return true })