Filtering organization groups when managing or processing groups by pedroigor · Pull Request #30590 · keycloak/keycloak

pedroigor · 2024-06-19T21:05:01Z

Adds a type to groups to filter organization groups from group queries. The reason for using a type is to avoid a nullable column and potentially impact database indexes and also eventually leverage groups for purposes other than realm and organization
Adds filtering to the user cache layer to filter organization groups when returning the groups the user is a member
Organization groups are no longer shown in the Group and User UIs and this improves the UX
Organization groups are no longer added to tokens when using a group mapper
Listing organizations will not automatically fetch their groups because the attributes are removed from the representation
Deleting a realm will remove organization and groups using a query rather than iterate over groups/orgs as before
Adding a database index on ORG_DOMAIN for the ORG_ID. Queries run a lot faster when listing organizations. For instance, from Execution Time: 129.512 ms to Execution Time: 0.084 ms in a database with 100k organizations each one with 10 domains.

keycloak-github-bot · 2024-06-19T21:46:41Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.organization.exportimport.OrganizationExportTest#testExport

Keycloak CI - Base IT (3)

java.lang.AssertionError: expected:<201> but was:<404>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

jakarta.ws.rs.NotFoundException: HTTP 404 Not Found
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.handleErrorStatus(ClientInvocation.java:242)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.extractors.DefaultEntityExtractorFactory$3.extractEntity(DefaultEntityExtractorFactory.java:41)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invokeSync(ClientInvoker.java:136)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invoke(ClientInvoker.java:103)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-06-20T16:07:55Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.broker.KcSamlBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - FIPS IT (strict)

java.lang.AssertionError: expected:<Invalid authenticator code.> but was:<null>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

vramik

@pedroigor Thank you for preparing the PR. I have few comments/questions, please see below.

Overall it seems good. Conceptually I'm not convinced about the fact that a Group and an Organization would share the same ID in the database to simulate @OneToOne relationship without explicitly defining it in the entity. I believe it could bring several issues and overall it's not very good practice.

There are several points in general why I believe so:

Data integrity: it violates the entity uniqueness.
Lack of referential integrity: FK constrains, cascading operations
ORM Tooling Limitations like annotations, without them the ORM cannot manage relationships correctly
Personally I find it more error prone and less readable

I am not saying we suffer from any of those, but IMO it'd be safer to discuss this matter in broader team to get an agreement that Keycloak wants to go this route.

pedroigor · 2024-06-26T11:14:04Z

There are several points in general why I believe so:

Data integrity: it violates the entity uniqueness.

There is no data integrity violation because IDs for organizations and groups are still unique.

Lack of referential integrity: FK constrains, cascading operations

We already lack FKs and cascading in other places. I'm not changing that now. Please, consider we have already discussed this with others in the team.

ORM Tooling Limitations like annotations, without them the ORM cannot manage relationships correctly

We already have places where we don't use relationships like OneToMany or ManyToOne and I think for performance reasons. The previous code, for instance, was just using a column even though we had a one-to-one behind-the-scenes but with different IDs.

Personally I find it more error prone and less readable

I guess you are used to the same approach in other places where we don't have relationships defined explicitly. And I think that is fine.

I am not saying we suffer from any of those, but IMO it'd be safer to discuss this matter in broader team to get an agreement that Keycloak wants to go this route.

If we don't suffer from any of those, why we are discussing it :)

pedroigor · 2024-06-26T11:16:40Z

@vramik Do you know how we can fix https://github.com/keycloak/keycloak/actions/runs/9678438075/job/26702457200?pr=30590?

vramik

I am leaving the decision about the IDs up to you.

pedroigor · 2024-06-27T13:18:43Z

@vramik The decision has been taken since I opened the PR :) Please, consider it as OK from my side.

pedroigor · 2024-06-27T15:14:46Z

I've changed the PR to not share the same ID anymore as I found another issue when migrating (also had to update group attributes table).

Closes keycloak#30589 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

vramik

@pedroigor I didn't have a chance to look at the latest changes of the PR. but I believe it goes to the right direction, approving now to unblock merging.

pedroigor · 2024-06-28T12:56:32Z

Added #30919 as a follow-up.

ahus1

Approving based on @vramik's review.
Not merging it yet in case you want someone else to review it.

mposolda

Approving. I did not review very deeply. Relying on the review by @vramik . But the ideas behind the PRs look good to me per our discussion with @pedroigor

pedroigor · 2024-06-28T13:27:16Z

@mposolda @vramik Tks.

pedroigor changed the title ~~Issue 30589~~ Filtering organization groups when managing or processing groups Jun 19, 2024

keycloak-github-bot Bot added the flaky-test label Jun 19, 2024

This was referenced Jun 19, 2024

Flaky test: org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken #30111

Closed

Flaky test: org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled #30188

Closed

keycloak-github-bot Bot reviewed Jun 19, 2024

View reviewed changes

pedroigor force-pushed the issue-30589 branch from 3fb2729 to e749c41 Compare June 20, 2024 11:49

keycloak-github-bot Bot mentioned this pull request Jun 20, 2024

Flaky test: org.keycloak.testsuite.organization.exportimport.OrganizationExportTest#testExport #30598

Closed

pedroigor force-pushed the issue-30589 branch from e749c41 to 9c66d2c Compare June 20, 2024 14:54

keycloak-github-bot Bot reviewed Jun 20, 2024

View reviewed changes

pedroigor force-pushed the issue-30589 branch 7 times, most recently from f7d3eda to 385945d Compare June 25, 2024 18:52

pedroigor marked this pull request as ready for review June 25, 2024 18:54

pedroigor requested review from a team as code owners June 25, 2024 18:54

keycloak-github-bot Bot added team/cloud-native team/core-clients team/core-iam labels Jun 25, 2024

pedroigor force-pushed the issue-30589 branch from dbfc93b to 2e884a8 Compare June 25, 2024 22:37

vramik suggested changes Jun 26, 2024

View reviewed changes

pedroigor force-pushed the issue-30589 branch from 2e884a8 to cd1e3b4 Compare June 26, 2024 11:02

pedroigor force-pushed the issue-30589 branch 6 times, most recently from 6269657 to afe2410 Compare June 27, 2024 12:36

vramik reviewed Jun 27, 2024

View reviewed changes

Comment thread ...k/connections/jpa/updater/liquibase/custom/JpaUpdate26_0_0_MariaDBOrganizationGroupType.java Outdated

vramik requested review from vramik and removed request for vramik June 27, 2024 12:43

vramik reviewed Jun 27, 2024

View reviewed changes

pedroigor force-pushed the issue-30589 branch from afe2410 to 4886a56 Compare June 27, 2024 15:15

keycloak-github-bot Bot mentioned this pull request Jun 27, 2024

Flaky test: org.keycloak.testsuite.oauth.TokenIntrospectionTest#testUnsupportedToken #30884

Merged

Filtering organization groups when managing or processing groups

3a222da

Closes keycloak#30589 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

pedroigor force-pushed the issue-30589 branch from 4886a56 to 3a222da Compare June 27, 2024 19:53

vramik approved these changes Jun 28, 2024

View reviewed changes

pedroigor mentioned this pull request Jun 28, 2024

Update migration and upgrade guides about GroupRemovedEvent no longer fired when removing a realm #30919

Closed

ahus1 approved these changes Jun 28, 2024

View reviewed changes

mposolda approved these changes Jun 28, 2024

View reviewed changes

pedroigor merged commit cc2ccc8 into keycloak:main Jun 28, 2024

pedroigor deleted the issue-30589 branch June 28, 2024 13:27

vmuzikar mentioned this pull request Sep 6, 2024

Upgrade to Quarkus 3.14.2 #32519

Merged

Conversation

pedroigor commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keycloak-github-bot Bot commented Jun 19, 2024

Unreported flaky test detected

org.keycloak.testsuite.organization.exportimport.OrganizationExportTest#testExport

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot Bot commented Jun 20, 2024

Unreported flaky test detected

org.keycloak.testsuite.broker.KcSamlBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pedroigor commented Jun 26, 2024

Uh oh!

pedroigor commented Jun 26, 2024

Uh oh!

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jun 27, 2024

Uh oh!

pedroigor commented Jun 27, 2024

Uh oh!

vramik left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jun 28, 2024

Uh oh!

ahus1 left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jun 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pedroigor commented Jun 19, 2024 •

edited

Loading