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

Additional attributes not assigned to users created via SSO #4

Open
AngelAlvarado opened this issue May 23, 2016 · 7 comments
Open
Assignees

Comments

@AngelAlvarado
Copy link
Owner

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

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">[email protected]</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
Copy link
Collaborator

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

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

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

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

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

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

Also using your solution for the attributes problem is way cleaner

joas8211 added a commit to joas8211/multiple_idp_simplesamlphp that referenced this issue Oct 6, 2017
It makes perfect sense that fields from `Additional attributes`
are populated automatically, instead of user having to hardcode
how the fields are populated in source code of this module.

This implementation has no support for translations or more
complex fields. But it should work fine with basic fields that
have simple value column.

The implementation is inspired by comment by @pablo-tapia in
AngelAlvarado#4.
joas8211 added a commit to joas8211/multiple_idp_simplesamlphp that referenced this issue Oct 6, 2017
It makes perfect sense that fields from `Additional attributes`
are populated automatically, instead of user having to hardcode
how the fields are populated in source code of this module.

This implementation has no support for translations or more
complex fields. But it should work fine with basic fields that
have simple value column.

The implementation is inspired by comment by @pablo-tapia in
AngelAlvarado#4.
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

No branches or pull requests

2 participants