keycloak-protocol-cas icon indicating copy to clipboard operation
keycloak-protocol-cas copied to clipboard

CAS Username Mapper

Open daramos opened this issue 2 years ago • 6 comments

This Merge request adds a new protocol mapper to allow the username that the CAS protocol returns to the service be derived from one of the user's attributes.

This is useful for services that for example expect a user's email to be returned by CAS instead of the username. At my organization we have several vendor products that requires CAS to provide a GUID attribute instead of a username.

This functionality is supported by the Apereo CAS server and the Shibboleth server's CAS module implementations of the CAS protocol.

Implementing this functionality will allow a migration path from those severs.

The mapper has an option to default to the username if the required attribute is missing.

I know this is a rather large change but I am more than happy to accommodate any changes you'd like to see.

Thanks Again!

daramos avatar Mar 29 '22 21:03 daramos

I'll try to review it by the end of the week.

jacekkow avatar Mar 29 '22 21:03 jacekkow

Thank you. One thing I forgot to mention in the merge description is that I tried to follow the same basic structure of the SAML Name ID Mapper and related interface which does something similar for SAML.

I think the current naming can be confusing as it's "username" and "mappedUsername" everywhere. The Apereo CAS server calls this identifier the "Principal ID" but I wasn't sure if that would be more or less ambiguous. I'm open to any changes.

One other issue was - what happens if more than Username mapper is defined for a specific client? There was no good answer that I could see. Ultimately I opted to just use the first one found, but maybe that was not the best solution?

I'm open to any changes you suggest.

Thanks Again!

daramos avatar Mar 30 '22 14:03 daramos

What an amazing work. I needed this feature so much, i have use brokering to return the proper attribute that i want to. But technically it's not sain. I hope it will be merged soon. Thanks to you and the owner of this repo. @jacekkow do you think you will merge it this week ?

Ekouyoja avatar Apr 05 '22 06:04 Ekouyoja

  1. In CAS response the field is named user (cas:user, when including the namespace from docs) - could you consistently use CasUser instead of CasUsername? Same goes for category name (CAS User Mapper) and so on.
  2. Could you provide rationale for extracting AbstractUserAttributeMapper? Why not simply override it in subclass? Do you intend to add other CasUserMappers? If so, create AbstractCasUserMapper and use it.
  3. Class is named UserAttributeCasUsernameMapper yet this UserAttribute(...)Mapper does not derive from AbstractUserAttributeMapper. I would argue then it must not be UserAttribute(...)Mapper! Maybe just CasUserMapper?
  4. Setting CONF_FALLBACK_TO_USERNAME_IF_NULL to false is useless - Keycloak would return empty username while it should error out. I would go with something in the realm of "fail if null". It gives far more control over mapping.
  5. Regarding multiple mappers I see a better solution: find first non-null mapped value and use it. If none was found, then fallback to "normal" username.

jacekkow avatar Apr 22 '22 20:04 jacekkow

@daramos - would you follow-up on this pull request?

jacekkow avatar Oct 04 '22 20:10 jacekkow

Hi, this is a really needed feature, we are waiting for its release, Thank you for all your efforts.

albsol avatar Oct 03 '23 13:10 albsol