User account registration works only, if user image is not required

Setting to needs review to trigger the testbot.

Hi @Peter Majmesku,

Thanks for reporting this. Could you please re-submit your patch according to Drupal patch standards so that it would apply correctly? And at the same time, please ensure that your patch is against the latest DEV as the dev is several commits ahead of 8.x-3.0 (this issue is marked as 8.x-3.0 so I assume your patch is against 8.x-3.0 and not 8.x-3.x-dev)

Cheers,
Markus

I was checking this earlier today before your latest patch updates. We have here a conceptual challenge.

First of all, we do not download the Facebook default silhouette picture if the user has not set a profile pic on Facebook. In other words, there is no file to be added as the Drupal user's profile pic.

Second, the API call to get the user's profile pic can fail in a rare corner case even if the user's profile could be read. Also in this case we would not have any file to be added.

The second one is quite theoretical corner case but the first one is by design. So setting the user's profile pic as required is quite difficult...

Markus

I did understand that your site requires a profile picture and I see the rationale why some sites are requiring it.

However, the second scenario can be caused by a network issue for example or Facebook API to be unavailable at the time of the second API call where we try to get the URL for the profile pic. What would be your suggeetion to handle this error situation or should we just not create the Drupal user record if this happens?

What would you suggest to happen if the user has not added a profile pic on Facebook? Should we not create a Drupal user and show a message for the user to add a pic on Facebook first?

Cheers,
Markus

If the user has just no facebook profile image, then there should be a specific error message: "We can only accept user registrations with a user profile image. Please upload a profile image at facebook or use our registration form."

This message that complains about the missing profile pic needs to be shown only when the Drupal user pic is required. Sites that don't have the user pictures defined as required should accept the registration without the picture.

Are you able to update your patch with this check applied? This same handling covers also the rare corner case where profile pic url query fails.

Markus

To continue: I'd assume that we can figure out if the field is required using EntityFieldManager the same way how we check the directory for user pictures in SimpleFbConnectUserManager::getPictureDirectory

Hi again,

Just be aware that if you use your patch as it is

• Your user creation will fail if the user does not have a picture on Facebook
• The file will not have the owner set up correctly

It is of course up to you to judge if these are acceptable on your site and use case.

If somebody wishes to continue with this issue, we need to properly handle a couple of scenarios so that this could be committed.

Pre-requisite: we need to add a new method to SimpleFbConnectUserManager that checks if user pictures are required.

  protected function userPictureRequired() {
    $field_definitions = $this->entityFieldManager->getFieldDefinitions('user', 'user');
    if (isset($field_definitions['user_picture'])) {
      return $field_definitions['user_picture']->get('required');
    }
    // If user_picture field is not defined, it is not required.
    return FALSE;
  }

Scenarios 1: User pictures are enabled but not required (Drupal default behavior)

Scenario 1.1:

• User pictures are enabled but not required (Drupal default behavior)
• API call for getting the Facebook profile picture fails OR the profile picture download fails

Expected behavior for this scenario:

• Log an error that the API call to read the profile picture failed / file download failed
• Drupal user's profile picture field is left empty (OK, because the field is not required)

Scenario 1.2:

• User pictures are enabled but not required (Drupal default behavior)
• API call for getting the Facebook profile picture is successful
• GraphNode representing the Facebook profile picture indicates that the profile picture is the default Facebook silhouette

Expected behavior for this scenario:

• Drupal user's profile picture field is left empty so that we will respect the possible Empty Image setting that the site builder has defined for the user picture field

Scenario 1.3 (happy flow):

• User pictures are enabled but not required (Drupal default behavior)
• API call for getting the Facebook profile picture is successful
• GraphNode representing the Facebook profile picture indicates that the profile picture is NOT the default Facebook silhouette
• File could be downloaded successfully

Expected behavior for this scenario:

• Facebook profile picture is used as Drupal user profile picture.
• The file is set as Drupal user's profile picture before we dispatch the event to other modules
• The owner of the file is the newly created Drupal user.

Scenarios 2: User pictures are enabled and required (configuration where this issue started from)

Scenario 2.1:

• User pictures are enabled and required
• API call for getting the Facebook profile picture fails OR the profile picture download fails

Expected behavior for this scenario:

• Log an error that the API call to read the profile picture failed / file download failed
• Show a message to the user that new Drupal user account could not be created
• Drupal user account is not created.

Scenario 2.2:

• User pictures are enabled and required
• API call for getting the Facebook profile is successful and picture was downloaded successfully

Expected behavior for this scenario:

• Facebook profile picture is used as Drupal user profile picture even if it was the default Facebook silhouette.
• The file is set as Drupal user's profile picture before we dispatch the event to other modules
• The owner of the file is the newly created Drupal user.

Scenario 3: Drupal user profile does not have the standard user picture field

• Do not try to insert the profile picture to a field which does not exist

About the file ownership: I was referring to which Drupal user owns the file. I believe how you implemented the patch removes the part where the file ownership is set.

And what it comes to the other topics I listed: it's all about handling the errors properly and logging them so pretty basic stuff.

When the user_picture is NOT required, then all error situations should be handled correctly already. But when the the user picture is required and we don't have it because one of the three cases (api call failed / download failed / user didn't have a pic on Facebook), then we need to improve the error handling here.

I understand if you don't have time for this and respect that. My point was more that if someone else wants to contribute on fixing this.

Markus

Hi Peter,

would you have time to test if the attached patch works in the scenarios that you have had?

This patch now tries to cover the logic I was writing earlier. You were right that the logic was too complex so I simplified it a bit. Proper error handling is also included.

• If profile pictures are enabled and required, we accept the default silhouette from Facebook.
• If profile pictures are enabled but NOT required and the user does not have a Facebook profile picture, we do NOT download the default silhouette from Facebook. Instead we leave the Drupal user_picture field empty.
• If profile pictures are not enabled, we don't download the picture from Facebook.

I'd really appreciate if you could test this a bit under the different scenarios before I commit this. Other members of the community are of course also more than welcome to help with testing this.

Cheers,
Markus

Hi,

I have tested the patch and could register via facebook with a profile image and without a profile image. Interesting was, that there is a default profile image, which comes from facebook.

This is by design of this new patch. When we make the API query to get the Facebook profile picture, we get as a response a GraphNode object which has the URL of the image. We also can check from this GraphNode object if the image that is indicated is the Facebook default silhouette.

As mentioned earlier:

• If the Drupal user_picture is required, we download this silhouette so that the user creation will not fail due to missing picture.
• If the Drupal user_picture is NOT required, then we ignore the Facebook picture if it is a silhouette. This allows the site builder to use Drupal's Default Image behavior.

  Cheers,
  Markus

Thanks also for the code review, I appreciate it. A couple of comments:

You are commenting too much.

This is probably true for some occasions. My coding style is quite verbose when it comes to documenting. The reason for this is that I want to remember the logic why I have written the logic like that when I need to get back to it.

General thing: the controller src/Controller/SimpleFbConnectController.php is doing too much. The methods are very long and scattered with if-cases and the logic is very complex. You have quite no chance to completely overview that. Properly made, there should go more into a tested service.

We have two route callbacks in the controller. One that redirects the user to Facebook for authentication and the second which responds to the route where Facebook returms the user after the user has authenticated on Facebook.

The redirect to Facebook is not complex at all in my opinion but I completely agree the method handling the return from Facebook is complex because many things can go wrong: the user may not have given the permission to his/her email address, the user may not have email address on Facebook (yes, that is possible if you've registered with phone number only) and so on. I've tried to describe with the inline comments the flow whag is going on.

It might be feasible to have this whole flow moved from the controller to a service. I'll have to think about it at some point.

I could also run the unit tests manually. However the facebook sdk is a dependency. Only having the uninstalled module in the module folder, does not work. So there is a bit too much of dependency.

There's no other way than to have the SDK as a dependency unless we would write and maintain the heavy part of this whole functionality by ourselves. That would not be feasible at all and there would for sure be some security aspects that we would overlook if we would even try.

When it comes to unit testing, it is well possible to mock the Facebook SDK classes so that we could unit test our own code. The coverage is somewhat ok for the other classes than our SimpleFbConnectFbManager (there is an open issue for this and patches are more than welcome).

Cheers,
Markus

Committed to 8.x-3.x-dev. Thanks for your help with this!

Cheers,
Markus