Access token saved to session only after events are triggered [#2838248]

Scenario :

I created the facebook application as explained in the module explanations. In a custom module, I add an EventSubcriber to the services.
I install and configure the module with the keys provided by the facebook application.
I implement a createdUser () method attached to the event 'simple_fb_connect.user_created', to change the user to his registration (adding civility, name and first name)
I have error logs in the createdUser () method
Clear caches to add the EventSubscriber to the Drupal registry
I go to the URL /user/simple-fb-connect for the first time, I accept to share facebook application datas, which redirects me back to my site

The following error appears:

Error validating access token: Session does not match current stored session. This may be because the user changed the password since the time the session was created or Facebook has changed the session for security reasons.

I also try to attach the Eventsubscriber on the login event, without success.

This error occurs only at the first exchange between the Facebook application and my site.

WAMP 2.5
Apache 2.4.9
PHP 5.6.20
Windows 10 64 bits

Here's the code of the EventSubscriber :


    static function getSubscribedEvents() {
        $events = array();
        $events['simple_fb_connect.user_created'][] = array('userCreated');
        return $events;
    }

    public function userCreated(GenericEvent $event) {

        $user = $event->getArgument('account');
        // Let's see how we can get make our own API calls to Facebook. We need
        // user's Facebook access token, which SimpleFbConnect has stored to session
        // for other modules.
        $access_token = $this->persistentDataHandler->get('access_token');

        if ($access_token) {
            try {
                $graph_node = $this->facebook->get('/me?fields=email,first_name,last_name,gender', $access_token)->getGraphNode()->asArray();

                $graph_node['gender'] === 'male' ? $user->set('field_user_civility', '1') : $user->set('field_user_civility', '2');
                $user->set('field_user_name', $graph_node['last_name']);
                $user->set('field_user_firstname', $graph_node['first_name']);
                $user->set('name', $graph_node['email']);

                $user->save();
            }
            catch (FacebookRequestException $ex) {
                // Add exception handling here for FacebookRequestExceptions.
                exit($ex->getMessage());
            }
            catch (FacebookSDKException $ex) {
                // Add exception handling here for all other exceptions.
                exit($ex->getMessage());
            }
        }
        else {
            drupal_set_message("No FB access token found for current user!");
        }
    }

Comment	File	Size	Author
#16	2838248-persist-access-token-before-events-16.patch	2.69 KB	masipila
#16
#12	2838248-12-persist-access-token-before-events.patch	6.96 KB	Kevin P Davison
#12
#10	2838248-persist-access-token-before-events.patch	7.74 KB	Gaius57
#10

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

21 December 2016 at 08:48

Gaius57 created an issue. See original summary.

Comment #2

masipila CreditAttribution: masipila commented 21 December 2016 at 09:36

Hi,

This is most probably regression caused by #2832806: Ensure that access token is saved to session only after succesful login.

Could you debug and try to see what is the value from this piece of your code when the error occurs (I assume it's empty but please confirm):
$access_token = $this->persistentDataHandler->get('access_token');

Markus

Comment #3

Gaius57 CreditAttribution: Gaius57 commented 21 December 2016 at 12:49

All right, I've re-test everything from the start and indeed, $access_token is null.

\modules\custom\idlsm_user\src\EventSubscriber\IDLSMUserSubscriber.php:68:null

After searching in your code, I've find that the following instruction in modules/contrib/simple_fb_connect/src/SimpleFbConnectUserManager.php line 160:

$this->persistentDataHandler->set('access_token', $access_token);

The datas are persist after createUser() or loginUser(), so the access_token is not available at the moment of the first EventSubcriber, is it right ?

However, thanks for this module, and for your help. I've understand many cool concepts of Drupal 8 with this module ;-).

Comment #4

Gaius57 CreditAttribution: Gaius57 commented 21 December 2016 at 13:05

I'm not sure, but shouldn't the token be persist before the events are triggered ?

Example, like this :

// Try to save the new user account.
    try {
      $new_user->save();

      $this->loggerFactory
        ->get('simple_fb_connect')
        ->notice('New user created. Username @username, UID: @uid', array('@username' => $new_user->getAccountName(), '@uid' => $new_user->id()));

      // HERE DO THE ACCESS_TOKEN PERSISTENCE

      // Dispatch an event so that other modules can react to the user creation.
      // Set the account twice on the event: as the main subject but also in the
      // list of arguments.
      $event = new GenericEvent($new_user, ['account' => $new_user, 'fbid' => $fbid]);
      $this->eventDispatcher->dispatch('simple_fb_connect.user_created', $event);

      return $new_user;
    }

Comment #5

masipila CreditAttribution: masipila commented 21 December 2016 at 14:52

Title:	"Error validating access token" when create/login user on EventSubscriber	» Access token saved to session only after events are triggered
Category:	Support request	» Bug report

Yes, you are right. The access token must be saved before the events are triggered. This bug is regression from the other issue I linked above.

The access token is now saved in Controller only after the user has been created / only after the user has been logged in. But the events are triggered from SimpleFbConnectUserManager before that and that is the problem just like you mentioned.

What needs to be done is that
1) we need to inject the persistent data handler service to SimpleFbConnectUserManager
2) Then we can move the responsibility of access token saving from Controller to SimpleFbConnectUserManager.

