RBAC: Initial implementation #9844

cbbayburt · 2025-02-24T18:58:22Z

This PR introduces the layout for RBAC filtering structure, without enforcing access or adding any endpoint/namespace data.

Optionally, here's how to enable RBAC in this version:

Add endpoint/namespace data to DB manually (I will provide DMLs separately)
Add server.susemanager.rbac = 1 config option to rhn.conf

Documentation

No documentation needed: private release

Test coverage

Tests are still in progress

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:

No changelog needed

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:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

github-actions · 2025-02-24T18:58:34Z

👋 Hello! Thanks for contributing to our project.
Acceptance tests will take some time (aprox. 1h), please be patient ☕

You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/9844/checks
Once tests finish, if they fail, you can check 👀 the cucumber report. See the link at the output of the action.
You can also check the artifacts section, which contains the logs at https://github.com/uyuni-project/uyuni/pull/9844/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!
⚠️ You should not merge if acceptance tests fail to pass. ⚠️

rjmateus

Looks good to me, just some small nitpick that I think doesn't influence the overall quality and the ability to merge the code. All the comments can be addressed after the beta version.

rjmateus · 2025-02-24T21:05:55Z

java/code/src/com/suse/manager/api/HttpApiRegistry.java

@@ -136,7 +139,7 @@ public static Set<String> getUnautenticatedRoutes() {
     */
    private void registerAuthEndpoints() {
        registrationHelper.addPostRoute(HTTP_API_ROOT + "auth/login", LoginController::apiLogin);
-        registrationHelper.addPostRoute(HTTP_API_ROOT + "auth/logout", withUser(LoginController::logout));
+        registrationHelper.addGetRoute(HTTP_API_ROOT + "auth/logout", withUser(LoginController::logout));


Why was this changed from get to post? Will this influence the way other pages/locations call this method? Or is this endpoint only used by costumers? If that is the case, we would need to add something to release notes.

I think it was a mistake in the initial HTTP API implementation. In XML-RPC it's @ReadOnly which means it's supposed to be a GET request in HTTP API (also no payload). That's why I changed it now. The change only affects the HTTP API (and the testsuite, which is included in the PR).

And yes, since it's on the public API, we'll need a release note.

rjmateus · 2025-02-24T21:08:21Z

java/code/src/com/redhat/rhn/common/security/acl/Access.java

+        boolean authorized = UserManager.getPermittedNamespaces(user)
+                .filter(ns -> params[0].equals(ns.getNamespace()) ||
+                        ns.getNamespace().startsWith(params[0] + '.')
+                ).anyMatch(ns -> params.length < 2 ||


we should have some comments explaining this magic number. Why 2? (I'm assuming that it must have and least one parent, otherwise it would match the full path)

Right, this one needs a description.

Second element in params is the optional "access mode". If not provided, either R or W would match.

Here are some examples: 4f50a5d

rjmateus · 2025-02-24T21:13:45Z

java/code/src/com/redhat/rhn/frontend/xmlrpc/BaseHandler.java

@@ -136,6 +141,15 @@ public Object invoke(String methodCalled, List params) throws XmlRpcFault {
            }
        }

+        try {


We should consider if we should continue to have the read-only user logic or if that should/can be replaced by a User group.
I think we must replace the read-only API user with a new group in the logic that is being implemented in the RBAC, and in that case, the code that is placed a few lines above (L18-L142) should be removed.

Yes, the read-only logic can be fully replaced with RBAC. And yes, there'll be many obsolete parts then :)

rjmateus · 2025-02-24T21:24:54Z

java/code/src/com/redhat/rhn/manager/user/UserManager.java

+     * @param mode the access mode (view/modify)
+     * @throws PermissionException if the user is not permitted to access the specified namespace
+     */
+    public static void ensureRoleBasedAccess(User user, String namespace, Namespace.AccessMode mode) {


I think this method is not used. It also feels strange, since it doesn't return anything and checks user access and through an exception if he doesn't have access to the endpoint.

Oh this one has a long story:

Imagine a struts page that renders a JSP table with pagination buttons, select all button, etc.
On top of that, there's a button to do some real action on selected rows.

In Struts, all the pagination and the actual action are handled in the same handler with the same POST endpoint. However, pagination is a read operation, while the action is usually a "modify".

Since the endpoint is the same, the only sensible way is to make the differentiation in the handler code.
So this method is called from these handlers. And the way it doesn't return but throws is for convenience.

Here's some example usages: d4517b3

rjmateus · 2025-02-24T21:27:59Z

java/code/src/com/redhat/rhn/manager/user/UserManager.java

+     * @return the stream of permitted namespaces
+     */
+    public static Stream<Namespace> getPermittedNamespaces(User user) {
+        return Stream.concat(user.getNamespaces().stream(),


I think we can have this method with a more efficient implementation, where we could get all the user namespaces with a single query execution. We don't need to change it for this PR, but we should refactor it.

I still need to test the performance of different approaches, but with eager fetching, Hibernate should perform good enough on this.

rjmateus · 2025-02-24T21:35:12Z

java/code/src/com/redhat/rhn/frontend/servlets/AuthorizationFilter.java

+
+    private void handleAjaxAccess(HttpServletRequest hreq, Set<String> noAuthEndpoints) {
+        String path = hreq.getServletPath() + hreq.getPathInfo();
+        if (!authenticationService.requestURIRequiresAuthentication(hreq) || noAuthEndpoints.contains(path)) {


I would prefer to not have a dependency from AuthenticationServiceFactory, but I was looking at the endpoints in there and they are super generic (like /css), so would be hard to move them to the database.

rjmateus · 2025-02-24T21:41:35Z

java/code/src/com/redhat/rhn/frontend/servlets/AuthorizationFilter.java

+        RequestContext requestContext = new RequestContext(hreq);
+        User user = requestContext.getCurrentUser();
+        if (user == null) {
+            if (!noAuthEndpoints.contains(route.getMatchUri())) {


I think we can just throw the exception without the if statement, since we are already checking the noAuthEndpoints in line 173.
HOwever, it doesn't cause any issue if we keep it as is. Up to you to decide :)

Right, I guess I overlooked this one.

rjmateus · 2025-02-24T21:42:53Z

java/code/src/com/redhat/rhn/frontend/servlets/AuthorizationFilter.java

+
+        User user = new RequestContext(hreq).getCurrentUser();
+        if (user == null) {
+            if (!noAuthEndpoints.contains(hreq.getServletPath())) {


Same comment from the handleSparkAccess, up to you if you want to keep this check.

cbbayburt requested a review from rjmateus February 24, 2025 18:58

cbbayburt requested review from a team as code owners February 24, 2025 18:58

github-actions bot added java java_lint_checkstyle java_pgsql_tests API schema_migration_test_pgsql database schema_migration_test_oracle webapp data-model xmlrpc old-ui labels Feb 24, 2025

cbbayburt force-pushed the rbac-base branch from 6fe6cf6 to ef72ae9 Compare February 24, 2025 19:25

cbbayburt changed the base branch from master to rbac-base-schema February 24, 2025 19:49

Add experimental config flag for RBAC

34f4db2

cbbayburt force-pushed the rbac-base branch 2 times, most recently from ad0e46e to b0574d1 Compare February 24, 2025 20:33

cbbayburt force-pushed the rbac-base-schema branch from 6540ae8 to 843333c Compare February 24, 2025 20:36

cbbayburt changed the base branch from rbac-base-schema to master February 24, 2025 20:54

cbbayburt requested a review from a team as a code owner February 24, 2025 21:34

github-actions bot added testing ruby_rubocop test-framework labels Feb 24, 2025

rjmateus previously approved these changes Feb 24, 2025

View reviewed changes

cbbayburt added 3 commits February 24, 2025 23:24

Implement authorization filters for spark, struts and ajax routes

2f2c7c7

Fix unauthorized routes in ApiHandler

b023419

ACL method for RBAC namespaces in struts

7b5a8fe

cbbayburt dismissed rjmateus’s stale review via 7b5a8fe February 24, 2025 22:24

cbbayburt force-pushed the rbac-base branch from c902072 to 7b5a8fe Compare February 24, 2025 22:24

cbbayburt merged commit 7b5a8fe into master Feb 24, 2025
25 of 26 checks passed

cbbayburt deleted the rbac-base branch February 24, 2025 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC: Initial implementation #9844

RBAC: Initial implementation #9844

cbbayburt commented Feb 24, 2025 •

edited

Loading

github-actions bot commented Feb 24, 2025

rjmateus left a comment

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

rjmateus Feb 24, 2025

cbbayburt Feb 24, 2025

rjmateus Feb 24, 2025

                           }
                       }
+                      try {

RBAC: Initial implementation #9844

RBAC: Initial implementation #9844

Conversation

cbbayburt commented Feb 24, 2025 • edited Loading

Documentation

Test coverage

Changelogs

Re-run a test

Before you merge

github-actions bot commented Feb 24, 2025

rjmateus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbbayburt commented Feb 24, 2025 •

edited

Loading