Skip to content

Add scoped named permissions and restricted user roles - #2607

Open
nhoening wants to merge 21 commits into
mainfrom
feat/security-permissions-more-detailled
Open

nhoening wants to merge 21 commits into
mainfrom
feat/security-permissions-more-detailled

Conversation

@nhoening

@nhoening nhoening commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Description

From discussion #2577. Closes #2608. This PR makes more fine-grained roles and permissions, which enables the read-only access to some account's resources that was the main goal, but also brings access rights management to a better-defined level.

Roles (same as before, plus:

  • "account-reader" (this one is fulfilling the original issue, just this role means you are read-only in your account)
  • "account-member" (this is what members had by default before)
  • "account-data-integrator" (preparing for the future, a user only allowed to read & write data, e.g. for API integrations)

Permissions:

  • "read",
  • "post-data",
  • "annotate",
  • "trigger-schedules",
  • "trigger-forecasts",
  • "trigger-reports",
  • "manage-automations",
  • "edit-flex-config",
  • "edit-assets",
  • "edit-sensors",
  • "delete-data",
  • "manage-users",
  • "edit-profile",
  • "reset-password",
  • "edit-account"

Added permissions (that were not part of the discussion):

  • edit-profile covers a user changing their own profile.
  • reset-password names the existing reset action: users can reset their own password, and authorised account admins can reset passwords within their organisation.

We could reduce this set by 3 if all trigger- permissions are wrapped into one trigger-calculations permission, and
edit-assets includes edit-flex-config.

These 3 legacy permissions remain available to plugin ACLs:.

  • "create-children",
  • "update",
  • "delete",

Roles define what the user's granted permissons are. ACLs define per resource if the grants apply to them (e.g. how a user with admin role differs from one with account-admin - they can only be admin on assets in their account). In some cases, our API is fine-tuning that, e-g- for consultants. That already happened before.

Here is how Role's new permissions property (aligning with Flask-Security practices maps from itself to permissions:

ROLE_PERMISSION_GRANTS = {
    ACCOUNT_READER_ROLE: frozenset({"read"}),
    ACCOUNT_DATA_INTEGRATOR_ROLE: frozenset({"read", "post-data", "reset-password"}),
    ACCOUNT_MEMBER_ROLE: PERMISSIONS
    - {"delete-data", "manage-users", "delete"},
    ACCOUNT_ADMIN_ROLE: PERMISSIONS,
    CONSULTANT_ROLE: PERMISSIONS,
    ADMIN_READER_ROLE: frozenset({"read"}),
    ADMIN_ROLE: PERMISSIONS,
}

You can see the three new roles getting a special set, while admins get all grants.

Felix’s scope rule is included: home roles apply in a user’s own organisation, while consultant applies through client organisation ACLs. Role.permissions is defined in code. The migration grants existing users member to preserve their former implicit access.

  • added 3 new roles: read-only, member, integration
  • permissions live on Role
  • fine-grained permissions: exoanx the permission set (see above)
  • migration to backfill account-member role to all existing users, and add new roles, and descriptions to all roles
  • User edit UI adds role descriptions to selection form and also in tooltips of selected roles
  • Added changelog item in documentation/changelog.rst

The db migration has no downgrade logic. We would have to persist member role associations on disk, I believe. Should we do that?

Look & Feel

image

How to test

Further Improvements

  • The permission set has expanded, but our business logic to grant them to roles not yet. Probably something we opened up now for later.
  • Should we reduce the number of permissions already now?

@nhoening nhoening self-assigned this Sep 27, 2026
@nhoening nhoening added this to the 1.1.0 milestone Sep 27, 2026
@nhoening
nhoening marked this pull request as ready for review September 29, 2026 10:36
…implify role list

Signed-off-by: Nicolas Höning <nicolas@seita.nl>
…:FlexMeasures/flexmeasures into feat/security-permissions-more-detailled
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
@nhoening
nhoening requested a review from Flix6x September 29, 2026 15:50
@Flix6x

Flix6x commented Oct 3, 2026

Copy link
Copy Markdown
Member

Heads-up now that #2634 has merged: this branch's merge migration no longer merges all the heads.

8e9348063a5d_merging joins df847c1a72b0 and b63a02d5e184. Since #2634, b63a02d5e184 is no longer a head — c5e1a7b94d20 is its child. So after this branch is brought up to date there would be two heads again: this merge revision and c5e1a7b94d20. flexmeasures db upgrade refuses to run with more than one, and it fails in the Docker build rather than anywhere obvious.

The fix is to point the merge at the current head instead, so down_revision = ("df847c1a72b0", "c5e1a7b94d20"). Worth re-checking after any future merge into main that brings a migration with it, since a merge revision is pinned to the heads that existed when it was written.

flexmeasures db heads   # should print exactly one

is the check, and it is quick.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC

#2634 added c5e1a7b94d20 on top of b63a02d5e184, so the revision this branch merges was no longer a head
and the database would have been left with two. The merge now names the current head instead.

One conflict needed a choice rather than both sides: this branch turns a sensor page into a 403 for a user of another organisation,
where main had grown assertions about which panels such a user is shown. A 403 answers that outright, so this branch's expectation stands.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC
Signed-off-by: F.N. Claessen <claessen@seita.nl>
@Flix6x

Flix6x commented Oct 3, 2026

Copy link
Copy Markdown
Member

Brought this branch up to date with main in 1c94510, and fixed the migration heads as flagged above: 8e9348063a5d now merges df847c1a72b0 with c5e1a7b94d20 rather than with b63a02d5e184, which stopped being a head when #2634 merged. flexmeasures db heads prints one.

One conflict needed a choice, and it is yours to confirm. In test_sensor_views.py, this branch turns a sensor page into a 403 for a user of another organisation — commit 667d946 changes two assertions from 200 to 403 deliberately. Meanwhile main grew assertions on the same tests about which panels such a user is shown, from the annotations work. The two cannot both hold: a 403 has no panels. I kept this branch's 403 and dropped main's panel assertions for those two tests, since that is what the branch set out to change.

Worth a conscious look, because it is a policy change rather than a test detail: a user of another organisation currently can open such a page and simply sees fewer controls, and after this they cannot open it at all. If that is intended, it deserves a line in the changelog; if the page should stay readable with the controls hidden, then those assertions should come back and the scoped permission needs to allow the read.

32 tests pass across test_sensor_views.py and test_sources_api.py on the merged branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Users with read-only permissions

2 participants