Framework: Auth::id() stays after clearing users table

[emadha]

• Laravel Version: 5.8.*
• PHP Version: 7.3.5
• Database Driver & Version: mysql

Description:

The Auth::id() keeps the id stored even when I delete the users table.

Steps To Reproduce:

1 - dd(Auth::id()); // After being logged in
2 - Delete the logged in user row, or clear the whole table
3 - dd(Auth::id()) again and it will show the stored value (the delete user's id)

Looks like it's store in the cookies, however that method causes issue with models using the Auth::id() for the user_id column using Auth::id() to get the value from.

Post::create(['user_id'=>Auth::id(),...]); // Is unsafe when user_id is nullable without further validation.

---

As per documentation:

// Get the currently authenticated user's ID...
$id = Auth::id();

But that is not the case, and the user should not be authenticated when it's not there in the first place.

Now i have to recheck anywhere i used Auth::id() in.

---

Doing Auth::check() ? : request()->session()->invalidate(); will clear that Auth::id() value, but will clear everything as well including csrf...

Doing Auth::logout() in a middleware like AfterSessionStarts will work :

public function handle($request, Closure $next)
{

    if (Auth::check()) {

    // I have some code here..

    } elseif (Auth::id()) {
         Auth::logout();
    }

   return $next($request);
}

It makes more sense if Auth::id() reflects the Auth()->user()->id or null and not the store value in Session.

[staudenmeir]

Please post the relevant parts of your code. Are you logging out the user when deleting them?

[emadha]

No, i'm simply deleting the user row from the database, so the session stays, but invalid, however the id can still be used in their browser, and that is unsafe without using Auth::check() before.
So in my case doing Eloquent::create(['user_id'=>Auth::id(),...]); will need extra validation because it didn't return null, and my case user_id is and can be nullable.

[emadha]

Doing a Auth::check() ? : request()->session()->invalidate(); will clear that value.

[emadha]

@staudenmeir as i said, that Eloquent i use can have a user_id of null, so no it's not protected by a middleware, to be clear it's a Reviews model, where the user doesn't have to be registered, and in case the user is register i use user_id column, otherwise i use the email, name, and user_id of null. In my tests it threw user id of 2 which is a user i deleted while testing, and confused me for sometime until i found out about this Auth::id() thing, doing a middlware to check if user is not Auth then do a Auth::logout() solved it. but that's a workaround and i don't think it's the best way to do this, Auth::id() need to check if the user really exists instead of taking the value from the stored session.

[staudenmeir]

I see the issue in cases like yours, but I'm not sure if we can change the current behavior. There's probably a reason for it (although it's not tested).

[devcircus]

This seems like an obvious expected behavior. Plus it seems strange to delete the user table like this or a single user without logging them out

[emadha]

@staudenmeir Auth::id() should return null if the value is not real, there's nothing special and no changing to behavior there, this right now is malfunctioned behavior.

@devcircus but it's there and happening, the documentation states that Auth::id() returns the id of the authenticated user not the stored and unchecked ID.

A real world example:

Let's say you have a User 1 with ID 2, and that user can write Reviews, and you delete that user (without logging them out), and in the Review Eloquent you store user_id of nullable|exists:users,id, in his browser, he can still submit reviews as user_id 2 even when that user does not exist in the table anymore since you're believing that Auth::id() will try to check if the id actually exists, thinking that Auth::id() is equal to Auth::user()->id, but no! Auth::id() is stored, and Auth::user()->id is the actual database object, that make is misguiding when people say Auth::id == Auth::user()->id; that's not true anymore.

so when i used to think that Auth::id() will validate and return null if i delete the user, that didn't happen, and now i need to check anywhere Auth::id() is used.

However i came up with a workaround to this, i created a middleware that checks:

public function handle($request, Closure $next)
{

    if (Auth::check()) {

        // I have some code here..

    } elseif (Auth::id()) {
         Auth::logout();
    }

    return $next($request);
}

since Auth::logout() will clear the session.

Now i understand that extra validation is always a good practice and i always do that, that's how i reached this issue, and for that i had to add that middlware (not sure if the best approach but it works).

---

So, no it's not expected behavior that Auth::id() should be like that, it's expected to give an actual value not stored value, and i still believe it's an issue not a misunderstanding or misusing the framework.

[laurencei]

Auth::id() is equal to Auth::user()->id, but no! Auth::id() is stored, and Auth::user()->id is the actual database object, that make is misguiding when people say Auth::id == Auth::user()->id; that's not true anymore.

Are you sure? I cant check at the moment - but the first "call" will always hit the database, and every subsequent call (in the one lifecycle) will be cached.

If we hit the database everytime you check the ID it would be very inefficient. If you delete a user, log them out at the same time.

[emadha]

Not convinced.

So you're telling me that Auth::user(), user(), request()->user() are cached ?

For every request made on the page Auth::user(), and all these functions will fetch the user object from the actual database and are not cached, if you check query logs you'll see SELECT * FROM users where id ....

What's preventing the Auth::id() from returning Auth::user()->id ? that will not hit the database everytime i check the id... because it's there anyway, on every request.

[laurencei]

So you're telling me that Auth::user(), user(), request()->user() are cached (as cookies/session) ?

No - I said for the one lifecycle per request.

you check query logs you'll see SELECT * FROM users where id ...

You should only see this once for the Auth related part.

[emadha]

@laurencei
I understand, so 2 of Auth::user() will not fetch from database, that is clear. But i guess you didn't understand the issue i'm talking about.

Try to login using one user, dd(Auth::id()); delete the user manual from database, refresh the page with dd(Auth::id()) will still there, so Auth::id() doesn't really fetch the actual user from database.

[laurencei]

Ok - I understand what you are saying. I'll have to check later.

[emadha]

Just making a guess here i didn't dig into its code yet, that Auth::id(); decrypts the php session and returns the ID stored in there, that makes it inconsistent, renaming it to Session::user_id()... or whatever related to that; will make more sense than Auth::id(); because when the user gets deleted, the Auth::id(); should return the Authenticated User id, Auth refers to Authenticated User, Authentication, whatever you wanna call it... and in that case there is no user to be authenticated in the first place.

[emadha]

The main reason why i didn't just ignore it and lived with the workaround is because just like me i'm quite sure that there are a lot of people out there assigning user_id column value from the Auth::id(); value and not from Auth::user()->id, because Auth::id() returns null when the user is not Authenticated while Auth::user()->id will throw error when the user is not authenticated, So Auth::id() works better for nullable user_id column. and all this time i thought that Auth::id() does something like

   return Auth::check() ? Auth::user()->id : null;

But no.

[emadha]

well, yes

/**
 * Get the ID for the currently authenticated user.
 *
 * @return int|null
 */
public function id()
{
    if ($this->loggedOut) {
        return;
    }

    return $this->user()
                ? $this->user()->getAuthIdentifier()
                : $this->session->get($this->getName());
}

[devcircus]

dd short-circuits the request which doesn't get to finish properly. How does this workout if you don't use the dd? Which would never be utilized in a production environment.

[emadha]

@devcircus
I am not discussing production environment.

if you don't like dd in my example you can {{Auth::id();}} and as i mentioned Post::create(['uid=>Auth::id()]); will be invalid.