Nice to hear that this module and the module handbook examples have been useful to you for learning D8 concepts! Do you want to test your learning and create a patch for this issue? I can also do it but in case you'd like to do it, I'm open for it :)

Cheers,
Markus

Comment #6

Gaius57 CreditAttribution: Gaius57 commented 21 December 2016 at 15:47

I'll try it tonight. I've never created a patch for a contrib module.

I keep you in touch ;-)

Comment #7

masipila CreditAttribution: masipila commented 21 December 2016 at 16:18

Hi!

Here's a guide on creating patches: https://www.drupal.org/node/707484

Cheers,
Markus

Comment #8

masipila CreditAttribution: masipila commented 21 December 2016 at 16:26

And a shorter version:
https://www.drupal.org/node/2150283/git-instructions/8.x-3.x/nonmaintainer

Comment #9

Kevin P Davison CreditAttribution: Kevin P Davison at Hook 42 for Go Overseas commented 21 December 2016 at 22:01

I'm following this issue closely, and please let me know if you could use help testing for peer review.
I have a test fb app and my own EventSubscriber ready to go, with the same issue.
I don't see how to solve for #5 myself, and I'm hoping @Gaius57 is now making progress. Good luck!

Comment #10

Gaius57 CreditAttribution: Gaius57 commented 21 December 2016 at 23:31

File	Size
2838248-persist-access-token-before-events.patch	7.74 KB

Here's the patch I've made for this fix. I passed the Facebook access token as argument (default = NULL) of createUser() and loginUser() in order to persist it inside this functions before just events are fired.

Comment #11

Gaius57 CreditAttribution: Gaius57 commented 21 December 2016 at 23:35

Status:

Active

» Needs review

Comment #12

Kevin P Davison CreditAttribution: Kevin P Davison at Hook 42 for Go Overseas commented 22 December 2016 at 00:43

File	Size
2838248-12-persist-access-token-before-events.patch	6.96 KB

@Gaius57 the ONLY thing I changed was some code formatting (a few spaces to help w/ testing), and I'd recommend checking out https://www.drupal.org/docs/develop/standards/coding-standards. Depending on the IDE you might be using, PHPStorm for example has plugins and settings to help too. I'd be happy to share more directly. AND this was perfect. Thank you, and welcome to Drupal!

Comment #13

masipila CreditAttribution: masipila commented 22 December 2016 at 06:15

Hi!

Thanks Gaius57 & Kevin for your effort! I reviewed the patch with my mobile (i.e. didn't test it yet) and figured out that we still have one corner case that is not covered.

When we don't have an existing Drupal user, we create a new one. However, it might well be that the Drupal account settings are defined so that the new accounts must be reviewed by site admin. In this case 1) the new user is created but 2) it remains blocked and 3) the user remains as anonymous / not-logged-in user. In this scenario we show a message to the user that the account is pending admin review.

We have to pass the access token to the event in this case (this patch fixes this) but we must erase the access token from the session after we identify that the user could not be logged in. Otherwise the access token would persist in the session that has been started for the anonymous user and that is not ok.

So basically these scenarios in the Controller need to be handled:

      else {
        // New user was created but the account is pending approval.
        drupal_set_message(t('You will receive an email when site administrator activates your account.'), 'warning');
        return $this->redirect('user.login');
      }
    }

    else {
      // User could not be created.
      return $this->redirect('user.login');
    }

We also need to check if the unit tests need updates. The automatic unit tests are not yet enabled for this module at d.o. but they can be run locally.

Cheers,
Markus

Comment #14

masipila CreditAttribution: masipila commented 22 December 2016 at 06:15

Status:

Needs review

» Needs work

Comment #15

masipila CreditAttribution: masipila commented 22 December 2016 at 19:25

Assigned:

Unassigned

» masipila

Just to let you know that I'm working on this right now to avoid double work.

Comment #16

masipila CreditAttribution: masipila commented 22 December 2016 at 19:53

