From d28ea130eeecc6c4a5042a95aa7d4e7875e0c171 Mon Sep 17 00:00:00 2001
From: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Date: Thu, 27 Apr 2023 10:37:07 -0400
Subject: [PATCH] Fix ListPermissions when separate id/type grants are used
 (#3183)

This makes two changes:

1. ListPermissions assumed every grant string had a type field, which
   does not match all grant formats. It now infers type from ID for
   formats which do not specify type.

2. Allow ID-only grant strings to be valid so long as the type specified
   matches the type of the specified ID.
---
 internal/perms/acl.go      |  4 +--
 internal/perms/acl_test.go | 51 +++++++++++++++++++++++++++++---------
 internal/perms/grants.go   |  2 +-
 3 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/internal/perms/acl.go b/internal/perms/acl.go
index f1c5b3701a..94ad3d1ba7 100644
--- a/internal/perms/acl.go
+++ b/internal/perms/acl.go
@@ -163,7 +163,7 @@ func (a ACL) Allowed(r Resource, aType action.Type, userId string, opt ...Option
 		case grant.id == r.Id &&
 			grant.id != "" &&
 			grant.id != "*" &&
-			grant.typ == resource.Unknown &&
+			(grant.typ == resource.Unknown || grant.typ == globals.ResourceTypeFromPrefix(grant.id)) &&
 			!action.List.IsActionOrParent(aType) &&
 			!action.Create.IsActionOrParent(aType):
 
@@ -262,7 +262,7 @@ func (a ACL) ListPermissions(requestedScopes map[string]*scopes.ScopeInfo,
 		grants := a.scopeMap[scopeId]
 		for _, grant := range grants {
 			// This grant doesn't match what we're looking for, ignore.
-			if grant.typ != requestedType && grant.typ != resource.All {
+			if grant.typ != requestedType && grant.typ != resource.All && globals.ResourceTypeFromPrefix(grant.id) != requestedType {
 				continue
 			}
 
diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go
index b7e70ef6bb..b798603ca2 100644
--- a/internal/perms/acl_test.go
+++ b/internal/perms/acl_test.go
@@ -382,13 +382,14 @@ func Test_ACLAllowed(t *testing.T) {
 
 func TestACL_ListPermissions(t *testing.T) {
 	tests := []struct {
-		name           string
-		userId         string
-		aclGrants      []scopeGrant
-		scopes         map[string]*scopes.ScopeInfo // *scopes.ScopeInfo isn't used at the moment.
-		resourceType   resource.Type
-		actionSet      action.ActionSet
-		expPermissions []Permission
+		name                        string
+		userId                      string
+		aclGrants                   []scopeGrant
+		scopes                      map[string]*scopes.ScopeInfo // *scopes.ScopeInfo isn't used at the moment.
+		resourceType                resource.Type
+		actionSet                   action.ActionSet
+		expPermissions              []Permission
+		skipGrantValidationChecking bool
 	}{
 		{
 			name: "Requested scope(s) not present in ACL scope map",
@@ -440,10 +441,11 @@ func TestACL_ListPermissions(t *testing.T) {
 					grants: []string{"type=*;actions=list,read"},
 				},
 			},
-			scopes:         map[string]*scopes.ScopeInfo{"o_1": nil},
-			resourceType:   resource.Session,
-			actionSet:      action.ActionSet{action.List, action.Read},
-			expPermissions: []Permission{},
+			scopes:                      map[string]*scopes.ScopeInfo{"o_1": nil},
+			resourceType:                resource.Session,
+			actionSet:                   action.ActionSet{action.List, action.Read},
+			expPermissions:              []Permission{},
+			skipGrantValidationChecking: true,
 		},
 		{
 			name: "Allow all ids",
@@ -777,6 +779,31 @@ func TestACL_ListPermissions(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:         "separate_type_id_resource_grants",
+			scopes:       map[string]*scopes.ScopeInfo{"p_1": nil},
+			resourceType: resource.Target,
+			actionSet:    action.ActionSet{action.Read, action.Cancel},
+			aclGrants: []scopeGrant{
+				{
+					scope: "p_1",
+					grants: []string{
+						"type=target;actions=list",
+						"id=ttcp_1234567890;actions=read",
+					},
+				},
+			},
+			expPermissions: []Permission{
+				{
+					ScopeId:     "p_1",
+					Resource:    resource.Target,
+					Action:      action.List,
+					ResourceIds: []string{"ttcp_1234567890"},
+					All:         false,
+					OnlySelf:    false,
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -788,7 +815,7 @@ func TestACL_ListPermissions(t *testing.T) {
 			var grants []Grant
 			for _, sg := range tt.aclGrants {
 				for _, g := range sg.grants {
-					grant, err := Parse(sg.scope, g, WithSkipFinalValidation(true))
+					grant, err := Parse(sg.scope, g, WithSkipFinalValidation(tt.skipGrantValidationChecking))
 					require.NoError(t, err)
 					grants = append(grants, grant)
 				}
diff --git a/internal/perms/grants.go b/internal/perms/grants.go
index 7cfdaa5c05..8d472625ab 100644
--- a/internal/perms/grants.go
+++ b/internal/perms/grants.go
@@ -483,7 +483,7 @@ func Parse(scopeId, grantString string, opt ...Option) (Grant, error) {
 				}
 			}
 			if !allowed {
-				return Grant{}, errors.NewDeprecated(errors.InvalidParameter, op, "parsed grant string would not result in any action being authorized")
+				return Grant{}, errors.NewDeprecated(errors.InvalidParameter, op, fmt.Sprintf("parsed grant string %q would not result in any action being authorized", grant.CanonicalString()))
 			}
 		}
 	}