Skip to content

refactor: improve handling of permissions, users, and settings#278

Merged
tsagadar merged 19 commits intomasterfrom
permisions-rework
Apr 22, 2025
Merged

refactor: improve handling of permissions, users, and settings#278
tsagadar merged 19 commits intomasterfrom
permisions-rework

Conversation

@b-rowan
Copy link
Copy Markdown
Member

@b-rowan b-rowan commented Mar 24, 2025

This PR aims to:

  • Move user handling into the database
  • Refactor permission handling into a more strict style
  • Add a settings page for working with locally managed users, including adding and removing them and modifying their permissions
  • Allow parsing a "top level" permission structure into a menu of user-configurable permissions, to allow the user permissions to be configured via the UI
  • Add an API endpoint to allow users to be configured programmatically
  • Allow enabling and disabling users so that the initial user created by goosebit can be disabled if needed

@b-rowan
Copy link
Copy Markdown
Member Author

b-rowan commented Mar 24, 2025

Fixes: #276

@b-rowan b-rowan linked an issue Mar 24, 2025 that may be closed by this pull request
@b-rowan b-rowan force-pushed the permisions-rework branch 2 times, most recently from bf971a5 to d6a55e4 Compare April 3, 2025 20:41
@b-rowan b-rowan marked this pull request as ready for review April 3, 2025 20:41
@b-rowan b-rowan requested a review from tsagadar April 3, 2025 20:42
@b-rowan
Copy link
Copy Markdown
Member Author

b-rowan commented Apr 3, 2025

@tsagadar RE #276, this is pretty much my proposed solution to that, in theory we could actually handle API path permissions too (have BFF use UI permissions and separate API permissions), but I think just being able to properly configure users is good enough. In any case, even with API permissions, the program could just use the BFF endpoints anyway.

This feels like a relatively elegant solution that allows getting rid of some configuration inside the initial yaml file, let me know your thoughts.

Copy link
Copy Markdown
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things that popped out from the code. Will do some manual testing at a later stage.

Comment thread goosebit.yaml Outdated
Comment thread goosebit/db/__init__.py Outdated
Comment thread goosebit/auth/permissions.py
Comment thread goosebit/ui/bff/settings/users/routes.py
@b-rowan b-rowan force-pushed the permisions-rework branch from d6a55e4 to ff117cc Compare April 7, 2025 14:46
Comment thread goosebit/ui/static/js/setup.js Dismissed
@tsagadar
Copy link
Copy Markdown
Collaborator

tsagadar commented Apr 21, 2025

Tried out the new functionality. Looks nice, but I had a few issues to use it. Some of them likely can be addressed in a later PR as well.

  1. Initial user creation page: best to add a short explanation like "Welcome to goosebit. As a first step create your admin account."
  2. Users without the "goosebit.devices" permission cannot use the UI: after log-in one is redirected to /ui/devices and if permissions are mission it just shows {"detail":"Not enough permissions"}. Once in that state, even logout is not possible. So one should get redirected to the first screen where read permissions are given.
  3. Changing user permissions is not yet implemented
  4. Creating a user without permission is possible but does make no sense. Disallow it?
  5. Likely an old issue: login in with wrong password does not show any error

Copy link
Copy Markdown
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #278 (comment) for open issues.

@b-rowan
Copy link
Copy Markdown
Member Author

b-rowan commented Apr 21, 2025

Changing user permissions is not yet implemented

Do we want to be able to reset a user's password via the Update User UI? Can add that as well while doing this if its a feature we want.

@b-rowan b-rowan force-pushed the permisions-rework branch from 7896600 to dec3b64 Compare April 21, 2025 14:33
@b-rowan
Copy link
Copy Markdown
Member Author

b-rowan commented Apr 21, 2025

@tsagadar Fixed issue 1, 2 and 4, will do 3 alongside user password resets if you feel we want that feature, and not entirely sure whats going on with 5.

Comment thread goosebit/ui/static/js/settings.js Dismissed
Copy link
Copy Markdown
Collaborator

@tsagadar tsagadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor things to address.

There will be a need that user can be modified (permissions and password) by "admins". But I would not add it to this PR.

Comment thread goosebit/ui/templates/setup.html.jinja Outdated
Comment thread goosebit/ui/static/js/settings.js Dismissed
Comment thread goosebit/ui/static/js/settings.js Outdated
Comment thread goosebit/ui/static/js/settings.js Outdated
b-rowan added 13 commits April 21, 2025 14:26
This makes the permission handling more consistent between the fastAPI routes and the nav handler.
Moving user handling from an in-memory settings config to the database will allow configuring the user information dynamically.
Using stacked `Permission` objects will allow keeping track of valid permissions, and will eventually allow displaying them on a settings page.
Inverted permission values are no longer supported.
Test creating users via API, and test BFF responses.
@b-rowan b-rowan force-pushed the permisions-rework branch from 5621e8d to b9a0cf1 Compare April 21, 2025 20:26
@b-rowan b-rowan requested a review from tsagadar April 21, 2025 20:28
@tsagadar tsagadar merged commit c837865 into master Apr 22, 2025
5 checks passed
@tsagadar tsagadar deleted the permisions-rework branch April 22, 2025 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement API Path-Based Permissions for Granular Access Control

3 participants