CCCHH/keycloak-attribute-endpoints-provider
0
0
Fork 0
Code Issues 5 Pull requests 5 Projects Releases Packages Activity Actions

Add exporting individual attributes and a map endpoint #26

Open
stb wants to merge 3 commits from add-map-endpoint into main
pull from: add-map-endpoint
merge into: CCCHH:main
Conversation 11 Commits 3 Files changed 12 +633 -250
stb commented 2026-07-12 19:22:06 +02:00
Owner
Copy link

Based off #32

Also:

  • rework README.md
  • improve configuration error messages

Closes #22

Based off #32 Also: * rework README.md * improve configuration error messages Closes #22
Fix matchRole lookup to include group-inherited and composite roles
All checks were successful
/ Verify (push) Successful in 1m8s
Details
/ Verify (pull_request) Successful in 46s
Details
2c3c0a3dab 
getRoleMembersStream only returns users with the role assigned
directly, so exported attributes silently excluded users who have
matchRole via group membership. Filter all realm users with
hasRole() instead, matching the resolution already used for authRole.
Add returning attributes split by name
All checks were successful
/ Verify (push) Successful in 44s
Details
/ Verify (pull_request) Successful in 48s
Details
464c02dfb0 
Adds an endpoint that returns the list of attribute values as a map of attribute names in the requested attribute group.

This is useful for requesting the mailing list addresses, where the individual attributes apply to different mailing lists.
stb requested reviews from june, kritzl, lilly 2026-07-12 19:22:22 +02:00
kritzl requested changes 2026-07-14 14:57:51 +02:00
Dismissed
kritzl left a comment
Copy link

Haven't tested the PR yet, but it looks reasonable so far.

Haven't tested the PR yet, but it looks reasonable so far.
attribute-endpoints-provider/src/main/java/de/ccc/hamburg/keycloak/attribute_endpoints/AttributeEndpointsResourceProvider.java Outdated
@ -146,0 +83,4 @@
* @return
*/
@GET
@Path("export/{attributeGroupName}/map")
kritzl commented 2026-07-14 14:55:47 +02:00
Member
Copy link

why is the slug named attributeGroupName in this path? As far as I can see, it has no connection to the actual attribute group name and can still be chosen freely.

why is the `slug` named `attributeGroupName` in this path? As far as I can see, it has no connection to the actual attribute group name and can still be chosen freely.
kritzl commented 2026-07-14 15:02:26 +02:00
Member
Copy link

@stb

Since @june and I have put a lot of time and energy into this project and I’d like to continue standing behind the codebase, I’d like to ask you to clarify whether the code was modified by an AI Agent or if you refactored it by hand.

@stb Since @june and I have put a lot of time and energy into this project and I’d like to continue standing behind the codebase, I’d like to ask you to clarify whether the code was modified by an AI Agent or if you refactored it by hand.
stb commented 2026-07-14 15:22:22 +02:00
Author
Owner
Copy link

It's done by hand, but it's not correct. Give me some time to wrap my head around everything.

