-
Notifications
You must be signed in to change notification settings - Fork 18
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 configurable known groups #963
Add configurable known groups #963
Conversation
No need for them to be `pub(crate)`
They all use JSON because: - `add` needs to be able to add many groups in one go (otherwise it might be too slow). - Coming up with an arbitrary way to specify all group data via CLI args is also not amazing.
See comments for explanation.
I learned a bit about Postgres extensions. In PG 12 and earlier, most extensions (including But this makes this PR potentially breaking. I marked it with For reference, on the dev DB:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, the commands for adding/updating/etc. the known groups
are all working very well.
I noticed two things during my testing:
db clear
anddb reset
aren't working unless thehstore
extension is manually dropped from the db first (db error: ERROR: cannot drop type ghstore because extension hstore requires it. HINT: You can drop extension hstore instead.
).- The
superset
detection thing in the access ui isn't using theimplies
field of theknown_groups
table entries yet, so the only groups regarded as supersets are the default ones (everyone, logged in users).
frontend/src/ui/Access.tsx
Outdated
})(); | ||
|
||
const buildDag = (): GroupDag => { | ||
const buildDag = (knownGroups: KnownRoles): GroupDag => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't use the supersets/implies
entries from groups.json
yet (there is a TODO
for that in line 662). Do you plan to do this in a follow up PR?
(The dummy groups are not very "dummy", but we still don't want to preconfigure those without a way to remove them.)
66074fa
to
48a5cc4
Compare
I think this filter is a lot better than the previous one. It also filters out the internal array types for example.
Well done catching those! It should all be fixed now. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Fixes #952
Known groups are stored in the database and manipulated via CLI commands. This makes it a lot easier to script and/or add lots of groups, compared to storing them in the config file. The documentation added in one of the later commits should explain everything in detail, and the commit messages should help understand this patch. Can be reviewed commit by commit.
As usual, the last commit (configuring groups for the test deployment) is not used by this test deploy yet, but only after merging this PR. And as usual, since I couldn't easily test this, there is probably something slightly broken :P but i can just fix that after merging.