ACLs Should Honor Most Permissive Access Level (#1089)

Co-authored-by: Ryan Emerle <ryan_emerle@comcast.com>

modules/api/src/test/functional/tests/recordsets/update_recordset_test.py

@@ -821,9 +821,9 @@ def test_user_rule_priority_over_group_acl_rule(shared_zone_test_context):
 
 
 @pytest.mark.serial
-def test_more_restrictive_acl_rule_priority(shared_zone_test_context):
+def test_more_permissive_acl_rule_priority(shared_zone_test_context):
     """
-    Test more restrictive rule takes priority
+    Test more permissive rule takes priority
     """
     ok_zone = shared_zone_test_context.ok_zone
     client = shared_zone_test_context.ok_vinyldns_client
@@ -832,14 +832,14 @@ def test_more_restrictive_acl_rule_priority(shared_zone_test_context):
         read_rule = generate_acl_rule("Read", userId="dummy")
         write_rule = generate_acl_rule("Write", userId="dummy")
 
-        result_rs = seed_text_recordset(client, "test_more_restrictive_acl_rule_priority", ok_zone)
+        result_rs = seed_text_recordset(client, "test_more_permissive_acl_rule_priority", ok_zone)
         result_rs["ttl"] = result_rs["ttl"] + 1000
 
-        # add rules
+        # add rules 
         add_ok_acl_rules(shared_zone_test_context, [read_rule, write_rule])
 
-        # Dummy user cannot update record
-        shared_zone_test_context.dummy_vinyldns_client.update_recordset(result_rs, status=403)
+        # Dummy user can update record
+        shared_zone_test_context.dummy_vinyldns_client.update_recordset(result_rs, status=202)
     finally:
         clear_ok_acl_rules(shared_zone_test_context)
         if result_rs:

modules/api/src/test/scala/vinyldns/api/domain/access/AccessValidationsSpec.scala

@@ -18,7 +18,6 @@ package vinyldns.api.domain.access
 
 import cats.scalatest.EitherMatchers
 import org.joda.time.DateTime
-
 import org.scalatest.matchers.should.Matchers
 import org.scalatest.wordspec.AnyWordSpec
 import vinyldns.api.{ResultHelpers, VinylDNSTestHelpers}
@@ -44,8 +43,14 @@ class AccessValidationsSpec
     ACLRule(AccessLevel.Read, userId = Some(okAuth.userId), groupId = None)
   private val userWriteAcl =
     ACLRule(AccessLevel.Write, userId = Some(okAuth.userId), groupId = None)
+  private val userNoAccessAcl =
+    ACLRule(AccessLevel.NoAccess, userId = Some(okAuth.userId), groupId = None)
+  private val userDeleteAcl =
+    ACLRule(AccessLevel.Delete, userId = Some(okAuth.userId), groupId = None)
   private val groupReadAcl = ACLRule(AccessLevel.Read, userId = None, groupId = Some(okGroup.id))
   private val groupWriteAcl = ACLRule(AccessLevel.Write, userId = None, groupId = Some(okGroup.id))
+  private val groupDeleteAcl = ACLRule(AccessLevel.Delete, userId = None, groupId = Some(okGroup.id))
+  private val groupAclNone = ACLRule(AccessLevel.NoAccess, userId = None, groupId = Some(okGroup.id))
   private val allReadACL = ACLRule(AccessLevel.Read, userId = None, groupId = None)
 
   private val badUserWriteAcl = ACLRule(AccessLevel.Write, userId = Some("bad-id"), groupId = None)
@@ -741,8 +746,8 @@ class AccessValidationsSpec
       result shouldBe AccessLevel.Write
     }
 
