Improve consistency of current user's data

[Yorwba]

This pull request addresses issue #2614, which has multiple causes:

1. The first reason the profile picture was replaced by the text "Former member" is that this was mistakenly used as the alt text for all users, because the username wasn't passed through to the image-tag generating function.

   To prevent similar problems from occurring in the future, I refactored the function to take the whole user object instead, so you can't accidentally pass it only the username or only the image file.

2. The second reason the profile picture was replaced by the text "Former member" is that the image file to use for the current user wasn't correctly changed after deleting the picture, causing the image to break.

   The underlying issue is that CurrentUser was using the data from $this->Auth->user(), which is stored in the session when authenticating and not reliably updated when the user data changes. This could also cause various other issues (e.g. what if a user's access gets restricted, but they're still logged into a session with higher privileges?) so I think the best solution is to always get the current data from the database.

[jiru]

While this definitely fixes this issue (as well as the need of having a user to logout and login after changing his role), I am concerned with the design of pooling the users table on each and every request, most of the time needlessly. Pooling is also what we do to display the unread private message counter at the top-right, and it bloats the list of fixtures required to run most of our controller tests. I feel like it’s not a good design and I wonder if there is any better way to go.

As for the profile picture issue, you could as well update the session data after saving it. That’s what we do everywhere else, for example:

tatoeba2/src/Controller/UserController.php

Lines 318 to 325 in 9850fd4

	$this->Users->patchEntity($user, $data, ['fields' => $allowedFields]);
	$savedUser = $this->Users->save($user);
	if ($savedUser) {
	$this->Auth->setUser($savedUser->toArray());
	$this->Flash->set(
	__('Profile saved.')
	);

[jiru]

Well I’ve thought it over and I can’t think of a better way to go. The current implementation fails in a lot of ways, including when a user logs in from two different browsers simultaneously. We already pool the users_languages table in CurrentUser::_setProfileLanguages() anyway. You could then remove the call to setUser() that I mentioned, as well as other similar calls.

My only remaining concern is that I think (haven’t tested) that the Auth component won’t know about a user role update because you don’t run $this->Auth->setUser() with the latest user data. Because of this, the restrictions defined in config/auth_actions.php won’t apply match to the new role.

[Yorwba]

Aside: I think you mean "poll" instead of "pool".

You could then remove the call to setUser() that I mentioned, as well as other similar calls.

Will do.

the Auth component won’t know about a user role update because you don’t run $this->Auth->setUser() with the latest user data.

I was actually calling $this->Auth->setUser() at first, but then the request failed with an "auth error" (if I remember correctly), so I figured that the Auth component isn't supposed to be used like that.

Because of this, the restrictions defined in config/auth_actions.php won’t apply match to the new role.

Thanks for pointing these out to me. I thought the permission checks all went through CurrentUser::canEditSentenceOfUserId and the like, so it would be enough to keep CurrentUser up-to-date. I'll look into updating the Auth user data as well.

[Yorwba]

I was actually calling $this->Auth->setUser() at first, but then the request failed with an "auth error" (if I remember correctly), so I figured that the Auth component isn't supposed to be used like that.

The problem was that calling Auth->setUser invalidates the current session, causing the security check for POST requests to fail. This is not a problem if the call to setUser happens in beforeRender instead of beforeFilter.

I thought that would make the other setUser calls entirely redundant, but one of the tests didn't agree. When changing the user data, there would a delay of one request until the new data reaches Auth->user. So I opted to update CurrentUser after calls to Users->save.

[Yorwba]

So I opted to update CurrentUser after calls to Users->save.

Scratch that part. I tested the wrong commit. Turns out beforeRender doesn't get called if there's nothing to render because the request is redirected. So Auth->setUser has to be called directly to make changes take effect immediately.

[jiru]

I thought that would make the other setUser calls entirely redundant, but one of the tests didn't agree. When changing the user data, there would a delay of one request until the new data reaches Auth->user. So I opted to update CurrentUser after calls to Users->save.

Hah, I bet you’re talking about testSaveBasic_changingEmailUpdatesAuthData()! I wrote it long ago because of a similar bug #526. Of course the test fails because it only performs one request; it’s tied to that design of calling Auth->setUser() on user "write" rather than "read". Fell free to rewrite or remove that test.

[Yorwba]

I've rewritten the test to actually follow the redirect before checking that the Auth data was changed.

[trang]

For what I've checked, looks good to me.
Maybe @jiru you can confirm that this is okay for you too?
In any case, if no objections I'll merge this pull request this weekend 🙂