Skip to content

Commit

Permalink
[8.x] Protect against ambiguous columns (#43278)
Browse files Browse the repository at this point in the history 
* [8.x] Protect against ambiguous columns

Resolving #43274

* Updating tests.
  • Loading branch information
@BenWalters
BenWalters authored Jul 19, 2022
1 parent 97e68c6 commit a6d9307
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 6 deletions.
4 changes: 2 additions & 2 deletions src/Illuminate/Auth/EloquentUserProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function retrieveById($identifier)
$model = $this->createModel();

return $this->newModelQuery($model)
->where($model->getAuthIdentifierName(), $identifier)
->where($model->qualifyColumn($model->getAuthIdentifierName()), $identifier)
->first();
}

Expand All @@ -65,7 +65,7 @@ public function retrieveByToken($identifier, $token)
$model = $this->createModel();

$retrievedModel = $this->newModelQuery($model)->where(
$model->getAuthIdentifierName(), $identifier
$model->qualifyColumn($model->getAuthIdentifierName()), $identifier
)->first();

if (! $retrievedModel) {
Expand Down
12 changes: 8 additions & 4 deletions tests/Auth/AuthEloquentUserProviderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ public function testRetrieveByIDReturnsUser()
$mock = m::mock(stdClass::class);
$mock->shouldReceive('newQuery')->once()->andReturn($mock);
$mock->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$mock->shouldReceive('where')->once()->with('id', 1)->andReturn($mock);
$mock->shouldReceive('qualifyColumn')->with('id')->andReturn('users.id');
$mock->shouldReceive('where')->once()->with('users.id', 1)->andReturn($mock);
$mock->shouldReceive('first')->once()->andReturn('bar');
$provider->expects($this->once())->method('createModel')->willReturn($mock);
$user = $provider->retrieveById(1);
Expand All @@ -39,7 +40,8 @@ public function testRetrieveByTokenReturnsUser()
$mock = m::mock(stdClass::class);
$mock->shouldReceive('newQuery')->once()->andReturn($mock);
$mock->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$mock->shouldReceive('where')->once()->with('id', 1)->andReturn($mock);
$mock->shouldReceive('qualifyColumn')->with('id')->andReturn('users.id');
$mock->shouldReceive('where')->once()->with('users.id', 1)->andReturn($mock);
$mock->shouldReceive('first')->once()->andReturn($mockUser);
$provider->expects($this->once())->method('createModel')->willReturn($mock);
$user = $provider->retrieveByToken(1, 'a');
Expand All @@ -53,7 +55,8 @@ public function testRetrieveTokenWithBadIdentifierReturnsNull()
$mock = m::mock(stdClass::class);
$mock->shouldReceive('newQuery')->once()->andReturn($mock);
$mock->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$mock->shouldReceive('where')->once()->with('id', 1)->andReturn($mock);
$mock->shouldReceive('qualifyColumn')->with('id')->andReturn('users.id');
$mock->shouldReceive('where')->once()->with('users.id', 1)->andReturn($mock);
$mock->shouldReceive('first')->once()->andReturn(null);
$provider->expects($this->once())->method('createModel')->willReturn($mock);
$user = $provider->retrieveByToken(1, 'a');
Expand All @@ -78,7 +81,8 @@ public function testRetrieveByBadTokenReturnsNull()
$mock = m::mock(stdClass::class);
$mock->shouldReceive('newQuery')->once()->andReturn($mock);
$mock->shouldReceive('getAuthIdentifierName')->once()->andReturn('id');
$mock->shouldReceive('where')->once()->with('id', 1)->andReturn($mock);
$mock->shouldReceive('qualifyColumn')->with('id')->andReturn('users.id');
$mock->shouldReceive('where')->once()->with('users.id', 1)->andReturn($mock);
$mock->shouldReceive('first')->once()->andReturn($mockUser);
$provider->expects($this->once())->method('createModel')->willReturn($mock);
$user = $provider->retrieveByToken(1, 'a');
Expand Down

9 comments on commit a6d9307

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks my application:

Before with only $model->getAuthIdentifierName() was working:
"command":{"find":"users","filter":{"$and":[{"_id":{"$oid":"628e6230b59b425fd10c1543"}},{"deleted_at":null}]},"limit":1

Now with the change ($model->qualifyColumn($model->getAuthIdentifierName())):
"command":{"find":"users","filter":{"$and":[{"users._id":{"$oid":"628e6230b59b425fd10c1543"}},{"deleted_at":null}]},"limit":1
the search column users._id not working

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenWalters any suggestion?

@BenWalters
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rennorodrigo what's the use case? Is it when calling retrieveById?

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when i call auth()->user()->id from the controller , the JWTGuard is calling on the line:

return $this->user = $this->provider->retrieveById($payload['sub']);

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/PHP-Open-Source-Saver/jwt-auth/blob/main/src/JWTGuard.php line 108

@BenWalters
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rennorodrigo are you using MongoDB?

@BenWalters
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're suffering the same issue as posted here - mongodb/laravel-mongodb#2424
And I'm partly making that assumption on the fact that your primary key is _id and not id as that is what's set by their abstract model - https://github.com/jenssegers/laravel-mongodb/blob/master/src/Eloquent/Model.php

There's a workaround suggested in the issue listed above. I've also commented there with what I think needs to be changed in that package.

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i am using mongodb

@rennorodrigo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! this workaround fixed the error

Please sign in to comment.