feat(api): implement permissions system #927

jordy2254 · 2024-04-01T18:45:11Z

No description provided.

codecov · 2024-04-01T18:51:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.00%. Comparing base (b2d591b) to head (33e9336).
Report is 3 commits behind head on master.

❗ Current head 33e9336 differs from pull request most recent head 2d30ece. Consider uploading reports for the commit 2d30ece to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #927       +/-   ##
===========================================
+ Coverage   57.34%   70.00%   +12.65%     
===========================================
  Files         196      100       -96     
  Lines       15607    11200     -4407     
  Branches      533      533               
===========================================
- Hits         8950     7840     -1110     
+ Misses       6408     3360     -3048     
+ Partials      249        0      -249

Flag	Coverage Δ
api	`?`
ui	`70.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

viktorstrate

I think there's a problem with the database_models in database.go, see comment.

viktorstrate · 2024-04-07T12:00:36Z

api/database/database.go

@@ -153,8 +154,13 @@ func SetupDatabase() (*gorm.DB, error) {
 	return db, nil
 }

-var database_models []interface{} = []interface{}{
+var database_models_1 []interface{} = []interface{}{


I think the tests don't work since you make two database models and try to migrate them both. Why did you add a database_models_1?

@viktorstrate the reason for this was due to some query size constraints atleast within mysql. I was going to investigate further to find a better solution. I will take a further look st this and hope its the cause I'd the failures

viktorstrate · 2024-04-07T12:03:12Z

api/database/migrations/permissions.go

+)
+
+// MigrateForPermissions Migrates new permissions into the system and upgrades system roles according to the new permissions.
+func MigrateForPermissions(db *gorm.DB) error {


We should test this migration thoroughly before releasing, since it is a little more involved.
Just to make sure a lot of users databases doesn't get corrupted.

Fully agree on this one, I've tested the scenarios I can think of albeit on a small dataset and only a few user's. These are as below.

Migrating from an old version of photoview with various admin users, normal user & demo user

Addition of a new permission

Addition of an existing permission to a system user group doesn't add to the user group

Addition of a new permission to a system user group automatically adds to the user group

viktorstrate · 2024-04-07T12:05:36Z

api/graphql/schema.graphql

-    admin: Boolean
-  ): User! @isAdmin
+    roleId: ID
+    admin: Boolean @deprecated(reason: "The roles system replaces the old admin boolean.")


I really like that you deprecated the old method. Then when we want to make a 3.0 release at some point, we can refactor out all the deprecations.

viktorstrate · 2024-04-07T12:06:02Z

api/graphql/schema.graphql

@@ -1,9 +1,16 @@
 directive @isAuthorized on FIELD_DEFINITION
-directive @isAdmin on FIELD_DEFINITION
+directive @hasPermission(permission: Permission!) on FIELD_DEFINITION


Having a @hasPermission directive is awesome :)

jordy2254 · 2024-04-15T13:34:58Z

Just an update for anyone interested on this. First of all I'll need some user's to test migrations if anyone see's this and is interested ping me a message on discord.

I've not had much time to look at this as I had hoped but finally managed to get a look this afternoon at some of the test faliures and it's not mostly working. below is what's left/what I've changed since last pushing

The test suite is now nearly fixed, I've implemented database transactions for all tests rather than the delete everything system for the end of a test that was implemented before.
I have one more test set to fix which is the scanner tests, this is because of the scanner creating a transaction per thread which locks the outer transaction causing errors, I'm yet to investigate this one but have some ideas.
UI Changes still need doing the initial pass of these will be the ability to select a user role for a user only and not create and amend the roles themselves.

I'm hoping I'll be able to finish this off during the week depending on how much dev time I get but I won't promise that just yet.

jordy2254 · 2024-04-15T15:26:15Z

api/graphql/schema.graphql

@@ -104,7 +127,8 @@ type Query {
 type Mutation {
  "Authorizes a user and returns a token used to identify the new session"
  authorizeUser(username: String!, password: String!): AuthorizeResult!
-
+  createRole(role: NewRoleInput): Role!


Missing permission derectives on these 2 mutations

Need to implement a delete mutation

api/api

NOTE: a few tests are not transactional due to concurent transactions this is the scanner tests.

jordy2254 · 2024-04-18T15:10:08Z

ui/src/Pages/SettingsPage/Users/RoleSelector.tsx

+    items.push({ label: 'Loading...', value: '' })
+  } else {
+    if (!data?.roles.find(role => role.id === props.selected)) {
+      items.push({ label: 'Please Select', value: '' })


Missing translation

jordy2254 added feature A new idea or feature API Related to the backend api server written in Go labels Apr 1, 2024

jordy2254 self-assigned this Apr 1, 2024

jordy2254 marked this pull request as ready for review April 1, 2024 18:46

jordy2254 force-pushed the permissions-system branch from 3a0512c to 33e9336 Compare April 1, 2024 18:47

jordy2254 marked this pull request as draft April 1, 2024 18:53

viktorstrate requested changes Apr 7, 2024

View reviewed changes

jordy2254 force-pushed the permissions-system branch from 0a091ee to ca1b1e3 Compare April 15, 2024 15:11

jordy2254 commented Apr 15, 2024

View reviewed changes

api/api Outdated Show resolved Hide resolved

jordy2254 added 4 commits April 18, 2024 16:08

feat(api): implement permissions system

881070f

test: allow test suite to run in transactions and fix tests

c67b18e

NOTE: a few tests are not transactional due to concurent transactions this is the scanner tests.

feat(ui): enable ability to select user role on edit and create

f763aca

chore: regenerate ui files

2d30ece

jordy2254 force-pushed the permissions-system branch from ca1b1e3 to 2d30ece Compare April 18, 2024 15:08

jordy2254 commented Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): implement permissions system #927

feat(api): implement permissions system #927

jordy2254 commented Apr 1, 2024

codecov bot commented Apr 1, 2024 •

edited

viktorstrate left a comment

viktorstrate Apr 7, 2024

jordy2254 Apr 7, 2024

viktorstrate Apr 7, 2024

jordy2254 Apr 7, 2024

viktorstrate Apr 7, 2024

viktorstrate Apr 7, 2024

jordy2254 commented Apr 15, 2024

jordy2254 Apr 15, 2024

jordy2254 Apr 18, 2024

feat(api): implement permissions system #927

Are you sure you want to change the base?

feat(api): implement permissions system #927

Conversation

jordy2254 commented Apr 1, 2024

codecov bot commented Apr 1, 2024 • edited

Codecov Report

viktorstrate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordy2254 commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2024 •

edited