Open michellemounde opened 7 months ago
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)])
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")
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"]
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 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!
Require 2 admin signoffs for changes to permissions. Fixes #2194