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

CAS Username Mapper #49

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

daramos
Copy link
Contributor

@daramos daramos commented Mar 29, 2022

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!

@jacekkow
Copy link
Owner

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

@daramos
Copy link
Contributor Author

daramos commented Mar 30, 2022

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!

@Ekouyoja
Copy link

Ekouyoja commented Apr 5, 2022

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 ?

@jacekkow jacekkow self-assigned this Apr 22, 2022
@jacekkow
Copy link
Owner

  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
Copy link
Owner

jacekkow commented Oct 4, 2022

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

@jacekkow jacekkow assigned daramos and unassigned jacekkow Oct 4, 2022
@jacekkow jacekkow force-pushed the master branch 2 times, most recently from e0ae2a8 to bba8bfe Compare June 28, 2023 10:55
@albsol
Copy link

albsol commented Oct 3, 2023

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

XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Apr 4, 2024
XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Apr 4, 2024
XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Apr 4, 2024
XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Aug 17, 2024
XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Aug 17, 2024
XSmeets added a commit to smeetsee/keycloak-protocol-cas that referenced this pull request Aug 17, 2024
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.

4 participants