Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add migrations for new permissions structure #21945

Merged
merged 24 commits into from
Apr 11, 2024
Merged

Conversation

rijkvanzanten
Copy link
Member

@rijkvanzanten rijkvanzanten commented Mar 21, 2024

Scope

What's changed:

  • Adds new directus_policies table that's a 1-1 copy of the former directus_roles
  • Removes the access control fields from directus_roles
  • Updates directus_permissions to:
    • Be attached to policies instead of roles
    • Split up permissions/fields/validation/presets into individual rows based on a new type column

Potential Risks / Drawbacks

  • This will most likely not work during zero-downtime deployments, as we're changing the data model for permissions heavily. The old version can't run on the new structure and vice versa.

Review Notes / Questions

  • Wondering if there's a cleverer way to replace the previous permissions than a full truncate + insert
  • Todo's left are:
    • Heavy testing

This comment was marked as off-topic.

paescuj

This comment was marked as outdated.

@rijkvanzanten

This comment was marked as outdated.

@paescuj

This comment was marked as outdated.

@rijkvanzanten rijkvanzanten marked this pull request as ready for review March 26, 2024 21:28
@rijkvanzanten rijkvanzanten added this to the Auditus milestone Mar 26, 2024
@rijkvanzanten

This comment was marked as outdated.

@rijkvanzanten rijkvanzanten changed the title [WIP] Add migrations for new permissions structure Add migrations for new permissions structure Mar 29, 2024
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Getting the following error at the moment (same with other DB vendors):

[Error: INSERT INTO "_knex_temp_alter254" SELECT * FROM "directus_permissions"; - SQLITE_CONSTRAINT: NOT NULL constraint failed: _knex_temp_alter254.policy] {
  errno: 19,
  code: 'SQLITE_CONSTRAINT'
}

@rijkvanzanten
Copy link
Member Author

@paescuj Oh no here come the database specific bugs :| _knex_temp_alter254.policy sounds like the temporary copy of the table that knex generates during alter statements

@paescuj
Copy link
Member

paescuj commented Apr 1, 2024

@paescuj Oh no here come the database specific bugs :| _knex_temp_alter254.policy sounds like the temporary copy of the table that knex generates during alter statements

Oh yeah, how we love those 😅
Fortunately, it doesn't seem to be one of that kind in this case 😌

It happens for permissions assigned to the "Public" role (a.k.a. null role), which conflicts with

table.uuid('policy').notNullable().alter();

Not sure how we want to handle the "Public" role in the new policy system, so depending on that we either need to transform null values to something else in the migration or need to remove the NOT NULL constraint.

Example permission causing the issue (before ALTER statement):

  {
    id: 30,
    role: null,
    collection: 'test',
    action: 'create',
    permissions: null,
    validation: '{"_and":[{"id":{"_eq":"1"}}]}',
    presets: '{"id":1}',
    fields: 'id',
    policy: null
  }

@alexchopin alexchopin linked an issue Apr 2, 2024 that may be closed by this pull request
@DanielBiegler DanielBiegler self-requested a review April 10, 2024 16:02
@DanielBiegler DanielBiegler marked this pull request as draft April 10, 2024 16:41
rijkvanzanten and others added 5 commits April 10, 2024 12:53
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
Co-authored-by: Daniel Biegler <DanielBiegler@users.noreply.github.com>
@rijkvanzanten rijkvanzanten marked this pull request as ready for review April 10, 2024 17:49
@rijkvanzanten
Copy link
Member Author

Please use Rebase and merge instead of Squash and merge below ⭐

Copy link
Contributor

@DanielBiegler DanielBiegler left a comment

Choose a reason for hiding this comment

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

After running the migration I'm stuck when trying to open the app. Still gotta figure out why exactly but thought to comment here just in case. Will update when I find more.

api dev: [20:50:24.361] ERROR: select `ip_access` from `directus_roles` where `id` is null limit 1 - SQLITE_ERROR: no such column: ip_access
api dev:     err: {
api dev:       "type": "Error",
api dev:       "message": "select `ip_access` from `directus_roles` where `id` is null limit 1 - SQLITE_ERROR: no such column: ip_access",
api dev:       "stack":
api dev:           Error: select `ip_access` from `directus_roles` where `id` is null limit 1 - SQLITE_ERROR: no such column: ip_access
api dev:       "errno": 1,
api dev:       "code": "SQLITE_ERROR",
api dev:       "extensions": {
api dev:         "stack":
api dev:             Error: select `ip_access` from `directus_roles` where `id` is null limit 1 - SQLITE_ERROR: no such column: ip_access
api dev:       }
api dev:     }

@rijkvanzanten
Copy link
Member Author

@DanielBiegler That's fully expected! This PR is only the migrations for the database, but doesn't update anything else yet in the API or app 🙂

That's also why we're rebasing this into the auditus branch rather than the main branch 👍🏻

Just doing it in smaller PRs on top of auditus to make the review cycle easier as we can review each step of the way in isolation, which should hopefully mean that the final review of the whole project branch is going to go way smoother as every individual piece has already been reviewed 🚀

@DanielBiegler
Copy link
Contributor

That's fully expected!

Oh whoops sry @rijkvanzanten . I assumed it would work because I saw the merge commits from auditus into this branch by you and Pascal and thought those would include some fixes but alrighty then. 🤔

@rijkvanzanten
Copy link
Member Author

All good! Those updates are all to keep auditus up to date with the upstream main.

@DanielBiegler DanielBiegler merged commit 82102fd into auditus Apr 11, 2024
1 check passed
@DanielBiegler DanielBiegler deleted the auditus-21760 branch April 11, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants