8000 Issue with Handling Negative Values in Certain Fields of Brute Force Detection by douglaspalmer · Pull Request #40538 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Issue with Handling Negative Values in Certain Fields of Brute Force Detection #40538

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 24, 2025
Merged

Issue with Handling Negative Values in Certain Fields of Brute Force Detection #40538

merged 2 commits into from
Jun 24, 2025

Conversation

douglaspalmer
Copy link
Contributor

Closes #40353

@douglaspalmer douglaspalmer requested review from a team as code owners June 16, 2025 22:37
keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jun 16, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

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.cluster.ClientScopeInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

org.keycloak.testsuite.cluster.RealmInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

org.keycloak.testsuite.cluster.SessionFailoverClusterTest#sessionFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

rmartinc
rmartinc suggested changes Jun 17, 2025
View reviewed changes
Copy link
Contributor
@rmartinc rmartinc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglaspalmer I think that it would better to check this when moving from rep to model in the DefaultExportImportManager. And I always prefer to return an error than doing nothing. WDYT?

And try with this commit rmartinc@2204992 in the UI part. It's not perfect but much better.

@douglaspalmer douglaspalmer marked this pull request as draft June 18, 2025 00:32
@douglaspalmer douglaspalmer marked this pull request as draft June 18, 2025 00:32
@douglaspalmer douglaspalmer marked this pull request as ready for review June 18, 2025 04:00
rmartinc
rmartinc suggested changes Jun 18, 2025
View reviewed changes
Copy link
Contributor
@rmartinc rmartinc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you like the changes in the UI rmartinc@2204992 ?

@douglaspalmer douglaspalmer requested a review from a team as a code owner June 18, 2025 15:45
keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Jun 18, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

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.cluster.GroupInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

java.lang.RuntimeException: java.lang.IllegalStateException: Keycloak unexpectedly died :(
	at org.keycloak.testsuite.arquillian.containers.KeycloakQuarkusServerDeployableContainer.start(KeycloakQuarkusServerDeployableContainer.java:71)
	at org.jboss.arquillian.container.impl.ContainerImpl.start(ContainerImpl.java:185)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:137)
	at org.jboss.arquillian.container.impl.client.container.ContainerLifecycleController$8.perform(ContainerLifecycleController.java:133)
...

Report flaky test

rmartinc
rmartinc approved these changes Jun 20, 2025
View reviewed changes
Copy link
Contributor
@rmartinc rmartinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglaspalmer For me it's OK. But maybe adding a method to all the calls would more readable:

public static int checkNonNegativeNumber(int value, String name) {
    if(value < 0) {
        throw new ModelException(name + " may not be a negative value");
    }
    return value;
}

And just call it something line:

if (rep.getMaxTemporaryLockouts() != null) {
    newRealm.setMaxTemporaryLockouts(checkNonNegativeNumber(rep.getMaxTemporaryLockouts(), "Maximum temporary lockouts"));
}

Besides this way we execute the CI again to remove the failure. But the idea is OK, it's just an improvement.

@douglaspalmer douglaspalmer force-pushed the 40353 branch 2 times, most recently from b6262df to b94c968 Compare June 20, 2025 16:58
douglaspalmer and others added 2 commits June 20, 2025 15:08
@douglaspalmer
Issue with Handling Negative Values in Certain Fields of Brute Force …
c8a1660 
…Detection

Closes keycloak#40353

Signed-off-by: Douglas Palmer <dpalmer@redhat.com>
@rmartinc @douglaspalmer
Issue with Handling Negative Values in Certain Fields of Brute Force …
433f6ab 
…Detection

Closes keycloak#40353

Signed-off-by: rmartinc <rmartinc@redhat.com>
rmartinc
rmartinc approved these changes Jun 23, 2025
View reviewed changes
Copy link
Contributor
@rmartinc rmartinc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @douglaspalmer! LGTM!

ssilvert
ssilvert approved these changes Jun 23, 2025
View reviewed changes
Copy link
Contributor
@ssilvert ssilvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved the UI part.

@abstractj abstractj merged commit 434a4ef into keycloak:main Jun 24, 2025
76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@abstractj abstractj abstractj approved these changes

@edewit edewit edewit approved these changes

@ssilvert ssilvert ssilvert approved these changes

@rmartinc rmartinc rmartinc approved these changes

Assignees
No one assigned
Labels
flaky-test team/cloud-native team/core-clients team/core-iam team/ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with Handling Negative Values in Certain Fields of Brute Force Detection
5 participants
@douglaspalmer @abstractj @edewit @ssilvert @rmartinc
0