Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Protect against ambiguous columns #43278

Merged
merged 3 commits into from
Jul 19, 2022
Merged

Conversation

BenWalters
Copy link
Contributor

@BenWalters BenWalters commented Jul 19, 2022

Resolving #43274

When applying a global scope to a User model that joins a secondary table, which also has an ID field, the retrieveById and retrieveByToken functions of Illuminate\Auth\EloquentUserProvider do not qualify the identifier field name in the where clause which. This subsequently causes a "Integrity constraint violation: 1052 Column 'id' in where clause is ambiguous" error.

Wrapping $model->getAuthIdentifierName() in $model->qualifyColumn() on both methods protects against this.
This is a non-breaking change and is in-line with other abstract queries/builders. Tests updated accordingly.

Example global scope:

class ActiveUsersScope extends AbstractGlobalScope implements Scope
{
    /**
     * Apply the scope to a given Eloquent query builder.
     *
     * @param Builder $builder
     * @param Model $model
     * @return void
     */
    public function apply(Builder $builder, Model $model): void
    {
        $userOrganizationModel = new UserOrganization();

        $builder
            ->join(
                $userOrganizationModel->getTable(),
                $userOrganizationModel->qualifyColumn('user_id'),
                '=',
                $model->qualifyColumn('id')
            )
            ->where($userOrganizationModel->qualifyColumn('status'), UserStatus::ACTIVE);
    }
}

@driesvints
Copy link
Member

@BenWalters please add a thorough explanation to your main PR description (see the PR template) so Taylor knows what this is about. Thanks.

@BenWalters
Copy link
Contributor Author

@driesvints updated! Let me know if anything else is needed.

@driesvints
Copy link
Member

@BenWalters maybe also share an example of the global scope

@BenWalters
Copy link
Contributor Author

@driesvints it's a WIP but I've added an example.

@driesvints
Copy link
Member

This looks like a breaking change so we're reverting this: #43353

@BenWalters
Copy link
Contributor Author

@driesvints no problem, I would however say that this is 'breaking' because of a 3rd party packages bad implementation. The Mongo DB package does not prefix fields with table names. They have only overridden some of the qualify column methods that are implemented on the abstract Model class that Laravel Framework provides.

@BenWalters
Copy link
Contributor Author

@driesvints sorry, I'd not seen the octane issue also. I cannot comment on that one.

@BenWalters
Copy link
Contributor Author

Having just looked at it though it's also because MongoDB is being used.

taylorotwell pushed a commit that referenced this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants