apokalipto / devise_saml_authenticatable

Devise SAML 2.0 authentication strategy
MIT License
292 stars 153 forks source link

Handling a SAML response with a mix of single/multiple attribute values #61

Open leondmello opened 7 years ago

leondmello commented 7 years ago

The ruby-saml gem exposes the single_value_compatibility setting which dictates whether only the first value for an attribute is returned.

When we enable it to return all values of a saml attribute, the code here then just tries to insert it into the user model as an array.

So, if you have a mix of single value and multiple value fields in the model, this approach doesn't work.

Maybe the user can be allowed to supply his own version of set_user_saml_attributes to dictate how the SAML variable values are stored.

adamstegman commented 7 years ago

Yeah, that could work, though we might need a different method name to check for in case Devise blows away the developer's implementation of set_user_saml_attributes. It would probably help if we throw the subject in too, something like

           if Devise.saml_update_user || (resource.new_record? && Devise.saml_create_user)
-             set_user_saml_attributes(resource, attributes)
+             all_attributes = attributes.dup
             if (Devise.saml_use_subject)
-              resource.send "#{key}=", auth_value
+              all_attributes[key] = auth_value
             end
+            if resource.respond_to?(:saml_attributes=)
+              resource.saml_attributes = all_attributes
+            else
+              set_user_saml_attributes(resource, all_attributes)
+            end
             resource.save!
           end
SxDx commented 4 years ago

I opened a pull request regarding this feature: https://github.com/apokalipto/devise_saml_authenticatable/pull/159

doconnor-clintel commented 7 months ago

In https://github.com/apokalipto/devise_saml_authenticatable/pull/245 I did a version of SxDx's work with test coverage, but it doesn't feel right as users are mutating global state.

I think the better answer to this is you turn off single_value_compatibility.