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

[11.x] Improve authenticateViaBearerToken() performance #1447

Merged
merged 1 commit into from
May 26, 2021
Merged

[11.x] Improve authenticateViaBearerToken() performance #1447

merged 1 commit into from
May 26, 2021

Conversation

alecpl
Copy link
Contributor

@alecpl alecpl commented May 26, 2021

One select from oauth_access_tokens and one from oauth_clients less. The issues mentioned in #382.

Before:

select * from `oauth_access_tokens` where `id` = ? limit 1
select * from `oauth_access_tokens` where `id` = ? limit 1
select * from `oauth_clients` where `id` = ? limit 1
select * from `oauth_access_tokens` where `id` = ? limit 1
select * from `oauth_clients` where `id` = ? limit 1

After:

select * from `oauth_access_tokens` where `id` = ? limit 1
select * from `oauth_clients` where `id` = ? limit 1
select * from `oauth_access_tokens` where `id` = ? limit 1

as you see there's still one redundant query, but it might be harder to get rid of it.

@driesvints driesvints changed the title Improve authenticateViaBearerToken() performance [11.x] Improve authenticateViaBearerToken() performance May 26, 2021
@driesvints
Copy link
Member

@billriess I think it's best if you go over these changes if you can spare the time?

// Finally, we will verify if the client that issued this token is still valid and
// its tokens may still be used. If not, we will bail out since we don't want a
// user to be able to send access tokens for deleted or revoked applications.
if ($this->clients->revoked($clientId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check still happening somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$this->clients->findActive() above makes sure the $client is not revoked.

@billriess
Copy link
Contributor

I can take a look at this in the next day or two but glancing over it seems good.

@taylorotwell
Copy link
Member

I see this was sent to master. Is there a breaking change? Should this go to 10.x?

@alecpl
Copy link
Contributor Author

alecpl commented May 26, 2021

@driesvints didn't like the protected method removal.

@taylorotwell taylorotwell merged commit 95bdc16 into laravel:master May 26, 2021
@driesvints
Copy link
Member

@alecpl @taylorotwell technically we could do the changes on 10.x if we left the method in there.

@billriess thanks for reviewing 👍

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.

4 participants