Assigned:	masipila	» Unassigned
Status:	Needs work	» Needs review

File	Size
2838248-persist-access-token-before-events-16.patch	2.69 KB

Hi,

I had now a closer look at this (the previous comments were written from my mobile on the way to work).

As mentioned in #13, we must make sure that we are not storing the access token in the session in case the user could not be logged in. This means that we need to anyway have the unsetting logic in the controller. This heavily suggests that the setting logic should also be in the controller because otherwise the access token setting is done in SimpleFbConnectUserManager and the unsetting is done in the controller, which makes the code very hard to follow.

@Gaius57 and @Kevin Davison, could you please test the attached patch and confirm that it works properly?

@Gaius57, I apologize for leading you to the wrong direction in comment #5. I hope that you still consider the effort for writing your first contrib patch worth it and I will still credit you for this commit.

Cheers,
Markus

Comment #17

Kevin P Davison CreditAttribution: Kevin P Davison at Hook 42 for Go Overseas commented 22 December 2016 at 21:05

Testing now, thanks! Expect my response shortly...

Comment #18

Kevin P Davison CreditAttribution: Kevin P Davison at Hook 42 for Go Overseas commented 22 December 2016 at 22:50

@masipila I've tested patch #16 successfully:

Logged OUT of Facebook and a new Drupal user - Drupal account creation and FB Login success!
Logged IN to Facebook, logged OUT of Drupal, and an active Drupal user - Drupal FB Login success!
Logged OUT of Facebook, logged OUT of Drupal, and an active Drupal user - Drupal FB Login success!
Logged IN to Facebook, logged OUT of Drupal, and a BLOCKED Drupal user - Drupal FB Login FAIL, but success! ("You could not be logged in because your user account Sharon Aladgafagccbc Hook is not active.")

Really happy with this collaborative work, and thank you @masipila and @Gaius57! Looking forward to RTBC and RC3.

Comment #19

masipila CreditAttribution: masipila commented 23 December 2016 at 06:42

@Kevin, did you also test that the triggered events were OK?

Comment #20

masipila CreditAttribution: masipila commented 23 December 2016 at 10:30

I created a test module to validate that the subscribers can have the access token but that the access token is ereased if login was not successful.

1) Creation of new Drupal user when accounts require admin approval
- event subscriber was able to make API calls: SUCCESS
- access token was not found in the session variables because the user was not logged in: SUCCESS

2) Login attempt with a blocked user account
- event subscriber not triggered because the user was not logged in: SUCCESS
- access token was not found in the session variables because the user was not logged in: SUCCESS

3) Creation of new Drupal user when accounts can be created without admin approval
- event subscriber for user creation was able to make API calls: SUCCESS
- event subscriber for user login was able to make API calls: SUCCESS
- access token was found in the session variables because the user was logged in: SUCCESS

4) Login with an active user account
- event subscriber for user login was able to make API calls: SUCCESS
- access token was found in the session variables because the user was logged in: SUCCESS

Comment #21

23 December 2016 at 10:34

masipila committed 312f7c7 on 8.x-3.x authored by Gaius57

Issue #2838248 by Gaius57, Kevin Davison, masipila: Access token saved...

Comment #22

masipila CreditAttribution: masipila commented 23 December 2016 at 10:37

Version:	8.x-3.0-rc2	» 8.x-3.x-dev
Status:	Needs review	» Fixed

This is now fixed in the 8.x-3.x-dev branch. I will wait for a couple of weeks before cutting the next release candidate so that we can include all other possible fixes / changes to RC3 which will hopefully be the last release candidate before a stable release.

Thanks once more Gaius57 and welcome to the Drupal community! And cheers Kevin for your help with this!

Markus

Comment #23

Gaius57 CreditAttribution: Gaius57 commented 23 December 2016 at 10:56

Sorry to be late, I was on another subject last day.

@masipila, there's absolutely no problem for giving me the "wrong" direction, I'm glad to participate to the fixes, however my work is retained or not. So, thanks for the credit ;-).

I will test #16 patch on my side to and give you a feedback ASAP.

@Kevin, for the code formatting, I'm using PHPStorm. Maybe some of you are using it to, and can send me an export of his settings ? It'll be a much simpler for me for having the same formatting as the community :-).

Many thanks both of you !

edit : I see it's fixed now, great work !

Comment #24

masipila CreditAttribution: masipila commented 23 December 2016 at 10:59

@Gaius57: Quick googling on PHPStorm and Drupal resulted to several hits, this one seems quite good with a quick glance: https://www.drupal.org/docs/develop/development-tools/configuring-phpstorm