It's done by hand, but it's not correct. Give me some time to wrap my head around everything.
stb changed title from Add an endpoint producing the individual attribute values per name to WIP: Add an endpoint producing the individual attribute values per name 2026-07-14 15:22:48 +02:00
Merge branch 'main' into add-map-endpoint
Some checks failed
/ Verify (pull_request) Failing after 39s
Details
/ Verify (push) Failing after 40s
Details
9073ef086a 
* simplify AttributeEndpointsResourceProvider
* improve explanation of properties in AdminUiPage
* add example for mailing lists and map endpoint
* better explain configuration and operation of provider in main readme
* adjust the Keycloak realm
* Add unit test for endpoint class
Add running tests
All checks were successful
/ Test (push) Successful in 52s
Details
/ Test (pull_request) Successful in 54s
Details
1c2adb1a92
stb changed title from WIP: Add an endpoint producing the individual attribute values per name to Refactor and expand 2026-07-18 18:28:43 +02:00
stb requested review from kritzl 2026-07-18 18:30:29 +02:00
stb removed review request for lilly 2026-07-18 21:15:01 +02:00
kritzl reviewed 2026-07-18 21:33:52 +02:00
README.md Outdated
@ -20,79 +42,8 @@ Once all dependencies are met, simply call `make` to build the provider, which s
There's also `make clean` available for removing the output directory.
To add the provider to your Keycloak install, copy `attribute-endpoints-provider/target/attribute-endpoints-provider-1.0-SNAPSHOT.jar` to the Keycloak Provider directory (`/opt/keycloak/providers/).
kritzl commented 2026-07-18 21:33:52 +02:00
Member
Copy link

missing backtick:

- (`/opt/keycloak/providers/)
+ (`/opt/keycloak/providers/`)
missing backtick: ```diff - (`/opt/keycloak/providers/) + (`/opt/keycloak/providers/`) ```
stb marked this conversation as resolved
Typo
All checks were successful
/ Test (push) Successful in 52s
Details
/ Test (pull_request) Successful in 51s
Details
7452879ef5
Merge branch 'main' into add-map-endpoint
All checks were successful
/ Test (pull_request) Successful in 52s
Details
6ef6d7adeb
kritzl requested changes 2026-07-18 21:46:45 +02:00
kritzl left a comment
Copy link

Is it possible to split this PR into seperate PRs? It's too confusing for me to compare the large diff and a review commit by commit is also not possible, as some thing are reverted by a following commit.

I propose:

  • First PR
    • refactor code without adding new features
    • improve explanation of properties in AdminUiPage
    • better explain configuration and operation of provider in main readme
    • add example for mailing lists and map endpoint
    • adjust the Keycloak realm
  • Second PR
    • extend AttributeEndpointsResourceProvider to add a mapped result (may not be needed immediately)
  • Third PR
    • Add unit test for endpoint class
    • Add running tests

Does this work for you @stb?
I can also try wrapping my head around this and split the PR, but I don't think I will have time for that in the neear future :/

Is it possible to split this PR into seperate PRs? It's too confusing for me to compare the large diff and a review commit by commit is also not possible, as some thing are reverted by a following commit. I propose: - First PR - refactor code _without adding new features_ - improve explanation of properties in AdminUiPage - better explain configuration and operation of provider in main readme - add example for mailing lists and map endpoint - adjust the Keycloak realm - Second PR - extend AttributeEndpointsResourceProvider to add a mapped result (may not be needed immediately) - Third PR - Add unit test for endpoint class - Add running tests Does this work for you @stb? I can also try wrapping my head around this and split the PR, but I don't think I will have time for that in the neear future :/
stb commented 2026-07-18 22:13:10 +02:00
Author
Owner
Copy link

Hhm, splitting this up is a lot of additional work, and it risks introducing additional unwanted changes in the process. Let me think about what it would take to split it up like that.

Hhm, splitting this up is a lot of additional work, and it risks introducing additional unwanted changes in the process. Let me think about what it would take to split it up like that.
stb commented 2026-07-18 22:14:45 +02:00
Author
Owner
Copy link

CAn you at least try and run it as is and tell me whether the tests and the docker compose integration testing are working for you?

CAn you at least try and run it as is and tell me whether the tests and the docker compose integration testing are working for you?
kritzl commented 2026-07-18 23:02:34 +02:00
Member
Copy link

@stb wrote in #26 (comment):

CAn you at least try and run it as is and tell me whether the tests and the docker compose integration testing are working for you?

Yes, will try to test the PR tomorrow

@stb wrote in https://git.hamburg.ccc.de/CCCHH/keycloak-attribute-endpoints-provider/pulls/26#issuecomment-5641: > CAn you at least try and run it as is and tell me whether the tests and the docker compose integration testing are working for you? Yes, will try to test the PR tomorrow
stb referenced this pull request from a commit 2026-07-19 10:28:28 +02:00
Add just the testing setup from #26.
stb referenced this pull request from a commit 2026-07-19 10:45:11 +02:00
Add just the refactoring from #26.
stb commented 2026-07-19 10:47:02 +02:00
Author
Owner
Copy link

I've created #31 for the testing infrastructure, and #32 for the refactoring. After merging those, this can be rebased for an easier review. I can split out the doc changes, but I think it's easy enough to do those as part of this.

I've created #31 for the testing infrastructure, and #32 for the refactoring. After merging those, this can be rebased for an easier review. I can split out the doc changes, but I think it's easy enough to do those as part of this.
stb referenced this pull request from a commit 2026-07-19 13:22:06 +02:00
Add just the refactoring from #26.
stb force-pushed add-map-endpoint from 6ef6d7adeb
All checks were successful
/ Test (pull_request) Successful in 52s
Details
to c9f36f7100
All checks were successful
/ Verify (pull_request) Successful in 41s
Details
2026-07-19 13:38:27 +02:00 Compare
stb changed title from Refactor and expand to Add exporting individual attributes 2026-07-19 13:39:16 +02:00
stb changed title from Add exporting individual attributes to Add exporting individual attributes and a map endpoint 2026-07-19 13:39:30 +02:00
stb commented 2026-07-19 13:40:06 +02:00
Author
Owner
Copy link

I've rebased this PR off #32. I hope the three PRs together make the changes clearer.

I've rebased this PR off #32. I hope the three PRs together make the changes clearer.
stb commented 2026-07-19 13:40:55 +02:00
Author
Owner
Copy link

If people are happy with everything, merging #31 and #32 can be skipped, since this includes all changes.

If people are happy with everything, merging #31 and #32 can be skipped, since this includes all changes.
All checks were successful
/ Verify (pull_request) Successful in 41s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin add-map-endpoint:add-map-endpoint
git switch add-map-endpoint

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff add-map-endpoint
git switch add-map-endpoint
git rebase main
git switch main
git merge --ff-only add-map-endpoint
git switch add-map-endpoint
git rebase main
git switch main
git merge --no-ff add-map-endpoint
git switch main
git merge --squash add-map-endpoint
git switch main
git merge --ff-only add-map-endpoint
git switch main
git merge add-map-endpoint
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
june
kritzl
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Depends on
#25 Fix matchRole lookup to include group-inherited and composite roles
CCCHH/keycloak-attribute-endpoints-provider
Reference
CCCHH/keycloak-attribute-endpoints-provider!26
No description provided.