From 093226a0571a45836ac40ac54147a68fda4ab4ec Mon Sep 17 00:00:00 2001 From: Ryan Derr <54389874+RyanDerr@users.noreply.github.com> Date: Fri, 2 May 2025 13:12:32 -0500 Subject: [PATCH] ListResolvableAliasesPermissions Hotfix (#5673) * ICU-16921 Fix ListResolvableAliasesPermissions where function skips some permission building when all children scope is provided, this currently results in skipping subsequent direct grants. Also, update listResolvableAliasesRepo to return early with no data when splitting permissions yields no relevant permissions. * Remove conditional split permission check for listResolvableAliasesRefresh and listRemovedResolvableAliasIds as condition is never reachable. Additionally remove unecessary grants being given to list targets as part of TestACL_ListResolvableAliasesPermissions. * Remove additonal 'type=target;actions=list' grants as part of TestACL_ListResolvableAliasesPermissions * Update Switch Statement Too Factor For Edge Case With Children Granted With Role Scope Global * Resolve comment to refactor error return in listResolvableAliases to instead return no data and document why and when this can occur. * Add unit tests to cover case where provided permissions to ListResolvableAliases service contains no grant scopes where targets can reside. --- .../repository_alias_list_resolvable.go | 6 +- .../service_list_resolvable_ext_test.go | 28 +++ internal/perms/acl.go | 16 +- internal/perms/acl_test.go | 195 ++++++++++++++++-- 4 files changed, 222 insertions(+), 23 deletions(-) diff --git a/internal/alias/target/repository_alias_list_resolvable.go b/internal/alias/target/repository_alias_list_resolvable.go index a9b144b8d..bec1bcdff 100644 --- a/internal/alias/target/repository_alias_list_resolvable.go +++ b/internal/alias/target/repository_alias_list_resolvable.go @@ -115,8 +115,12 @@ func (r *Repository) listResolvableAliases(ctx context.Context, permissions []pe destinationIdClauses = append(destinationIdClauses, "destination_id in (select public_id from target where project_id = any(@target_scope_ids))") args = append(args, sql.Named("target_scope_ids", "{"+strings.Join(directScopeIds, ",")+"}")) } + // This condition checks if there are no destinations target Ids and no child scopes, + // while also ensuring that the "allDescendants" flag is not set, if so we return no data. + // An example scenario of when this can happen is when a role on the global scope grants a user + // access to list aliases and read targets, but only within the global scope and its immediate children. if len(destinationIdClauses) == 0 && len(childAllScopes) == 0 { - return nil, time.Time{}, errors.New(ctx, errors.InvalidParameter, op, "no target ids or scope ids provided") + return nil, time.Time{}, nil } whereClause = fmt.Sprintf("destination_id is not null and (%s)", strings.Join(destinationIdClauses, " or ")) } diff --git a/internal/alias/target/service_list_resolvable_ext_test.go b/internal/alias/target/service_list_resolvable_ext_test.go index 48d6be51c..ec687fd0d 100644 --- a/internal/alias/target/service_list_resolvable_ext_test.go +++ b/internal/alias/target/service_list_resolvable_ext_test.go @@ -88,6 +88,27 @@ func TestService_ListResolvableAliases(t *testing.T) { All: true, }, } + + // Test the scenario where permissions are provided to the ListResolvableAliases service, + // but none of the permissions include grant scopes corresponding to scopes of which targets reside. + noTargetScopePerms := []perms.Permission{ + { + RoleScopeId: scope.Global.String(), + GrantScopeId: globals.GrantScopeChildren, + Resource: resource.Target, + Action: action.ListResolvableAliases, + OnlySelf: false, + All: true, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: scope.Global.String(), + Resource: resource.Target, + Action: action.ListResolvableAliases, + OnlySelf: false, + All: true, + }, + } // Reverse since we read items in descending order (newest first) slices.Reverse(byScopeResources) @@ -159,6 +180,13 @@ func TestService_ListResolvableAliases(t *testing.T) { _, err := target.ListResolvableAliases(ctx, []byte("some hash"), 1, repo, nil) require.ErrorContains(t, err, "missing target permissions") }) + t.Run("no valid target permissions", func(t *testing.T) { + t.Parallel() + res, err := target.ListResolvableAliases(ctx, []byte("some hash"), 1, repo, noTargetScopePerms) + require.NoError(t, err) + require.NotNil(t, res) + require.Empty(t, res.Items) + }) }) t.Run("ListPage validation", func(t *testing.T) { t.Parallel() diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 694c6cff8..d5c65a230 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -468,21 +468,21 @@ func (a ACL) ListResolvableAliasesPermissions(requestedType resource.Type, actio // so skip it continue } - default: - // Since direct grants must be the same scope or downstream, the - // only possibility left for a children grant is that the parent is - // global and the grant is on the org -- if it was for projects it - // would need to be a descendants grant - if _, ok := childrenScopes[scope.Global.String()]; ok { - // We already looked at this scope in the children grants, so skip it + case p.RoleScopeId == scope.Global.String() && strings.HasPrefix(grantScopeId, scope.Org.Prefix()): + // Handle the case where the parent scope is global, and the grant is at the org level. + // Direct grants must either match the parent scope or be downstream. This condition + // accounts for a scenario where a child grant exists at the org level while the parent + // is global. If the grant were for projects, it would require a descendants grant instead. + // Skip processing if the current scope is already accounted for in the childrenScopes map. + if _, ok := childrenScopes[p.RoleScopeId]; ok { continue } } - if a.buildPermission(&scopes.ScopeInfo{Id: grantScopeId}, requestedType, actions, false, &p) { perms = append(perms, p) } } + return perms } diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index 02cebf3cd..889216458 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -830,7 +830,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleScope: "global", grantScope: globals.GrantScopeDescendants, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -856,7 +855,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleScope: "global", grantScope: globals.GrantScopeDescendants, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -864,12 +862,20 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleScope: "global", grantScope: "global", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, }, expPermissions: []Permission{ + { + RoleScopeId: scope.Global.String(), + GrantScopeId: scope.Global.String(), + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, { RoleScopeId: scope.Global.String(), GrantScopeId: globals.GrantScopeDescendants, @@ -879,6 +885,22 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { All: false, OnlySelf: false, }, + }, + }, + { + name: "global_with_this_no_descendants", + resourceType: resource.Target, + actionSet: action.NewActionSet(action.Read, action.Cancel), + aclGrants: []scopeGrant{ + { + roleScope: "global", + grantScope: "global", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + }, + expPermissions: []Permission{ { RoleScopeId: scope.Global.String(), GrantScopeId: scope.Global.String(), @@ -899,7 +921,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleScope: "global", grantScope: globals.GrantScopeChildren, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -916,6 +937,161 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { }, }, }, + { + name: "global_with_this_with_valid_children_and_direct_grant", + resourceType: resource.Target, + actionSet: action.NewActionSet(action.Read, action.Cancel), + aclGrants: []scopeGrant{ + { + roleScope: "global", + grantScope: "global", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + { + roleScope: "global", + grantScope: globals.GrantScopeChildren, + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + { + roleScope: "global", + grantScope: "p_1", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + }, + expPermissions: []Permission{ + { + RoleScopeId: scope.Global.String(), + GrantScopeId: scope.Global.String(), + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: globals.GrantScopeChildren, + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: "p_1", + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + }, + }, + { + name: "global_with_this_with_and_direct_grant", + resourceType: resource.Target, + actionSet: action.NewActionSet(action.Read, action.Cancel), + aclGrants: []scopeGrant{ + { + roleScope: "global", + grantScope: "global", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + { + roleScope: "global", + grantScope: "p_1", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + }, + expPermissions: []Permission{ + { + RoleScopeId: scope.Global.String(), + GrantScopeId: scope.Global.String(), + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: "p_1", + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + }, + }, + { + name: "global_with_this_with_multiple_direct_grants", + resourceType: resource.Target, + actionSet: action.NewActionSet(action.Read, action.Cancel), + aclGrants: []scopeGrant{ + { + roleScope: "global", + grantScope: "global", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + { + roleScope: "global", + grantScope: "o_1", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + { + roleScope: "global", + grantScope: "p_1", + grants: []string{ + "ids=ttcp_1234567890;actions=read", + }, + }, + }, + expPermissions: []Permission{ + { + RoleScopeId: scope.Global.String(), + GrantScopeId: scope.Global.String(), + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: "o_1", + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + { + RoleScopeId: scope.Global.String(), + GrantScopeId: "p_1", + Resource: resource.Target, + Action: action.ListResolvableAliases, + ResourceIds: []string{"ttcp_1234567890"}, + All: false, + OnlySelf: false, + }, + }, + }, { name: "org_no_this_with_children_and_direct_grant", resourceType: resource.Target, @@ -926,7 +1102,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: globals.GrantScopeChildren, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -935,7 +1110,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: "o_2", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -973,7 +1147,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: globals.GrantScopeChildren, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -982,7 +1155,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: "o_1", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1020,7 +1192,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: globals.GrantScopeChildren, grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1029,7 +1200,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: scope.Global.String(), grantScope: "o_1", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1038,7 +1208,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: "o_1", grantScope: "p_1a", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1047,7 +1216,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: "o_1", grantScope: "p_1b", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1056,7 +1224,6 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { roleParentScopeId: "o_2", grantScope: "p_2", grants: []string{ - "type=target;actions=list", "ids=ttcp_1234567890;actions=read", }, }, @@ -1112,7 +1279,7 @@ func TestACL_ListResolvableAliasesPermissions(t *testing.T) { acl := NewACL(grants...) perms := acl.ListResolvableAliasesPermissions(tt.resourceType, tt.actionSet) - require.ElementsMatch(t, tt.expPermissions, perms) + assert.ElementsMatch(t, tt.expPermissions, perms) }) } }