feat(permissions): add a command to reconcile the groups with their f… - #268
Open
mateodurante wants to merge 1 commit into
Open
feat(permissions): add a command to reconcile the groups with their f…#268mateodurante wants to merge 1 commit into
mateodurante wants to merge 1 commit into
Conversation
…ixture The permissions of the roles live in ngen/fixtures/group.json, but that fixture is only loaded on a brand new installation: loaddatafirsttime skips everything when the user table is not empty. So when a release adds a permission to a role, a new model or a view_ that was missing, the installations already running never get it. The two ways out were both bad. 'loaddata group' replaces the permissions of every group with the ones of the fixture, so it silently drops whatever an administrator granted or revoked. A data migration would rewrite decisions that belong to the operator, unattended, on upgrade. syncgroups is explicit and additive: it grants the permissions of the fixture the group does not have, creates the groups the fixture defines and the installation does not, and touches nothing else. --prune opts into removing what the fixture does not define, --dry-run reports without applying, --check exits with an error if there is anything pending, for CI, and --group limits it to one role. It also reports the roles holding add, change or delete on a model without its view. DRF maps GET to view_<model>, so those permissions cannot be used through the api at all: it is how the Incident Responder role ended up unable to read the playbooks it is meant to run.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new Django management command, syncgroups, to reconcile auth.Group permissions against the shipped fixture, and introduces a dedicated test suite to verify core behaviors (additive sync, prune, dry-run/check, and reporting).
Changes:
- Added
ngen/management/commands/syncgroups.pyimplementing fixture-based group/permission reconciliation with--dry-run,--check,--group, and--prune. - Added
ngen/tests/commands/test_syncgroups.pycovering the command’s key scenarios and options. - Added
ngen/tests/commands/__init__.pyto ensure the new test module is packaged/discoverable.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ngen/management/commands/syncgroups.py | Implements the syncgroups command: fixture parsing, permission reconciliation, and usability reporting. |
| ngen/tests/commands/test_syncgroups.py | Adds a comprehensive test suite for syncgroups behaviors and CLI options. |
| ngen/tests/commands/init.py | Initializes the commands test package. |
Suppressed comments (1)
ngen/management/commands/syncgroups.py:167
- When
get_or_create()creates a missing group,sync_group()prints “+ created” but the returned change count doesn’t include the creation. This can cause--checkto miss a required change for groups that should exist but have no permission diffs.
return len(to_add) + len(to_remove)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+139
to
+141
| f" + would be created with {len(entry['permissions'])} permission(s)" | ||
| ) | ||
| return len(entry["permissions"]) |
Comment on lines
+111
to
+124
| @staticmethod | ||
| def resolve_permissions(natural_keys): | ||
| permissions, unknown = [], [] | ||
| for codename, app_label, model in natural_keys: | ||
| permission = Permission.objects.filter( | ||
| codename=codename, | ||
| content_type__app_label=app_label, | ||
| content_type__model=model, | ||
| ).first() | ||
| if permission: | ||
| permissions.append(permission) | ||
| else: | ||
| unknown.append(codename) | ||
| return permissions, unknown |
| for entry in entries: | ||
| pending += self.sync_group(entry, dry_run=dry_run, prune=options["prune"]) | ||
|
|
||
| self.report_unusable_permissions([entry["name"] for entry in entries]) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new Django management command,
syncgroups, to reconcile group permissions with those defined in a fixture, and adds comprehensive tests for its behavior. The command ensures group permissions are additive (never removing admin-added permissions unless explicitly requested), supports dry-run and check modes, and reports on potentially unusable permissions. The accompanying test suite verifies all key scenarios and options.New management command:
ngen/management/commands/syncgroups.py, implementing asyncgroupscommand that:ngen/fixtures/group.json).--dry-run,--check,--group, and--pruneoptions for flexible operation.Testing:
ngen/tests/commands/test_syncgroups.pywith a comprehensive test suite covering: