Repository navigation
Conversation
9701da9 to
7febb85
Compare
91a8ab8 to
e04dec7
Compare
|
@srose Thanks for the reminder to get this done at KC Dev Day :). Finally got around to finishing the PR. I tested everything except the migration, as I currently cannot get the build to complete on my machine once again. But since I (and Claude) basically lifted the code wholesale from the previous migration which did pretty much the exact same thing, I would expect it to work as well. |
|
I just realized that 26.7.0 is not yet released, so I could also adapt the PR to put the new role into the same migration and add it to the changelog. It would also fit the theme quite well, as 26.7.0 also adds a bunch new roles. Let me know if I should change it - I'm holding off on it since I don't know if you are already in code freeze. |
There was a problem hiding this comment.
Thank you for this PR.
The code-freeze is still ~1.5 months in the future, so please adjust the PR so that its changes are part of 26.7.
Nitpick: Update
role_query-users
I tried to use the new permission with the Admin UI, and I found that with just the "export realm" permission, I wasn't able to navigate to the "Realm Setting" menu entry. Once I added "view realm" permissions, the drop-down action "partial export" was disabled.
This is what I would expect: When I have the "export-realm" role, I can navigate in the Admin UI to the "Realm settings" menu entry, and I can trigger the action "Partial export".
About the different tabs in the "Realm settings": Ideally they would be hidden.
Thanks again for the PR, and sorry that I didn't provide an earlier feedback.
|
I will make the changes to the migration and add the documentation strings soon, thanks for the heads up.
Thats actually a good point. I primarily designed the role for programmatic (API) access, and didn't really think about the implications for the UI. Do you think changing the UI would be a good idea? I felt that not being able to log into the admin console and see things would actually be a perk for the primary use case I forsee (programmatically downloading a config dump using a service account to do things like updating a Confluence page, running kcwarden, or similar things), but I realize now that this may be inconsistent with how you like to do things. |
My approach was to do some explorative testing for this new feature. The first step is then to set up a user, and see if it can access the API endpoint. As the PR doesn't contain any integration testing around this, and I didn't have the API endpoint at hand, this was the first thing I tried. And I assume an admin might want to do something similar when they set up that read-only user. As basically all existing roles work with the Admin UI, I consider this part of it. One more thing: I know we are very bad in describing what each role can do, and what its expected use is. Still, please add a paragraph to the docs about this new role. |
I could not find the place where the roles are actually documented - can you point me to the place in the docs where you think this would fit in? |
I'd suggest adding it to the |
|
Ah, I didn't do that because then it would be the only one that has such a documentation. So, even though that would be inconsistent with the rest, I should add it anyway? I can do that, no problem, just making sure. |
Yes. Someone would need to make a start and lead by example. |
|
✅ Fixed the UI With view-realm and export-realm: Without view-realm but with export-realm: 🤖 Disclosure: this code was written by Claude and reviewed and tested by me. However, since I am not familiar with the codebase, please take extra care in reviewing the .tsx files to see if it matches the expected code style. I (and Claude ;) could not find any relevant test cases that would have to be changed or added for this (there does not seem to be a pattern of testing with less privileged user accounts at the moment, and the endpoint checks are already tested by the existing tests added earlier). Let me know if I missed any. ✅ Moved migration code to 26.7 migration ✅ Added label and description to the roles Still in progress: Will add the information about the role to the documentation in a bit, but wanted to get the functional improvements out the door so you can take another look :). |
e04dec7 to
36a15be
Compare
36a15be to
bd1fb4b
Compare
|
I now also pushed the documentation update. I tried to create a description for each of the roles, to make the AI Disclosure: I had Claude check what each of the roles was doing, challenging its assertions based on what I know myself about these roles. However, there may still be inaccuracies that I am not aware of. |
bd1fb4b to
c45abfe
Compare
|
Looks like I fixed the linter issues that made the pipeline go red, and the new red pipeline is just a flaky test that affects more than just my branch, so I won't try to fix that one :). |
c45abfe to
bbc8ecc
Compare
|
Updated to resolve the conflict against main. |
bbc8ecc to
be4235d
Compare
|
Once again resolved conflicts against main. @ahus1 the last run had an error in the integration tests against the account frontend. Since I didn't change anything about that, I assume that it was not related to my changes, so I didn't try to fix it. Let's see if it crops up again in these workflows. From my side, the PR is ready for review and merge, if the pipeline doesn't fail again. |
This commit implements a new role, 'realm-export', that gives access to the partialExport endpoint. See the linked issue and the pull request description for more details. Closes keycloak#44797 Signed-off-by: Max Maass <max.maass@iteratec.com>
be4235d to
ef32916
Compare
|
@pedroigor - I had a look at this PR and it looks good to me. It is in your team's area, so it would be great to have feedback from you as well. I'm happy to see that it adds docs for the different roles that was on our roadmap for quite a while! |
|
For transparency: To avoid redundant work, I'll hold off on rebasing off main until we have agreement in principle that this should be merged - otherwise, I'll have to keep unnecessarily chasing commits. Just let me know once you need me to do this. :) |
Closes #44797
This PR implements part of the work required to add a new
export-realmrole, as discussed in #44797.Current status:
PermissionsTest.java.PartialExportTest.javacurrently does not test any roles, so I have not adapted it.Feel free to add anything else I have forgotten in the checklist.
Ping @ahus1 @srose - and thanks for your help in getting this far :).
AI disclosure: I used AI (Claude Code) to orient myself within the code base and find the right places for test cases. I wrote the functional code myself. The test cases (four lines of code) and the migration were generated by Claude Opus 4.6 and reviewed by me. (edit: further disclosures in the thread below)