Skip to content

10227: RelayState max length not respected (fix2)#13757

Merged
hmlnarik merged 3 commits intokeycloak:mainfrom
evtr:10227
Sep 6, 2022
Merged

10227: RelayState max length not respected (fix2)#13757
hmlnarik merged 3 commits intokeycloak:mainfrom
evtr:10227

Conversation

@evtr
Copy link
Contributor

@evtr evtr commented Aug 12, 2022

Closes #10227 RelayState max length not respected

@evtr
Copy link
Contributor Author

evtr commented Aug 13, 2022

Closes #10227 RelayState max length not respected

@evtr
Copy link
Contributor Author

evtr commented Aug 18, 2022

@ahus1: Can you approve running the workflows?

@ahus1 ahus1 self-assigned this Aug 19, 2022
@ahus1
Copy link
Member

ahus1 commented Aug 19, 2022

Hi @evtr, thanks for this PR.

Two things I note here:

  • Please put the Closes #10227 in the very first comment of the issue. Only then will it link the PR to the issue.
  • This project doesn't use Mockito. If you can't write the test without it, there needs to be an integration test for this new feature. One of those SAML integration tests is for example KcSamlXPathAttributeMapperTest.

Please let me know if you can think of a way to write such a test without Mockito, or if you need help writing such a SAML test case.

@evtr
Copy link
Contributor Author

evtr commented Aug 19, 2022

The objects that are mocked in the tests mocks very extensive interfaces. I can create TestClientModel and TestRealmModel classes but there are many methods to override. Is that ok?

@evtr
Copy link
Contributor Author

evtr commented Aug 22, 2022

@ahus1: I changed the unit tests to not using mockito

@evtr
Copy link
Contributor Author

evtr commented Aug 24, 2022

@ahus1: I don't really understand what is failing here? Can you help?

@ahus1
Copy link
Member

ahus1 commented Aug 24, 2022

Hi @evtr - failing Quickstart tests are unrelated to your changes. I'll figure out what needs to be done there.

I'll also reach out to some maintainers in a separate channel to find out whether to go with the manually created mocks, or with a SAML integration tests.

@evtr
Copy link
Contributor Author

evtr commented Aug 25, 2022

My thoughts one the SAML integration tests. Perhaps I shouldn't create new tests but choose one of the "sunshine" scenarios that are already in the test suite and add an assertion on the relaystate length somehow?

@ahus1
Copy link
Member

ahus1 commented Aug 25, 2022

@hmlnarik - this PR adds a class IdentityBrokerStateTestHelpers.java to avoid using a mocking framework like Mockito as I know that adding such dependencies is discouraged.

Could you please provide a hint if this is the way to go, or if the contributor should go for an Arquillian based integration test.

Thanks!

@hmlnarik hmlnarik added area/saml Indicates an issue on SAML area kind/bug Categorizes a PR related to a bug labels Sep 1, 2022
Copy link
Contributor

@hmlnarik hmlnarik left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@hmlnarik hmlnarik merged commit 4469bdc into keycloak:main Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/saml Indicates an issue on SAML area kind/bug Categorizes a PR related to a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RelayState max length not respected

3 participants