-    "choose group over all-user" in {
-      val zoneAcl = ZoneACL(Set(groupWriteAcl, allReadACL))
+    "prioritize more permissive user rules in a tie" in {
+      val zoneAcl = ZoneACL(Set(userReadAcl, userWriteAcl))
       val zone = Zone("name", "email", acl = zoneAcl)
 
       val mockRecordSet = mock[RecordSet]
@@ -751,8 +756,8 @@ class AccessValidationsSpec
       result shouldBe AccessLevel.Write
     }
 
-    "prioritize more restrictive user rules in a tie" in {
-      val zoneAcl = ZoneACL(Set(userReadAcl, userWriteAcl))
+    "prioritize user over group rules, regardless of permissiveness" in {
+      val zoneAcl = ZoneACL(Set(userReadAcl, groupWriteAcl))
       val zone = Zone("name", "email", acl = zoneAcl)
 
       val mockRecordSet = mock[RecordSet]
@@ -761,6 +766,56 @@ class AccessValidationsSpec
       result shouldBe AccessLevel.Read
     }
 
+    "prioritize NoAccess over other user rules" in {
+      val zoneAcl = ZoneACL(Set(userNoAccessAcl, userReadAcl))
+      val zone = Zone("name", "email", acl = zoneAcl)
+
+      val mockRecordSet = mock[RecordSet]
+      val result =
+        accessValidationTest.getAccessFromAcl(okAuth, mockRecordSet.name, mockRecordSet.typ, zone)
+      result shouldBe AccessLevel.NoAccess
+    }
+
+    "prioritize user over group rules, even when group rule is NoAccess" in {
+      val zoneAcl = ZoneACL(Set(userDeleteAcl, groupAclNone))
+      val zone = Zone("name", "email", acl = zoneAcl)
+
+      val mockRecordSet = mock[RecordSet]
+      val result =
+        accessValidationTest.getAccessFromAcl(okAuth, mockRecordSet.name, mockRecordSet.typ, zone)
+      result shouldBe AccessLevel.Delete
+    }
+
+    "prioritize user over group rules, and more permissive user rules in a tie" in {
+      val zoneAcl = ZoneACL(Set(userWriteAcl, userReadAcl, groupDeleteAcl))
+      val zone = Zone("name", "email", acl = zoneAcl)
+
+      val mockRecordSet = mock[RecordSet]
+      val result =
+        accessValidationTest.getAccessFromAcl(okAuth, mockRecordSet.name, mockRecordSet.typ, zone)
+      result shouldBe AccessLevel.Write
+    }
+
+    "prioritize user over group rules, and choose NoAccess over other rules" in {
+      val zoneAcl = ZoneACL(Set(userNoAccessAcl, userReadAcl, groupDeleteAcl))
+      val zone = Zone("name", "email", acl = zoneAcl)
+
+      val mockRecordSet = mock[RecordSet]
+      val result =
+        accessValidationTest.getAccessFromAcl(okAuth, mockRecordSet.name, mockRecordSet.typ, zone)
+      result shouldBe AccessLevel.NoAccess
+    }
+
+    "choose group over all-user" in {
+      val zoneAcl = ZoneACL(Set(groupWriteAcl, allReadACL))
+      val zone = Zone("name", "email", acl = zoneAcl)
+
+      val mockRecordSet = mock[RecordSet]
+      val result =
+        accessValidationTest.getAccessFromAcl(okAuth, mockRecordSet.name, mockRecordSet.typ, zone)
+      result shouldBe AccessLevel.Write
+    }
+
     "apply to specific record type" in {
       val rs = RecordSet(
         zoneNotAuthorized.id,

modules/core/src/main/scala/vinyldns/core/domain/zone/ACLRule.scala

@@ -21,7 +21,7 @@ import vinyldns.core.domain.record.RecordType.RecordType
 
 object AccessLevel extends Enumeration {
   type AccessLevel = Value
-  val NoAccess, Read, Write, Delete = Value
+  val NoAccess, Delete, Write, Read = Value
 }
 
 case class ACLRule(

modules/docs/src/main/mdoc/permissions.md

@@ -67,6 +67,10 @@ ACL rules provide an extremely flexible way to grant access to DNS records.  Eac
 
 Depending on your organization, you may choose to exclusively use _Zone Ownership_ for managing self-service.  This does come at a higher cost of administrative overhead than other options; however, it is the most restrictive and still very flexible.
 
+Users and groups can have multiple ACL entries associated with them. Permission conflicts are resolved via the following rules:
+- The **MOST** permissive rule is chosen in the case of a tie. For example, if a user has an ACL entry with a `Write` permission, and another with a `Read` permission, the `Write` permission will take precedence.
+- `NoAccess` takes precedence over other permission levels. For example, if a user has three ACL rules, `Read`, `Write`, and `NoAccess`, `NoAccess` will take precedence.
+- `User` rules take precedence over `group` rules. For example, if a user has an ACL entry with a `Read` permission, and a group ACL entry with a `Write` permission, the `Read` permission will take precedence, even though `Write` is more permissive.
 # Shared Zone Ownership
 
 Shared Zones were introduced in order to alleviate the administrative burden of Zone Ownership for large organizations.  If you mark a zone as `Shared` (via the Manage Zone tab or API), you effectively grant anyone who can login to VinylDNS permissions to manage records in that DNS Zone.  Read this section closely, as it can be a little confusing.