Add claims-based permission system#2892
Conversation
|
@ztripez unfortunately test failure |
|
|
||
| @api_command("config/providers/save", required_role="admin") | ||
| @api_command("config/providers/save") | ||
| @requires_permission(Permission.PROVIDERS_MANAGE) |
There was a problem hiding this comment.
Tiny nitpick, and more-so just asking a question, but what grammatical form should we prefer for function names in this codebase? Do we want the imperative form require_permission instead of requires_permission or required_permission?
There was a problem hiding this comment.
cache invalidation and naming things are the two hardest problem in dev. 😄
There was a problem hiding this comment.
I've heard it as cache invalidation, naming things and off by one errors are the hardest two problems in dev.
|
|
||
| @api_command("auth/user/enable", required_role="admin") | ||
| @api_command("auth/user/enable") | ||
| @requires_permission(Permission.USERS_MANAGE) |
There was a problem hiding this comment.
Do you want to guard each function with the "requires_permission" or only extend the api_command decorator with a permissions argument ?
There was a problem hiding this comment.
Also, would scope be a better (more recognizable) name instead of permission ?
I like permission but in general scope seems to be more widely used
There was a problem hiding this comment.
Also, would scope be a better (more recognizable) name instead of permission ? I like permission but in general scope seems to be more widely used
yeah, scope is correct but permission is more telling. It's up to you i guess.
|
Two comments from my side while I am testing this:
|
| :param required: Required permissions. | ||
| :return: True if user has all required permissions. | ||
| """ | ||
| user_permissions: list[str] | None = getattr(user, "_permissions", None) |
There was a problem hiding this comment.
Any specific reason that you did not add _permissions and _claims as attributes to User in the models package? This would prevent the need of getattr
Introduce a Permission enum and permission-checking helpers in music_assistant/helpers/permissions.py with wildcard support and role-based fallback. The api_command decorator now accepts a required_permissions parameter, and both WebSocket and HTTP dispatch enforce permissions before falling back to required_role. All 23+ admin-only endpoints are converted from required_role='admin' to fine-grained required_permissions (library, players, config, users, providers, streams). JWT tokens include permissions in the payload, and auth.py attaches permissions/claims on the User object. Includes 17 unit tests covering the Permission enum, role mapping, permission checks (wildcard, explicit, multi-permission, fallback), and JWT claim extraction.
84645bf to
307cf61
Compare
Summary
Adds a claims-based permission system for Music Assistant. Permissions are integrated directly into the
api_command()decorator via arequired_permissionsparameter — no separate decorator needed. The system works with existing roles today while enabling future support for external OIDC providers and provider-contributed custom claims.Dependencies
Models PR: music-assistant/models#172 — adds
permissionsandclaimsfields toUserdataclass. Until merged, this PR usesgetattr/type: ignore[attr-defined]to safely access these attributes.Changes
New Files
music_assistant/helpers/permissions.py: Permission system implementationPermissionenum with 17resource:actionscopes + wildcard (*)get_permissions_for_role(): MapsUserRole→ list ofPermissionhas_permission(): Checks if user has required permissions (wildcard-aware, role-fallback)get_user_claim(): Helper for future provider-contributed claimstests/core/test_permissions.py: 17 unit tests covering:resource:actionformatModified Files
music_assistant/helpers/api.py: Addedrequired_permissionsparameter toapi_command()decorator,APICommandHandler, andAPICommandHandler.parse()music_assistant/mass.py: Updatedregister_api_command()and_register_api_commands()to pass throughrequired_permissionsmusic_assistant/helpers/jwt_auth.py: Addedpermissionsclaim to JWT payloadmusic_assistant/controllers/webserver/auth.py: Added_attach_permissions()method called from both JWT and legacy token auth paths; converted 9 endpointsmusic_assistant/controllers/webserver/controller.py: HTTP dispatch checksrequired_permissionsbeforerequired_rolemusic_assistant/controllers/webserver/websocket_client.py: WebSocket dispatch checksrequired_permissionsbeforerequired_rolemusic_assistant/controllers/webserver/api_docs.py: API docs includerequired_permissionsfieldmusic_assistant/controllers/config.py: 9 endpoints converted fromrequired_roletorequired_permissionsmusic_assistant/controllers/players/controller.py: 3 endpoints convertedmusic_assistant/controllers/media/base.py: 2 endpoints converted (update/remove)music_assistant/controllers/media/genres.py: 8 endpoints convertedmusic_assistant/controllers/webserver/remote_access/__init__.py: 2 endpoints convertedPermission Scopes
*library:readlibrary:writelibrary:deleteplayers:readplayers:controlplayers:configureusers:readusers:manageconfig:readconfig:writeproviders:readproviders:managemetadata:readmetadata:refreshstreams:readstreams:controlRole → Permission Mapping
*(wildcard — grants everything)*:read+players:control+streams:controlArchitecture
api_command(required_permissions=[Permission.X])stores permissions on the handler metadata"permissions"claim generated from the user's roleauth.pyattachespermissionsandclaimsto theUserobject after authenticationhandler.required_permissionsfirst viahas_permission(), falling back tohandler.required_rolefor backward compatibilityhas_permission()checks the user's explicit permissions list; if empty/missing, generates permissions from the user's role as fallbackAddressing Reviewer Feedback
api_command— no standalone@requires_permissiondecorator (marcelveldt)tests/core/test_permissions.py(MarvinSchenkel)permissions/claimstoUser(MarvinSchenkel)Testing
test_tags.py)