Skip to content
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.

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

KEYCLOAK-12732 Improve SAMLAttribute parsing of unknown attributes #6681

Conversation

thomasdarimont
Copy link
Contributor

@thomasdarimont thomasdarimont commented Jan 21, 2020

We now store all unknown attributes present on a SAMLAttribute element
in the "otherAttributes" map associated with the element.

Previously only the x500:encoding attribute was handled while parsing
attribute elements.

Note that I updated the allowed Java version in the maven pom from 1.7 to 1.8 since the rest of Keycloak also uses Java 8 "already".

@stianst
Copy link
Contributor

stianst commented Jan 30, 2020

Is the Java update needed? saml-core is used in adapters and I believe we still support 1.7 in some cases :/

@thomasdarimont
Copy link
Contributor Author

I updated the Java Version to be able to use the Stream API. Of course I could rework that again, but I thought the upgrade would be fine, since most of the other Keycloak modules on the server-side are also using Java 8 API.
https://lists.jboss.org/pipermail/keycloak-dev/2016-January/006221.html

@thomasdarimont thomasdarimont force-pushed the issue/KEYCLOAK-12732-Improve-Unknown-SAMLAttribute-Handling branch from 66e60c8 to 6bc31c1 Compare January 30, 2020 11:15
@thomasdarimont
Copy link
Contributor Author

I downgraded the PR to be compatible with Java 7 API.

@thomasdarimont thomasdarimont force-pushed the issue/KEYCLOAK-12732-Improve-Unknown-SAMLAttribute-Handling branch from 6bc31c1 to e1101ae Compare January 30, 2020 11:20
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.

Thanks for your PR. I believe once the attributes would use qualified names instead of local names, and fixing the static initialization (please see inline), this would be a do contribution.

We now store all unknown attributes present on a SAMLAttribute element
in the "otherAttributes" map associated with the element.

Previously only the x500:encoding attribute was handled while parsing
attribute elements.
@thomasdarimont thomasdarimont force-pushed the issue/KEYCLOAK-12732-Improve-Unknown-SAMLAttribute-Handling branch from e1101ae to abd5ef1 Compare January 30, 2020 17:51
@thomasdarimont
Copy link
Contributor Author

thomasdarimont commented Jan 30, 2020

@hmlnarik thanks for the review! I just applied the requested changes, except for the logging, please see comments for rationale.

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.

Looks good now, thanks @thomasdarimont !

@hmlnarik hmlnarik merged commit fc397e8 into keycloak:master Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants