feat(breadbox): Added checks for ensuring that group entries always match full email or email domain.

breadbox/breadbox/api/groups.py

@@ -156,6 +156,12 @@ def add_group_entry(
                 raise HTTPException(404)
             raise HTTPException(403)
 
+        if not group_entry.exact_match and not group_entry.email.startswith("@"):
+            raise HTTPException(
+                422,
+                "If exact_match is False, which indicates the entry applies to all email addresses within a domain, the email field must start with a '@'",
+            )
+
         group_entry_db = group_crud.add_group_entry(db, user, group, group_entry)
     return group_entry_db
 

breadbox/breadbox/crud/access_control.py

@@ -23,6 +23,14 @@ def user_has_access_to_group(
     if user in settings.admin_users:
         return True
 
+    # adding a special case for special groups, to avoid running into the assertion below
+    # that all email entries must start with a "@" if group_entry.exact_match == False
+    # We should clean up that entry because I think it'd be better if it doesn't exist -- but for now, let's avoid the data migration.
+    # This means that only the public group which is possible to be viewable by _all_ users. Similarly all users need to be
+    # able to access the transient group.
+    if group.id in [PUBLIC_GROUP_ID, TRANSIENT_GROUP_ID] and write_access is False:
+        return True
+
     # Check if user is in group's group entries
     if write_access:
         group_entries = [
@@ -37,10 +45,21 @@ def user_has_access_to_group(
         group_entries = group.group_entries
 
     for group_entry in group_entries:
-        if group_entry.exact_match and group_entry.email == user:
-            return True
-        elif not group_entry.exact_match and user.endswith(group_entry.email):
-            return True
+        if group_entry.exact_match:
+            if group_entry.email == user:
+                return True
+        else:
+            # I'm adding this check because I'm concerned that it is too easy to accidentally add a group entry which isn't specific enough.
+            # For example, if `group_entry.email == ""` then all users would match that. Or similarly, if someone
+            # added an entry with `group_entry.email == "apple.com"` then the email address `[email protected]` would
+            # also return true. To avoid these potential problems require that the suffix always starts with "@".
+            # This is checked when adding an entry, but assert that's the case just as a last sanity test
+            assert group_entry.email.startswith(
+                "@"
+            ), f"If group_entry.exact_match is False, we require the email address field contain the full domain name, starting with @ but the value was {repr(group_entry.email)}"
+            if user.endswith(group_entry.email):
+                return True
+
     return False
 
 

breadbox/tests/api/test_groups.py

@@ -166,7 +166,7 @@ def test_add_group_entry(self, client: TestClient, settings: Settings, minimal_d
             json={
                 "email": "[email protected]",
                 "access_type": "read",
-                "exact_match": False,
+                "exact_match": True,
             },
             headers=admin_headers,
         )
@@ -176,12 +176,28 @@ def test_add_group_entry(self, client: TestClient, settings: Settings, minimal_d
             json={
                 "email": "[email protected]",
                 "access_type": "read",
-                "exact_match": False,
+                "exact_match": True,
             },
             headers=admin_headers,
         )
         assert_status_ok(post_entry_1_copy)
 
+        # this should fail because the email doesn't start with '@'
+        post_suffix_match_entry = client.post(
+            f"/groups/{owner_group['id']}/addAccess",
+            json={"email": "email.com", "access_type": "read", "exact_match": False,},
+            headers=admin_headers,
+        )
+        assert_status_not_ok(post_suffix_match_entry)
+
+        # this should succeed because now that email starts with '@'
+        post_suffix_match_entry = client.post(
+            f"/groups/{owner_group['id']}/addAccess",
+            json={"email": "@email.com", "access_type": "read", "exact_match": False,},
+            headers=admin_headers,
+        )
+        assert_status_ok(post_suffix_match_entry)
+
 
 class TestDelete:
     def test_delete_group(
@@ -299,10 +315,11 @@ def test_delete_entry(self, client: TestClient, settings: Settings, minimal_db):
             json={
                 "email": "[email protected]",
                 "access_type": "read",
-                "exact_match": False,
+                "exact_match": True,
             },
             headers=admin_headers,
         )
+        assert_status_ok(post_read_entry)
         read_only_user_entry = post_read_entry.json()
         group = client.get(f"/groups/{owner_group['id']}", headers=admin_headers).json()
         assert len(group["group_entries"]) == 2
@@ -313,10 +330,11 @@ def test_delete_entry(self, client: TestClient, settings: Settings, minimal_db):
             json={
                 "email": "[email protected]",
                 "access_type": "write",
-                "exact_match": False,
+                "exact_match": True,
             },
             headers=admin_headers,
         )
+        assert_status_ok(post_write_entry)
         write_only_user_entry = post_write_entry.json()
         group = client.get(f"/groups/{owner_group['id']}", headers=admin_headers).json()
         assert len(group["group_entries"]) == 3
@@ -359,7 +377,7 @@ def read_only_delete():
             json={
                 "email": "[email protected]",
                 "access_type": "owner",
-                "exact_match": False,
+                "exact_match": True,
             },
             headers=admin_headers,
         )