There is an issue with naming here and functionality here. Auth means Auth and not Session.

Rename/Place that id() elsewhere outside Auth instead of just focusing on how weird my issue is, and showing me that i'm writing wrong and reckless code. How many times did you use Auth::id() for user_id column instead of Auth::user()->id, request()->user()->id, user()->id or any user id coming from the real user object?

[emadha]

And if Auth::id() gets it's value from session to make it faster, that doesn't make it any faster since all Auth::user() will be requested with every run, once as @laurencei mentioned.

[emadha]

Another Real world example

_Don't mind the code as i'm imagining a scenario and writing the code here_

Let's say you record page views per user, and as a table row for example table viewed_by which stores if a certain page was viewed by a certain user, and the user can be null, in that case it will add a row which can be used to count the views including non registered users, and with valid id for registered users, and in that table you have foreign key assigned to the users table.

When User ID 2 views the page it inserts a row into DB ViewedBy::create(['user_id'=>Auth::id()]); and you decide to delete the user manually from db for some weird reason, or from backend without logging them out ( cuz i found about that just now, that when i need to delete the user i have to log them out as well.. weird enough), in that case if the USER 2 refreshes that page after being removed he will have his id stored in ViewedBy knowing that his user id isn't in database anymore, that will throw an unneeded error.

That's one of many examples where you have a nullable|exists:users,id column...

[mfn]

What is your proposal for a fix?

[mpyw]

@emadha I strongly agree with your proposal.

/**
 * Get the ID for the currently authenticated user.
 *
 * @return int|null
 */
public function id()
{
    return optional($this->user())->getAuthIdentifier();
}

This seems to be the correct implementation.
I think it is a big problem that there are inconsistency between optional(Auth::user())->id and Auth::id(). They should always return the same results.

[emadha]

The only place i found reference to Auth::id() was in:

https://github.com/laravel/framework/blob/73da739fb5783208e582a7d8af6712b73a75d748/src/Illuminate/Foundation/Exceptions/Handler.php#L154-L157

So i don't think it's a big issue to fix that, and i don't see a good reason to use the value from Session for this, and specifically in this method.

https://github.com/laravel/framework/blob/73da739fb5783208e582a7d8af6712b73a75d748/src/Illuminate/Auth/SessionGuard.php#L191-L205

but instead of using the optional() helper it's a tiny bit faster to just use:

return $this->user() ? $this->user()->getAuthIdentifier() : null;

[driesvints]

This is the expected behavior. Deleting DB rows shouldn't change the state of your session storage. I know this might look strange with the obvious use case in this issue but this has been the current behavior since the beginning of the framework. If you want this to be changed, please open up an issue on the ideas repo. Also see dozens of previous opened issues on this repo by searching the issues.

[emadha]

expected and doesn't make sense.

[Stalinko]

I hit the same issue.
That's so weird that Auth::id() and auth()->id() return $id from session in case when it fails to get the user from DB. There is no sense.

I've got real use case - some user gets logged into a web-app using session driver. Then he logs into the mobile app and deletes his own account. Then he opens the website again where he still has an active session and here we face the issue: auth()->id() returns the ID from session while all other auth-functions say that the user isn't logged in. Thus the parts of code which rely on auth()->id() get fooled and must check auth state through other functions additionally.

If it's not supposed to fix the id() function then at least there must be a warning that you can not rely on this function when checking auth status because it can return wrong info.