Swirrl / drafter

A clojure service and a client to it for exposing data management operations to PMD
Other
0 stars 0 forks source link

Rbac #610

Closed callum-oakley closed 2 years ago

callum-oakley commented 2 years ago

Changes the access control model in drafter to be based on permissions rather than roles, so that we can make full use of auth0's RBAC support. This PR doesn't introduce any new user facing features, but should make new features involving permissions (e.g. multi-user read only drafts) easier.

The only external changes are:

Tested against muttnik here: https://github.com/Swirrl/muttnik/pull/1508

Next steps

I think this can be reviewed and merged (review permitting) as it is, but we'll want follow up PRs to:

closes #592

Deployment Suggestions

NOTE you should run the migrations below for both PMD3 and PMD4 installations. We shouldn't need to run any migrations against the PMD3 mongo database, as the code translates deprecated roles into equivalent "canonical permissions"; only the drafter state graph needs to be migrated (in both PMD3 and PMD4).

RickMoynihan commented 2 years ago

👋 Hey @callum-oakley,

I've just taken a quick look at this and it looks at first glance like these might be breaking changes for existing API users, is that correct?

I was assuming that we'd still preserve compatibility for users using the old ?role= parameters, but mark those parameters in the docs as deprecated and encourage new users to use the permission parameter.

callum-oakley commented 2 years ago

@RickMoynihan Yeah that is a breaking change... but now that you mention it it would be pretty easy to keep the role parameter and map it to a permission internally. I'm doing this already in the muttnik PR so that the interface is the same from the perspective of a muttnik user, but it makes sense to do this in drafter instead for direct API users. Will add a commit. 🙏

RickMoynihan commented 2 years ago

Amazing thanks @callum-oakley! Sorry if I wasn't more explicit about this earlier!

callum-oakley commented 2 years ago

@RickMoynihan latest commit adds migrations to backfill drafter:claimPermission and delete drafter:claimRole triples, split in to two updates (plus one sanity check query).

callum-oakley commented 2 years ago

@RickMoynihan Leaving the drafter: prefix be now. Think I got them all. 🤞

RickMoynihan commented 2 years ago

Yeah, amazing. Thanks @callum-oakley. I happened to look at the page at the same moment you pushed the commit, so I've checked over the diffs whilst the tests were running.

I'm happy that the diffs look correct; though after lunch I might just do one last spot check and compare the set of files touched in this commit are the same as the set of the files changed prior to it; incase there's any places that should have been updated that haven't.

After that check I think we can merge though.