Launchpad MP (501141) - r00ta/openfga-groups-endpoints

[r00tabot]

This is autogenerated by maas.r00ta.com. Enjoy!

Commit message: feat: introduce v2 and v3 user groups endpoints.

Details:

• Implement servicelayer to CRUD user groups.
• Implement cascade logic to also delete openfga tuples when a group is deleted.
• Implement v3 endpoints
• Implement v2 endpoints and use directly the service layer to CRUD user groups.
• Introduce an alembic migration to add the new maasserver_usergroup table and pre-create the 2 default groups "administrators" and "users"
• Modify the existing openfga migrations to use the groups defined above and grant them permissions

[Copilot]

Pull request overview

Introduces first-class “user groups” support across the service layer and both v2 (Django/piston3) and v3 (FastAPI) APIs, and updates OpenFGA integration to reference DB-backed groups (with cascade cleanup of tuples on group deletion).

Changes:

• Added maasserver_usergroup persistence (SQLAlchemy table + Alembic migration + test DB dump updates) and a service/repository/model/builder stack to CRUD user groups.
• Added v3 /groups endpoints + tests, and v2 /groups piston3 handlers + tests.
• Updated OpenFGA integration to use numeric group IDs (DB groups) and added cascade deletion of OpenFGA tuples when a group is deleted.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/maasservicelayer/models/usergroups.py	Adds UserGroup domain model.
src/maasservicelayer/builders/usergroups.py	Adds builder for user groups.
src/maasservicelayer/db/tables.py	Registers maasserver_usergroup SQLAlchemy table.
src/maasservicelayer/db/repositories/usergroups.py	Adds repository + clause factory for user groups.
src/maasservicelayer/services/usergroups.py	Adds service layer for groups and OpenFGA membership/cascade deletion logic.
src/maasservicelayer/services/openfga_tuples.py	Adds delete_group() to delete membership + entitlement tuples.
src/maasservicelayer/db/repositories/base.py	Adds list_all() utility method.
src/maasservicelayer/services/base.py	Adds list_all() on services to support v2 handlers.
src/maasservicelayer/services/init.py	Wires UserGroupsService into ServiceCollectionV3.
src/maasservicelayer/db/alembic/versions/0019_create_usergroup_table.py	Creates the table and inserts default “Administrators” / “Users” rows.
src/maasapiserver/v3/api/public/handlers/usergroups.py	Implements v3 CRUD endpoints for groups.
src/maasapiserver/v3/api/public/models/requests/usergroups.py	Adds v3 request model and builder conversion.
src/maasapiserver/v3/api/public/models/responses/usergroups.py	Adds v3 response models (HAL + pagination).
src/maasapiserver/v3/api/public/handlers/init.py	Registers the new v3 handler.
src/tests/maasapiserver/v3/api/public/handlers/test_usergroups.py	Adds v3 API handler tests.
src/maasserver/api/usergroups.py	Adds v2 piston3 handlers for /groups.
src/maasserver/api/tests/test_usergroups.py	Adds v2 API tests, including OpenFGA permission checks.
src/maasserver/urls_api.py	Exposes v2 routes for group handlers.
src/maasserver/models/signals/users.py	Assigns new users to DB-backed default groups via the usergroups service.
src/maasserver/models/signals/tests/test_users.py	Updates signal test expectations to assert numeric group IDs.
src/maasopenfga/internal/migrations/00002_migrate_environments.go	Updates OpenFGA bootstrap migration to reference DB group IDs.
src/tests/fixtures/factories/usergroups.py	Adds test factory helper for user groups.
src/tests/maasservicelayer/db/repositories/test_usergroups.py	Adds repository tests for clause factory and list filtering.
src/tests/maasservicelayer/services/test_usergroups.py	Adds service + integration tests (including OpenFGA tuple cleanup).
src/tests/maasservicelayer/services/base.py	Extends common service tests to cover list_all().
src/tests/maasservicelayer/db/repositories/test_base.py	Extends common repository tests to cover list_all().
src/tests/maasservicelayer/db/repositories/base.py	Adjusts common repository tests (but includes a debug print).
src/tests/e2e/test_openfga_integration.py	Updates e2e OpenFGA setup to use numeric group IDs.
src/maasserver/testing/initial.maas_test.sql	Updates test DB dump for new table + OpenFGA tuple changes.
src/maasservicelayer/builders/openfga_tuple.py	Switches OpenFGA group identifiers to numeric IDs in tuple builders.

Comments suppressed due to low confidence (1)

src/maasservicelayer/builders/openfga_tuple.py:34

• build_user_member_group() now accepts group_id: int but assigns it directly to object_id, while object_id is stored as Text in OpenFGA and the builder field type is str. Relying on implicit casting/coercion here is brittle; please normalize IDs to str in the builder (and consider widening user_id to accept int | str, since call sites include integer Django user IDs).

    @classmethod
    def build_user_member_group(
        cls, user_id: str, group_id: int
    ) -> "OpenFGATupleBuilder":
        return OpenFGATupleBuilder(
            user=f"user:{user_id}",
            user_type="user",
            relation="member",
            object_id=group_id,
            object_type="group",
        )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The print(deleted_resources) looks like leftover debug output and will add noise/flakiness to test logs. Please remove it (assertions already cover the behavior).

Suggested change

print(deleted_resources)

[Copilot]

post_delete_many_hook() raises NotImplementedError, but BaseService.delete_many() calls this hook after it has already deleted the rows. This would delete multiple groups and then error, leaving callers thinking the operation failed while data is already gone. If bulk-delete is not supported, raise before any deletion (e.g., override delete_many() to raise, or raise from pre_delete_many_hook()).

Suggested change

	async def post_delete_many_hook(self, resources: List[UserGroup]) -> None:
	raise NotImplementedError(
	"Deleting multiple user groups is not supported."
	)
	async def pre_delete_many_hook(self, resources: List[UserGroup]) -> None:
	raise NotImplementedError(
	"Deleting multiple user groups is not supported."
	)
	async def post_delete_many_hook(self, resources: List[UserGroup]) -> None:
	# No-op: bulk deletion is disallowed by pre_delete_many_hook.
	return None

[Copilot]

add_user_to_group() does not handle the case where the requested group name doesn't exist (get_one(...) can return None), which will raise an AttributeError on group.id. Please either raise a domain-appropriate exception when the group is missing or use a method that enforces existence (so callers get a clean error).

Suggested change

	)
	)
	if group is None:
	raise ValueError(f"User group '{group_name}' not found.")

[Copilot]

description defaults to None and to_builder() passes that through, but the DB column is NOT NULL (defaulting to empty string). A request that omits description (or sends null) will attempt to insert/update NULL and fail with an integrity error. Align this with other request models (e.g. Fabrics/Spaces) by defaulting to "" and/or coercing None to "" before building the service-layer builder.

Suggested change

	description="The description of the group.", default=None
	)
	def to_builder(self) -> UserGroupBuilder:
	return UserGroupBuilder(name=self.name, description=self.description)
	description="The description of the group.", default=""
	)
	def to_builder(self) -> UserGroupBuilder:
	return UserGroupBuilder(
	name=self.name,
	description=self.description or "",
	)

[Copilot]

In createGroup, if the SELECT id FROM maasserver_usergroup ... query fails with an error other than sql.ErrNoRows, the code currently doesn't return that error and continues with groupID potentially left as 0. Please return err for unexpected DB errors to avoid inserting tuples for a bogus group id.

Suggested change

	}
	}
	return err

[Copilot]

The variable name created_resource_pools is misleading in a usergroups repository test; it makes the fixture harder to follow. Consider renaming it to something like created_groups/created_usergroups to match what’s actually being created and asserted.