SettingsController::actionGdprDelete() throws 403 for socialnetworked Users

[edegaudenzi]

What steps will reproduce the problem?

1. Log in and connect the same user with multiple SocialNetworks, I'm using o365 and a custom one, but it should affect all of them.
2. Then go to My Account -> Privacy -> Delete My Account -> Delete
3. It'll ask for a password; the password is the Yii2 one, which is random(?) if you created the user through a social network. You'll then need to 'forgot password' for your user and once you've finished, start again from step 1
4. Type the password and press 'Delete', a big 403 appears (and the user is not GDPR deleted)

This screenshot should visually show what's above

What is the expected result?

User gets GDPR deleted

What do you get instead?

403 and User doesn't get GDPR deleted

[edegaudenzi]

I can't reproduce for 'normal' users and I don't get why; reading at the code there might be two erros in SettingsController::actionGdprDelete() but I would be happy to be clarified:

1. Yii::$app->user->logout(); should be called after the user gets GDPR deleted, I don't get why it is before. Also, calling it after, fixes this issue with the socialnetworked users, but then I don't get how it worked for normal users so far.

2. SettingsController::disconnectSocialNetwork($id) exploits the property $this->socialNetworkAccountQuery and so far it's good. Problem is when a User had multiple connected Social Network. Looping across them aiming to delete them, de-facto we keep adding ->whereId($id) to $this->socialNetworkAccountQuery, resulting in the first successful social network removal in the first iteration, but a 404 in the second iteration.

We may need to replace the find() function, from:
$account = $this->socialNetworkAccountQuery->whereId($id)->one();
to
$account = SocialNetworkAccount::find()->whereId($id)->one();

[TonisOrmisson]

I would not replace $this->socialNetworkAccountQuery, using DI instead of instantiating it within the class seems like a good idea.

Maybe pass the whole Account object to disconnectSocialNetwork(SocialNetworkAccount $account) instead of passing just the id disconnectSocialNetwork($id) ?

[edegaudenzi]

Yes, it is a good idea indeed, problem here is that if you use $this->socialNetworkAccountQuery and you call ->whereId($id) a first time, then in yii2 there is no "removeWhere()" helper (maintainers expressed themselves on the topic at least once, here) and as a result $this->socialNetworkAccountQuery will keep that specific where() condition.

When SettingsController::actionGdprDelete() loops over ALL of the socialnetworks, then it'd need a new instance of \Da\User\Query\SocialNetworkAccountQuery at every cycle, so function whereId($id) can cleanly perform its return $this->andWhere(['id' => $id]);

At this point, you can either choose to create a new instance with the di or directly using the ar class and its helpers, it's an implementation choice.

About passing the whole object, I was also thinking the same, but that disconnectSocialNetwork($id) is also used by the action settings/disconnect?id=xxx, meaning we now need to perform the same find() in two different places, which is not ideal.

Personally, if I'd need to choose, I would still go for my first comment and I'd also go for ->logout() in the end and not in the beginning.

[edegaudenzi]

More than a year later, I found an elegant solution to this. Long story short, inspired by what standard yii\db\QueryTrait::andWhere() already does, Da\User\Query\SocialNetworkAccountQuery::whereId($id) should remove any pre-existing ['id' => ..] conditions, if any.

In this way, not only this issue is fixed, but it would also respect the semantic meaning of SocialNetworkAccountQuery::whereId($id) that will be searching for the actual passed $id every time someone calls it and not appending ['and', ['id' => ..], ['id' => ..], ['id' => ..], ...] to the SocialNetworkAccountQuery object every time as it does now.

Already tested locally, I'll submit a PR!