Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin signoffs for changes to permissions #3088

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

michellemounde
Copy link
Contributor

@michellemounde michellemounde commented Jan 16, 2024

Require 2 admin signoffs for changes to permissions. Fixes #2194

@gabrielBusta gabrielBusta self-assigned this Jan 23, 2024
@gabrielBusta gabrielBusta added admin admin app & api (aus4-admin.mozilla.org) permissions permissions and roles labels Jan 23, 2024
@gabrielBusta
Copy link
Member

diff --git a/tests/test_db.py b/tests/test_db.py
index b8477ffc..f520bd46 100644
--- a/tests/test_db.py
+++ b/tests/test_db.py
@@ -5362,7 +5362,13 @@ class TestPermissions(unittest.TestCase, MemoryDatabaseMixin):
         self.assertEqual(set(history_columns), set(expected))
 
     def testGrantPermissions(self):
-        self.permissions.insert("bob", username="cathy", permission="release", options=dict(products=["SeaMonkey"]))
+        self.permissions.insert(
+            "bob",
+            username="cathy",
+            signoffs=[{"sc_id": 1, "username": "bill", "role": "admin"}, {"sc_id": 1, "username": "zawadi", "role": "admin"}],
+            permission="release",
+            options=dict(products=["SeaMonkey"]),
+        )
         query = self.permissions.t.select().where(self.permissions.username == "cathy")
         query = query.where(self.permissions.permission == "release")
         self.assertEqual(query.execute().fetchall(), [("release", "cathy", dict(products=["SeaMonkey"]), 1)])

@gabrielBusta
Copy link
Member

gabrielBusta commented Jan 25, 2024

To debug the tests, you can run this script in the root of your clone of balrog to create a container using the CI's test image. Then, inside the container you can run the tests and use the builtin Python debugger (here's a nice tutorial on it.) To set breakpoints in the tests module you can raise an exception (instead of calling pytest.set_trace, which seems to be causing weird behavior on the db module's objects). For example:

diff --git a/tests/test_db.py b/tests/test_db.py
index b8477ffc..84e98564 100644
--- a/tests/test_db.py
+++ b/tests/test_db.py
@@ -5362,6 +5362,7 @@ class TestPermissions(unittest.TestCase, MemoryDatabaseMixin):
         self.assertEqual(set(history_columns), set(expected))
 
     def testGrantPermissions(self):
+        raise Exception("breakpoint")
         self.permissions.insert("bob", username="cathy", permission="release", options=dict(products=["SeaMonkey"]))
         query = self.permissions.t.select().where(self.permissions.username == "cathy")
         query = query.where(self.permissions.permission == "release")

@michellemounde michellemounde force-pushed the permission-changes-admin-signoffs branch from bd6e7c5 to acc8f7c Compare January 31, 2024 08:22
@gabrielBusta
Copy link
Member

diff --git a/src/auslib/db.py b/src/auslib/db.py
index 3861d5c6..91a46b1f 100644
--- a/src/auslib/db.py
+++ b/src/auslib/db.py
@@ -2649,7 +2649,7 @@ class Permissions(AUSTable):
             if not row:
                 continue
             else:
-                potential_required_signoffs["rs"].extend([dict(permission="admin", signoffs_required=2)])
+                potential_required_signoffs["rs"].extend(self.db.permissionsRequiredSignoffs)
         return potential_required_signoffs
 
     def assertPermissionExists(self, permission):
@@ -3016,6 +3016,7 @@ class AUSDatabase(object):
             self.setDburi(dburi, mysql_traditional_mode, releases_history_buckets, releases_history_class, async_releases_history_class)
         self.log = logging.getLogger(self.__class__.__name__)
         self.systemAccounts = []
+        self.permissionsRequiredSignoffs = []
 
     def setDburi(
         self,
@@ -3133,9 +3134,8 @@ class AUSDatabase(object):
     def productRequiredSignoffs(self):
         return self.productRequiredSignoffsTable
 
-    @property
-    def permissionsRequiredSignoffs(self):
-        return self.permissionsRequiredSignoffsTable
+    def setPermissionsRequiredSignoffs(self, permissionsRequiredSignoffs):
+        self.permissionsRequiredSignoffs = permissionsRequiredSignoffs
 
     @property
     def dockerflow(self):
diff --git a/uwsgi/admin.wsgi b/uwsgi/admin.wsgi
index 2daed094..314e23b0 100644
--- a/uwsgi/admin.wsgi
+++ b/uwsgi/admin.wsgi
@@ -46,6 +46,10 @@ if STAGING or LOCALDEV:
         }
     )
 
+if LOCALDEV:
+    PERMISSIONS_REQUIRED_SIGNOFFS = [{"permission": "admin", "signoffs_required": 1}]
+else:
+    PERMISSIONS_REQUIRED_SIGNOFFS = [{"permission": "admin", "signoffs_required": 2}]
 
 # Logging needs to be set-up before importing the application to make sure that
 # logging done from other modules uses our Logger.
@@ -148,6 +152,7 @@ else:
 dbo.setDb(os.environ["DBURI"], buckets)
 dbo.setSystemAccounts(SYSTEM_ACCOUNTS)
 dbo.setDomainAllowlist(DOMAIN_ALLOWLIST)
+dbo.setPermissionsRequiredSignoffs(PERMISSIONS_REQUIRED_SIGNOFFS)
 application.config["ALLOWLISTED_DOMAINS"] = DOMAIN_ALLOWLIST
 application.config["PAGE_TITLE"] = "Balrog Administration"
 application.config["SECRET_KEY"] = os.environ["SECRET_KEY"]

@michellemounde michellemounde marked this pull request as ready for review February 1, 2024 16:52
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand: this patch is adding the general ability to require signoffs based on permissions (even though we're only specifying one admin signoff for permission changes at the moment).

Assuming that's right, does this mean that we could theoretically set up required signoffs from roles and permissions in the same place?

@michellemounde
Copy link
Contributor Author

michellemounde commented Feb 26, 2024

Just to make sure I understand: this patch is adding the general ability to require signoffs based on permissions (even though we're only specifying one admin signoff for permission changes at the moment).

Assuming that's right, does this mean that we could theoretically set up required signoffs from roles and permissions in the same place?

@bhearsum Not with the way I have currently implemented the change it checks for either the permission or the signoff. But it could be modified to check a signoff against both.

@bhearsum
Copy link
Contributor

@bhearsum Not with the way I have currently implemented the change it checks for either the permission or the signoff. But it could be modified to check a signoff against both.

OK, that's fine. That's for the explanation!

@michellemounde michellemounde force-pushed the permission-changes-admin-signoffs branch from c0b2b74 to 2369aa1 Compare February 29, 2024 17:58
@bhearsum bhearsum removed their request for review June 4, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin admin app & api (aus4-admin.mozilla.org) permissions permissions and roles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require 2 admin signoffs for changes to permissions
3 participants