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 configurable known groups #963

Merged
merged 11 commits into from
Oct 23, 2023

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Oct 16, 2023

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.

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.
@github-actions github-actions bot temporarily deployed to test-deployment-pr963 October 16, 2023 13:29 Destroyed
@LukasKalbertodt LukasKalbertodt added the changelog:breaking Breaking changes label Oct 16, 2023
@LukasKalbertodt
Copy link
Member Author

LukasKalbertodt commented Oct 16, 2023

I learned a bit about Postgres extensions. In PG 12 and earlier, most extensions (including pgcrypto and hstore) require super user privileges to enable. With PG13, the concept of "trusted extensions" was added. For those trusted ones, any user with create privileges on the DB can enable them. The two extensions we use are trusted. Our documentation already mentions that in PG12 and earlier, one has to manually enable the extensions. But now I know why :D

But this makes this PR potentially breaking. I marked it with changelog:breaking just to remind us to write a note into the changelog for the next release. Otherwise it will be an annoying update for some.

For reference, on the dev DB:

tobira=# select * from pg_available_extension_versions() where name like '%pgcrypto%' or name like '%hstore%';
   name   | version | superuser | trusted | relocatable | schema | requires |                     comment                      
----------+---------+-----------+---------+-------------+--------+----------+--------------------------------------------------
 hstore   | 1.4     | t         | t       | t           |        |          | data type for storing sets of (key, value) pairs
 hstore   | 1.5     | t         | t       | t           |        |          | data type for storing sets of (key, value) pairs
 hstore   | 1.6     | t         | t       | t           |        |          | data type for storing sets of (key, value) pairs
 hstore   | 1.7     | t         | t       | t           |        |          | data type for storing sets of (key, value) pairs
 hstore   | 1.8     | t         | t       | t           |        |          | data type for storing sets of (key, value) pairs
 pgcrypto | 1.3     | t         | t       | t           |        |          | cryptographic functions

@github-actions github-actions bot temporarily deployed to test-deployment-pr963 October 16, 2023 14:11 Destroyed
Copy link
Member

@owi92 owi92 left a 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 and db reset aren't working unless the hstore 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 the implies field of the known_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 Show resolved Hide resolved
docs/docs/setup/config.md Outdated Show resolved Hide resolved
docs/docs/setup/config.md Outdated Show resolved Hide resolved
})();

const buildDag = (): GroupDag => {
const buildDag = (knownGroups: KnownRoles): GroupDag => {
Copy link
Member

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?

@LukasKalbertodt LukasKalbertodt force-pushed the configurable-known-groups branch from 66074fa to 48a5cc4 Compare October 23, 2023 12:23
@github-actions github-actions bot temporarily deployed to test-deployment-pr963 October 23, 2023 12:32 Destroyed
I think this filter is a lot better than the previous one. It also
filters out the internal array types for example.
@LukasKalbertodt
Copy link
Member Author

Well done catching those! It should all be fixed now. The db reset thing is a new commit, everything else is amended and should be visible in the force-push-compare-button.

@github-actions github-actions bot temporarily deployed to test-deployment-pr963 October 23, 2023 13:26 Destroyed
Copy link
Member

@owi92 owi92 left a comment

Choose a reason for hiding this comment

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

👌

@owi92 owi92 merged commit 8cfc123 into elan-ev:master Oct 23, 2023
5 checks passed
@LukasKalbertodt LukasKalbertodt deleted the configurable-known-groups branch October 23, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:breaking Breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make known groups of ACL selector configurable
2 participants