devise_saml_authenticatable icon indicating copy to clipboard operation
devise_saml_authenticatable copied to clipboard

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

Open leondmello opened this issue 8 years ago • 3 comments

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.

leondmello avatar Sep 06 '16 16:09 leondmello

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

adamstegman avatar Sep 12 '16 18:09 adamstegman

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

SxDx avatar Feb 12 '20 08:02 SxDx

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.

doconnor-clintel avatar Dec 06 '23 08:12 doconnor-clintel