-
Notifications
You must be signed in to change notification settings - Fork 197
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 RBAC schema implementation #9802
base: rbac
Are you sure you want to change the base?
Conversation
Temporary cherry-pick from: github.com/uyuni-project/pull/9688
906f4e4
to
c79bd4f
Compare
-- | ||
|
||
CREATE TABLE access.endpoint ( | ||
id NUMERIC NOT NULL PRIMARY KEY, |
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.
We should not use NUMERIC anymore. Also no sequence, but do not forget a constraint
Use:
BIGINT CONSTRAINT suse_issper_id_pk PRIMARY KEY
GENERATED ALWAYS AS IDENTITY
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.
Indeed, I was initially focused on setting up the infrastructure to run the existing scripts, but I'll review and adjust the column types, including this one. Thanks.
Since we're implementing the whole schema from scratch, I'd like to introduce another good practice: Let's add descriptions on our DB objects (schema, tables, even columns when necessary) so they're not so cryptic to future viewers anymore. For example; you can add this to
This serves as both a comment on the script and a description when browsing the DB. I'll come up with descriptions for the tables soon as well. |
Indeed, I agree that this is a good practice. I'll add this description and adjust the sanity check to accept comments in schemas as well. |
We have comments like this for the reportDB already. I think Thomas wrote also some checks that prevent not forgetting descriptions. But AFAIK they do not increase the readability when I remember correctly :-) Have a look at schema/reportdb/check_reportdb_doc |
Cool, I had no idea. psql's
|
c79bd4f
to
3752032
Compare
👋 Hello! Thanks for contributing to our project. You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/9802/checks If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code. Reference tests: KNOWN ISSUES Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience. For more tips on troubleshooting, see the troubleshooting guide. Happy hacking! |
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.
LGTM, I have just one comment
FROM access.endpointNamespace en, access.endpoint e, access.namespace n | ||
WHERE en.namespace_id = n.id AND en.endpoint_id = e.id; |
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 is a cross join, isn't it? Is that intended? Seems to me this can also be set as an inner join:
FROM access.endpoint e JOIN access.endpointnamespace en ON e.id = en.endpoint_id JOIN access.namespace n ON en.namespace_id = n.id;
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.
looks like it's missing the join with table namespace, which Ondrej correction fixes.
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 PR is not creating the userGroups table schema. Is that wanted?
FROM access.endpointNamespace en, access.endpoint e, access.namespace n | ||
WHERE en.namespace_id = n.id AND en.endpoint_id = e.id; |
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.
looks like it's missing the join with table namespace, which Ondrej correction fixes.
Plus, web.xml file already have the authorization filter, and the filter is already checking for access. This will block access to any endpoint. |
What does this PR change?
It adds the necessary code for RBAC schema, including changes to handle separate database schema usage.
GUI diff
No difference.
Documentation
No documentation needed: only internal and user invisible changes
DONE
Test coverage
ℹ️ If a major new functionality is added, it is strongly recommended that tests for the new functionality are added to the Cucumber test suite
No tests
DONE
Links
Issue(s): https://github.com/SUSE/spacewalk/issues/26409
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test
(see below)Re-run a test
If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:
Before you merge
Check How to branch and merge properly!