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-14891 Add a fluent builder for AuthnRequest/RequestedAuthnContext element #7292

Closed
wants to merge 3 commits into from

Conversation

lscorcia
Copy link
Contributor

Adds a builder class for the RequestedAuthnContext SAML element and the required connector method in the AuthnRequest builder.

This is useful for custom SAML-derived Identity Provider modules, but I will also try later to create the required configuration UI in order to expose the feature to the built-in SAML Identity Provider.

@lscorcia
Copy link
Contributor Author

I am almost done with the configuration UI, except that I can't figure out how to pass a multivalued attribute to the server:

immagine

The IdP configuration stores plain strings only, so for storage I am concatenating the values, separating them with commas.
In the UI I am using an array as the fields' model, but when I press the Save button I get an exception:

2020-07-25 17:37:02,643 ERROR [org.keycloak.services.error.KeycloakErrorHandler] (default task-22) Uncaught server error: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize instance of `java.lang.String` out of START_ARRAY token
 at [Source: (io.undertow.servlet.spec.ServletInputStreamImpl); line: 1, column: 4444] (through reference chain: org.keycloak.representations.idm.IdentityProviderRepresentation["config"]->java.util.LinkedHashMap["authnContextClassRefs"])
	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1445)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnexpectedToken(DeserializationContext.java:1219)
	at com.fasterxml.jackson.databind.deser.std.StdDeserializer._deserializeFromArray(StdDeserializer.java:714)
	at com.fasterxml.jackson.databind.deser.std.StringDeserializer.deserialize(StringDeserializer.java:40)
	at com.fasterxml.jackson.databind.deser.std.StringDeserializer.deserialize(StringDeserializer.java:10)
	at com.fasterxml.jackson.databind.deser.std.MapDeserializer._readAndBindStringKeyMap(MapDeserializer.java:527)
	at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:364)
	at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:29)
	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:288)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:151)
	at com.fasterxml.jackson.databind.ObjectReader._bind(ObjectReader.java:1682)
	at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:977)
	at org.jboss.resteasy.plugins.providers.jackson.ResteasyJackson2Provider.readFrom(ResteasyJackson2Provider.java:194)
	at org.jboss.resteasy.core.interception.AbstractReaderInterceptorContext.readFrom(AbstractReaderInterceptorContext.java:66)

I guess the Identity Providers config UI only allows single-valued attributes? I sure could concat the values on the client, but this looks inconsistent with the other places in the UI that use multi-valued attributes (i.e. Clients > Valid Redirect URIs).
Is there anything I missed?

@lscorcia lscorcia marked this pull request as draft July 26, 2020 02:32
@lscorcia
Copy link
Contributor Author

I had a bit of extra spare time and I have been able to prepare the UI for setting the RequestedAuthnContext parameters in the SAML Identity Provider configuration.

Before I go on and write some unit tests and update the user docs, I'd appreciate a quick review of the general approach:

  • As far as I could see, the Identity Provider configuration does not support multi-valued parameters. Is the workaround of concatenating values on the client the correct solution? It seems a bit clumsy...
  • Should I keep the additional configuration values in the same "SAML Config" UI section, or is it better to group them in a dedicated section like I did?
  • Right now the code generates the RequestedAuthnContext element if either ClassRefs or DeclRefs are set. Would it be more intuitive to add a boolean flag that hides/shows the appropriate section?
  • I am not a native English speaker, so any improvement to the translation strings would be welcome;
  • There is a bit of confusion between the original pull request/JIRA issue intent and the actual feature as implemented. I would be perfectly fine with closing it and opening a new one with a better description if necessary.

Thanks for the help!

@lscorcia lscorcia marked this pull request as ready for review July 26, 2020 15:19
@pedroigor pedroigor added area/saml Indicates an issue on SAML area kind/enhancement Categorizes a PR related to an enhancement missing/tests Tests are missing area/ui impact/low labels Jul 28, 2020
Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

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

I see this one as a good improvement to SAML brokering.

You will be asked to send a message to keycloak-dev too so that you may get feedback from others from the community.

Regarding using a comma to store values, or that or use a JSON. At then end, we store key/value into the database ...

Changes LGTM.

@lscorcia
Copy link
Contributor Author

I tried to change the code to use JSON serialization and I think it's actually better this way, as it allows for commas in URIs (not that I plan to ever use them!) but also because it makes it more generic and future-proof, just in case it's ever needed to add other attributes to the entries. I'll start writing the tests and I'll probably close this issue/PR and open a new one for a clean feature description and review. Suggestion for the questions above are still welcome!
Thanks for the help!

@lscorcia
Copy link
Contributor Author

I'm closing this PR as a better implementation, including tests, is ready at PR #7307 .

@lscorcia lscorcia closed this Jul 30, 2020
@lscorcia lscorcia deleted the KEYCLOAK-14891 branch September 16, 2020 15:43
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 impact/low kind/enhancement Categorizes a PR related to an enhancement missing/tests Tests are missing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants