Content
Added by Oliver Günther almost 9 years ago
For the deployment of the OmniAuth CAS Provider for our use case, I had to patch the core Concerns::OmniauthLogin to override the user attributes build process,
as the login name corresponds to the user’s mail adress, instead of the actual username.
The attributes retrieved from Concerns::OmniauthLogin.omniauth_hash_to_user_attributes are, as I see it, hardcoded:
def fill_user_fields_from_omniauth(user, auth)
user.update_attributes omniauth_hash_to_user_attributes(auth)
user.register
user
end
def omniauth_hash_to_user_attributes(auth)
info = auth[:info]
{
login: info[:email],
mail: info[:email],
firstname: info[:first_name] || info[:name],
lastname: info[:last_name],
identity_url: identity_url_from_omniauth(auth)
}
end
The patch of omniauth_hash_to_user_attributes is documented at Github
—
Apart from the aforementioned method chaining, is there another way (that I’ve missed) to override the user hash build process from omniauth attributes?
If not, I’d like to discuss on how (or rather, where) this could be integrated. I propose the option to add a callback/observer similar to OmniAuth::Authorization, just like the authorize_user and after_login callbacks provided there. Is this something the team would integrate to the core if I provide the implementation?
Best,
Oliver
Replies (9)
Hey Oliver,
you haven’t missed anything. There’s no other way to do it right now.
Yes, it would be good to provide one and we would gladly accept any contribution you make.
I’m not sure if
OmniAuth::Authorizationis the right place, though.I mean, conceptually, this is not a problem specifically with regard to authorization, but in general how to map user data.
So we might want to think about alternatives here.
Best,
Markus
Thanks for the clarification, Markus! :) I didn’t quite mean to put it into
Authorization, I was missing a ‘similar’ in my previous post.. oops.Maybe there’s a way to include this with the Provider registration (i.e. In the Auth Plugins) so to tie Provider definitition and attribute mapping to one location.
Best,
Oliver
Doing it in the provider registration is a good idea!
I’ve created a work package on openproject.org and two Pull Requests (openproject-auth_plugins, openproject) that implement this feature.
If there is a prettier way than a block to allow custom mappings (even with
:extraand:raw_infoin the AuthHash), let me know ;)Good job! I haven’t had a time to give it a thorough look yet, but what I saw looked great.
Just tests are missing. Would you mind writing some?
I’ve added specs to both pull requests. Do you have an officially defined code measure for spec coverage? If not, just drop me a note If you see something missing in the specs.
I’ve looked at the PRs and they look good. I will merge them as soon as I’m allowed to (currently the plugin dev branch is frozen, but tomorrow I should be able to do it).
Also: We have no officially defined coverage requirements that I know of. The specs look good either way.
Thanks for your contribution!
As we all agree that Oliver’s change is good, I have merged the pull requests. Thanks!