Apply suggestions from code review

Co-authored-by: John Schaeffer <[email protected]>
Signed-off-by: Bailin He <[email protected]>

.gitignore

@@ -17,6 +17,19 @@
 # Emacs stuff
 *~
 
+# vscode stuff
+.vscode/*
+!.vscode/settings.json
+!.vscode/tasks.json
+!.vscode/extensions.json
+!.vscode/*.code-snippets
+
+# Local History for Visual Studio Code
+.history/
+
+# Built Visual Studio Code Extensions
+*.vsix
+
 # .tools dir
 .tools/
 

internal/iapl/policy.go

@@ -167,15 +167,6 @@ func (v *policy) validateUnions() error {
 func (v *policy) validateResourceTypes() error {
 	for _, resourceType := range v.rt {
 		for _, rel := range resourceType.Relationships {
-			// two kinds of relationships target types to be validated
-			// 1. simple target type names
-			for _, tt := range rel.TargetTypes {
-				if _, ok := v.rt[tt.Name]; !ok {
-					return fmt.Errorf("%s: relationships: %s: %w", resourceType.Name, tt.Name, ErrorUnknownType)
-				}
-			}
-
-			// 2. target types with optional subject relation or subject identifier
 			for _, tt := range rel.TargetTypes {
 				if _, ok := v.rt[tt.Name]; !ok {
 					return fmt.Errorf("%s: relationships: %s: %w", resourceType.Name, tt.Name, ErrorUnknownType)
@@ -210,6 +201,17 @@ func (v *policy) validateConditionRelationshipAction(rt ResourceType, c Conditio
 		return fmt.Errorf("%s: %w", c.Relation, ErrorUnknownRelation)
 	}
 
+	// if there's a relationship action defined with only the relation,
+	// e.g.,
+	// ```yaml
+	// actionname: someaction
+	// typename: someresource
+	// conditions:
+	//   - relationshipaction:
+	//       relation: some_relation
+	// ```
+	// the above logics ensure that `some_relation` exists, thus can safely
+	// return without error
 	if c.ActionName == "" {
 		return nil
 	}
@@ -271,62 +273,20 @@ func (v *policy) validateActionBindings() error {
 	return nil
 }
 
-// validateMemberRoleRelationship ensures that there's a valid `member_role`
-// relationship exists in a given resource type
-func (v *policy) validateMemberRoleRelationship(res ResourceType) error {
-	relationshipExists := false
-	targetOk := false
-
-	for _, rel := range res.Relationships {
-		if rel.Relation != RoleOwnerMemberRoleRelation {
-			continue
-		}
-
-		relationshipExists = true
-
-		for i := 0; i < len(rel.TargetTypes) && !targetOk; i++ {
-			if rel.TargetTypes[i].Name == v.p.RBAC.RoleResource.Name {
-				targetOk = true
-			}
-		}
-
-		if !targetOk {
-			break
-		}
-	}
-
-	if !relationshipExists || !targetOk {
-		return fmt.Errorf(
-			"%w: role owner %s must have %s relation to %s",
-			ErrorMissingRelationship, res.Name, RoleOwnerMemberRoleRelation,
-			v.p.RBAC.RoleResource,
-		)
-	}
-
-	return nil
-}
-
 // validateRoles validates V2 role resource types to ensure that:
-//  1. role resource type has a valid owner relationship
-//  2. role-owner resource types have a valid member-role relationship points
-//     to the role resource
+//   - role resource type has a valid owner relationship
 func (v *policy) validateRoles() error {
 	if v.p.RBAC == nil {
 		return nil
 	}
 
 	for _, roleOwnerName := range v.p.RBAC.RoleOwners {
-		owner, ok := v.rt[roleOwnerName]
+		_, ok := v.rt[roleOwnerName]
 
 		// check if role owner exists
 		if !ok {
 			return fmt.Errorf("%w: role owner %s does not exist", ErrorUnknownType, roleOwnerName)
 		}
-
-		// role owner must have `member_role` relationship to `role` resource
-		if err := v.validateMemberRoleRelationship(owner); err != nil {
-			return err
-		}
 	}
 
 	return nil
@@ -364,6 +324,18 @@ func (v *policy) expandActionBindings() {
 // createV2RoleResourceType creates a v2 role resource type contains a list of relationships
 // representing all the actions, as well as relationships and permissions for
 // the management of the roles themselves.
+// This is equivalent to including a resource that looks like:
+//
+//	name: rolev2
+//	idprefix: permrv2
+//	relationships:
+//	  - relation: owner
+//	    targettypes:
+//	      - name: tenant
+//	  - relation: foo_resource_get_rel
+//	    targettypes:
+//	      - name: user
+//	        subjectidentifier: "*"
 func (v *policy) createV2RoleResourceType() {
 	role := ResourceType{
 		Name:     v.p.RBAC.RoleResource.Name,
@@ -408,7 +380,37 @@ func (v *policy) createV2RoleResourceType() {
 // createRoleBindingResourceType creates a role-binding resource type contains
 // a list of all the actions.
 // The role-binding resources will be used to create a 3-way relationship
-// between a resource, a subject and a role
+// between a resource, a subject and a role.
+//
+// this function effectively creates:
+// 1. a resource type like this:
+//
+//	name: rolebinding
+//	idprefix: permrbn
+//	relationships:
+//	  - relation: rolev2
+//	    targettypes:
+//	      - name: rolev2
+//	  - relation: subject
+//	    targettypes:
+//	      - name: user
+//	        subjectidentifier: "*"
+//
+// 2. a list of action-bindings representing permissions for all the actions in the policy
+//
+//	actionbindings:
+//	  - actionname: foo_resource_get
+//	    typename: rolebinding
+//	    conditionsets:
+//	      - conditions:
+//	        - relationshipaction:
+//	            relation: rolev2
+//	            actionname: foo_resource_get_rel
+//	      - conditions:
+//	        - relationshipaction:
+//	            relation: subject
+//	   # ... other action bindings on the rolebinding resource for each
+//	   # action defined in the policy
 func (v *policy) createRoleBindingResourceType() {
 	rolebinding := ResourceType{
 		Name:     v.p.RBAC.RoleBindingResource.Name,
@@ -469,7 +471,44 @@ func (v *policy) createRoleBindingResourceType() {
 // types that has `ResourceRoleBindingV2` defined.
 // Relationships like member_roles, available_roles, are created to support
 // role inheritance, e.g., an org should be able to use roles defined by its
-// parents
+// parents.
+// This function augments the resource types and effectively creating
+// resource types like this:
+//
+// 1. for resource owners, `member_role` relationship is added
+//
+//	```diff
+//	 resourcetypes:
+//	   name: tenant
+//	   idprefix: tnntten
+//	   rolebindingv2:
+//	     *rolesFromParent
+//	   relationships:
+//	     - relation: parent
+//	       targettypes:
+//	         - name: tenant_parent
+//	     # ... other existing relationships
+//	+	  - relation: member_role
+//	+	    targettypes:
+//	+	      - name: rolev2
+//
+//	```
+//
+// 2. for resources that inherit permissions from other resources, `available_roles`
+// action is added
+//
+//	```diff
+//	  actionbindings:
+//	  # ... other existing action bindings
+//	+   - actionname: available_roles
+//	+     typename: tenant
+//	+     conditions:
+//	+       - relationshipaction:
+//	+           relation: member_role
+//	+       - relationshipaction:
+//	+           relation: parent
+//	+           actionname: available_roles
+//	```
 func (v *policy) expandRBACV2Relationships() {
 	for name, resourceType := range v.rt {
 		// not all roles are available for all resources, available roles are
@@ -503,7 +542,7 @@ func (v *policy) expandRBACV2Relationships() {
 				availableRoles = append(availableRoles, Condition{
 					RelationshipAction: &ConditionRelationshipAction{
 						Relation:   from,
-						ActionName: AvailableRoleRelation,
+						ActionName: AvailableRolesList,
 					},
 				})
 			}
@@ -512,7 +551,7 @@ func (v *policy) expandRBACV2Relationships() {
 		// create available role permission
 		if len(availableRoles) > 0 {
 			action := ActionBinding{
-				ActionName: AvailableRoleRelation,
+				ActionName: AvailableRolesList,
 				TypeName:   resourceType.Name,
 				Conditions: availableRoles,
 			}

internal/iapl/policy_test.go

@@ -420,8 +420,8 @@ func TestPolicy(t *testing.T) {
 			Name: "RBAC_OK",
 			Input: PolicyDocument{
 				RBAC: &RBAC{
-					RoleResource:        rbacResourceDefinition{"rolev2", "permrv2"},
-					RoleBindingResource: rbacResourceDefinition{"role_binding", "permrbn"},
+					RoleResource:        RBACResourceDefinition{"rolev2", "permrv2"},
+					RoleBindingResource: RBACResourceDefinition{"role_binding", "permrbn"},
 					RoleSubjectTypes:    []string{"user"},
 					RoleOwners:          []string{"tenant"},
 					RoleBindingSubjects: []types.TargetType{{Name: "user"}},
@@ -458,8 +458,8 @@ func TestPolicy(t *testing.T) {
 
 func defaultRBAC() RBAC {
 	return RBAC{
-		RoleResource:        rbacResourceDefinition{"rolev2", "permrv2"},
-		RoleBindingResource: rbacResourceDefinition{"role_binding", "permrbn"},
+		RoleResource:        RBACResourceDefinition{"rolev2", "permrv2"},
+		RoleBindingResource: RBACResourceDefinition{"role_binding", "permrbn"},
 		RoleSubjectTypes:    []string{"user", "client"},
 		RoleOwners:          []string{"tenant"},
 		RoleBindingSubjects: []types.TargetType{{Name: "user"}, {Name: "client"}, {Name: "group", SubjectRelation: "member"}},

internal/iapl/rbac.go

@@ -10,9 +10,9 @@ const (
 	// RoleOwnerMemberRoleRelation is the name of the relationship that connects a resource
 	// to a role that it owns
 	RoleOwnerMemberRoleRelation = "member_role"
-	// AvailableRoleRelation is the name of the relationship that list all roles
-	// that are available for a given resource
-	AvailableRoleRelation = "avail_role"
+	// AvailableRolesList is the name of the action in a resource that returns a list
+	// of roles that are available for the resource
+	AvailableRolesList = "avail_role"
 	// RolebindingRoleRelation is the name of the relationship that connects a role binding to a role.
 	RolebindingRoleRelation = "role"
 	// RolebindingSubjectRelation is the name of the relationship that connects a role binding to a subject.
@@ -106,9 +106,9 @@ in IAPL policy terms:
 */
 type RBAC struct {
 	// RoleResource is the name of the resource type that represents a role.
-	RoleResource rbacResourceDefinition
+	RoleResource RBACResourceDefinition
 	// RoleBindingResource is the name of the resource type that represents a role binding.
-	RoleBindingResource rbacResourceDefinition
+	RoleBindingResource RBACResourceDefinition
 	// RoleSubjectTypes is a list of subject types that the relationships in a
 	// role resource will contain, see the example above.
 	RoleSubjectTypes []string
@@ -128,9 +128,9 @@ type RBAC struct {
 	roleownersset map[string]struct{}
 }
 
-// rbacResourceDefinition is a struct to define a resource type for a role
+// RBACResourceDefinition is a struct to define a resource type for a role
 // and role-bindings
-type rbacResourceDefinition struct {
+type RBACResourceDefinition struct {
 	Name     string
 	IDPrefix string
 }
@@ -204,7 +204,7 @@ func (r *RBAC) RoleBindingActions() []Action {
 		actions = append(actions, Action{Name: string(action)})
 	}
 
-	actions = append(actions, Action{Name: AvailableRoleRelation})
+	actions = append(actions, Action{Name: AvailableRolesList})
 
 	return actions
 }