-
Notifications
You must be signed in to change notification settings - Fork 18
Integrate the new authorization component #3908
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
Integrate the new authorization component #3908
Conversation
b2449bd to
8463c1f
Compare
|
@heger I rebase this PR locally, built the core image, and deployed to my local Docker Compose to look into the required UI changes. It looks like it imported the roles correctly (both users are in the superusers group): However, when I try to create an organization the request fails with this exception: I also see this exception in the logs: Also listing the organizations fails, there is one organization that I should see as superuser, but I get an empty list because the query looks like this ( Do you have an idea what could be the problem? |
|
@mnonnenmacher, the stacktrace indicates that the username cannot be extracted from the access token. It is expected to be contained in the |
With the upcoming changes in [1] it will be required that the name is not empty. Before, the admin user did not have any name set which lead to the `name` claim missing from the JWT. [1]: #3908 Signed-off-by: Martin Nonnenmacher <[email protected]>
|
b2ac338 to
d3f1157
Compare
e2ceb33 to
4a62e9d
Compare
4a62e9d to
42f61ec
Compare
|
Can I ask about the status of this PR? I'd need it to advance my search component. |
We have been doing tests this week, and so far, everything seems to be looking fine. So, I assume that it is going to be merged soon. |
2064a50 to
298803d
Compare
When installing the Keycloak extension, drop the `createRealmPerTest` flag per default. The additional isolation is no longer needed for most tests, since no roles or other data in Keycloak are manipulated during test execution, and an initial setup of test users used by the test cases is sufficient. For tests that do manipulate the state in Keycloak, support enabling this feature on demand. Avoiding the repeated setup of the realm saves a few seconds for every test case, which has a notable effect on the total test execution times. Signed-off-by: Oliver Heger <[email protected]>
The filter's `isWildcard` flag was always set to `true` for superusers. This prevented the `containedIn` filter to be applied correctly. Fix this by taking the presence of a `containedIn` filter into account. Signed-off-by: Oliver Heger <[email protected]>
In the endpoint to fetch the products of an organization, apply a `HierarchyFilter`. Extend `OrganizationService` accordingly. This makes sure that only products are listed that are visible to the user. If a user has only been granted access to specific repositories, he or she should only see the products these repositories belong to, even if there is an implicit READ right on the organization. Signed-off-by: Oliver Heger <[email protected]>
In the endpoint to fetch the repositories of a product, apply a `HierarchyFilter`. Extend `ProductService` accordingly. This makes sure that only repositories are listed that are visible to the user. By having access to some repositories, the user gets implicit READ permission on the owning products. However, in these products, not automatically all repositories are visible. Signed-off-by: Oliver Heger <[email protected]>
6a53bfb to
3da4140
Compare
Create a new `RolesToDbMigration` class offering a function that can create structures in the database that correspond to the roles and permissions currently kept in Keycloak. The function is going to be called on startup of ORT Server to perform a one-time migration. Signed-off-by: Oliver Heger <[email protected]>
Instead of doing a synchronization of roles in Keycloak, trigger a migration to new access rights structures on startup of the core component. Note that after starting ORT Server with this change, the new data structures are considered the source of truth for authorization information. To do a rollback, the `role_assignments` table should be manually cleaned. Signed-off-by: Oliver Heger <[email protected]>
This endpoint is going to be used by the UI to adapt itself according to the status of the current user. Make a function of AbstractAuthorizationTest public which can be reused here to access the authorization infrastructure. Signed-off-by: Oliver Heger <[email protected]>
The existing interface supported only checks whether a specific permission is granted or not. For some use cases, however, the full set of available permissions needs to be known. Therefore, extend the interface to provide this information. Implement the methods that check permissions as standard functions in the interface that delegate to the new functions. Signed-off-by: Oliver Heger <[email protected]>
This endpoint returns information about the current user and their permissions on a specific hierarchy element. It is going to be consumed by the UI which can use this information to adapt itself according to the user's rights. Signed-off-by: Oliver Heger <[email protected]>
Signed-off-by: Oliver Heger <[email protected]>
For authorized routes, the `generateOpenApiSpec` task generated wrong URLs, since the route selector was included. Rework the way the routes are added to the routing graph to fix this problem. Signed-off-by: Oliver Heger <[email protected]>
It is not guaranteed that the `name` claim in the access token is present. Since this information is not relevant for the business logic, make the `OrtServerPrincipal.fullName` property nullable and replace it with an empty string when it should be displayed. Signed-off-by: Oliver Heger <[email protected]>
Make sure that always all levels are processed when applying a `HierarchyFilter`; otherwise, the generated filter condition may be incomplete. Signed-off-by: Oliver Heger <[email protected]>
Add a new `permissionGrantedOnLevel()` function that returns the level on which permissions are granted. This can be used to distinguish between rights that have been explicitly granted and rights that are inherited through the hierarchy. This distinction is necessary for some use cases, for instance, when displaying the users having access to a specific element. Signed-off-by: Oliver Heger <[email protected]>
Introduce a new `RoleInfo` class that stores additional information on which hierarchy level a role has been assigned to a user. Change `AuthorizationService.listUsers()` to return such `RoleInfo` objects. This additional information can be used by the UI to distinguish between users who have been explicitly assigned to a hierarchy element and those with roles inherited from other elements. For now, the REST API only uses this new functionality to filter out users with inherited roles. This can be extended later to support more advanced views about users in the UI. Signed-off-by: Oliver Heger <[email protected]>
Include the `ort-server-client` client scope in the `ort-server-ui` and `ort-server-ui-dev` clients by default. This ensures that the `ort-server` client is included in the audience claim even when the user has no roles from that client assigned. Otherwise, the backend will not accept the JWT from such users. Signed-off-by: Martin Nonnenmacher <[email protected]>
Use the previously introduced endpoint to get information about the user's superuser status and permissions. The superuser status is made available globally via the `useUser` hook while hierarchy related permissions are made available via the router context. As before, if the user has the superuser status, checking the specific permissions is skipped. Also, the results of all queries are cached for 60s to reduce the amount of backend calls. This means that it can take up to 60s for some permission changes to be reflected by the UI. Signed-off-by: Martin Nonnenmacher <[email protected]>
3da4140 to
1943894
Compare
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.
UI code LGTM, but I'd like @lamppu to review as well.
Question to PR authors: I assume you have tested that the UI works? Specifically:
- that trying to access non-authed routes manually (by typing a forbidden route to the URL) leads to proper error
- that it is possible to CRUD secrets
- that it is possible to CRUD infrastructure services
- that when creating infrastructure services, the username and password secret dropdowns are properly populated by existing secrets inherited from all hierarchy levels
- that in the ORT run creation form, Analyzer / environment secret dropdown is populated likewise
- and in the same form, infra services dropdown for Analyzer / environment config is populated likewise
@mnonnenmacher, can you comment what you have tested in the UI? I went through some use cases with a restricted user. Since there are comprehensive integration tests for the API endpoints and the roles/permissions they require, I would expect that there are no behavior changes in this regard. |
lamppu
left a comment
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.
UI code LGTM, but I'd like @lamppu to review as well.
LGTM too for the most part, the only small thing is that the hasRole function could be completely removed from the useUser hook.
I will do that as a follow-up. |

This PR should contain everything that is needed to switch to the new authorization component:
This is still WIP.