AngelAlvarado / multiple_idp_simplesamlphp

A refactored version of the simplesamlphp_auth module. This version accepts more than one idP and provides a seemly deep linking integration.
GNU General Public License v2.0
8 stars 3 forks source link

Additional attributes not assigned to users created via SSO #4

Open AngelAlvarado opened 8 years ago

AngelAlvarado commented 8 years ago

This is what I got in the error log:

Notice: Undefined offset: 1 in SimpleSAML_Drupal->load() (line 136 of includes/simplesamlphp_auth.drupal.inc). Notice: Undefined offset: 1 in SimpleSAML_Drupal->load() (line 136 of includes/simplesamlphp_auth.drupal.inc).

AngelAlvarado commented 8 years ago

Even the SAML request is providing correct attributes. The additional attributes for an IdP are not being mapped into Drupal fields

<saml:AttributeStatement>
            <saml:Attribute Name="email"
                            NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic"
                            >
                <saml:AttributeValue xsi:type="xs:string">admin2@molanco.com</saml:AttributeValue>
            </saml:Attribute>
            <saml:Attribute Name="roles"
                            NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic"
                            >
                <saml:AttributeValue xsi:type="xs:string">2</saml:AttributeValue>
                <saml:AttributeValue xsi:type="xs:string">4</saml:AttributeValue>
            </saml:Attribute>
            <saml:Attribute Name="first_name"
                            NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic"
                            >
                <saml:AttributeValue xsi:type="xs:string">Carlos</saml:AttributeValue>
            </saml:Attribute>
        </saml:AttributeStatement>
screen shot 2016-05-22 at 9 42 41 pm

Notice the User entity has a textfield called first_name.

pablo-tapia commented 8 years ago

The issue is with this part of the code:

 if ($key == 'cs_fields') {
      foreach ($data as $additional_data) {
           $additional_data = explode(':', $additional_data);
           $this->_simplesaml_auth_source->{$additional_data[0]} = $additional_data[1];
      } // end foreach
} // end if

When we save the data on the DB (I mean the custom fields) we serialize the data and save it, when we load the IDP we unserialize the data and we add it as a dynamic property on the SimpleSAML_Drupal object, in this case it seems the explode part is not creating the array the right way. Maybe because the properties are not been correctly defined or you're using a different operator.

Can you debug the code and pass me the values you're using for those custom fields?

AngelAlvarado commented 8 years ago

"Maybe because the properties are not been correctly defined or you're using a different operator." You are right I was using the incorrect operator. The value in the Additional Attributes box was 'fisrt_name|first_name'. Once I changed it to 'first_name:first_name' this error dissapear. I think a validation in that form is needed.

Although, even providing the correct operator the attributes are not being assigned to the users.

This what $attributes looks like (after fixing the operator):

Array ( [username] => email [unique_id] => email [mailattr] => email [cs_fields] => Array ( [0] => first_name:first_name ) )

AngelAlvarado commented 8 years ago

I think I got it,

From simplesamlphp_auth.inc. We could substitute this

/**
         * If you want, you can place more custom fields and populate them here
         * @example
         * if (isset($simplesamlphp_authsource->firstname)) {
         *   $userinfo['field_first_name'] = array(
         *     'und' => array(array('value' => $simplesamlphp_drupal->getAttrsFromAssertion($simplesamlphp_authsource->firstname)))
         *   );
         * }
         */

With this:

// Insert custom dynamic attributes per IdP.
if($simplesamlphp_authsource->cs_fields){
          foreach ($simplesamlphp_authsource->cs_fields as $cs_field) {
            $field_info = explode(':', $cs_field);
            if (isset($simplesamlphp_authsource->$field_info[1])) {
                 $userinfo['field_' . $field_info[0]] = array(
              'und' => array(array('value' => $simplesamlphp_drupal->getAttrsFromAssertion($simplesamlphp_authsource->$field_info[1])))
                 );
            }
          }
        }
pablo-tapia commented 8 years ago

Ok, I think I solved the validation issue and the new issue we found with the value of the custom fields not working correctly when the same operator is used inside the value of the custom field. Take a look at this code and let me know what you think.

if ($key == 'cs_fields') {
  foreach ($data as $additional_data) {
    $additional_data = explode(':', $additional_data, 2);
    // Always look for index number one, that's the value of the custom field
    if (isset($additional_data[1]) && !empty($additional_data[1])) {
       $this->_simplesaml_auth_source->{$additional_data[0]} = $additional_data[1];
    } // end if
  } // end foreach
} // end if
pablo-tapia commented 8 years ago

For the custom fields I think I have a good solution for it, instead of saving custom fields as properties of the Drupal SAML object I save them as a custom fields object array, like this:

if (isset($additional_data[1]) && !empty($additional_data[1])) {
     // Wrap all custom fields around the custom fields property to access them later
    $this->_simplesaml_auth_source->custom_fields->{$additional_data[0]} = $additional_data[1];
 } else {
    watchdog('simplesamlphp_auth', 'Unable to load custom field into the SAML object, verify that you\'re using the correct operator', array(), WATCHDOG_DEBUG);
 } // end if - else

And then we can access the properties like this:

if (isset($simplesamlphp_authsource->custom_fields)) {
          foreach($simplesamlphp_authsource->custom_fields as $custom_field_name) {
            if (!empty($simplesamlphp_authsource->custom_fields->{$custom_field_name})) {
              $userinfo[$custom_field_name] = array(
                'und' => array(array('value' => $simplesamlphp_drupal->getAttrsFromAssertion($simplesamlphp_authsource->custom_fields->{$custom_field_name})))
              );
            } // end if
          } // end foreach
        } // end if

Let me know what you think.

AngelAlvarado commented 8 years ago

This one makes totally sense, we already test it : )

Also using your solution for the attributes problem is way cleaner