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

Revert removal is valid #276

Merged
merged 3 commits into from May 3, 2021
Merged

Revert removal is valid #276

merged 3 commits into from May 3, 2021

Conversation

doekenorg
Copy link
Contributor

As a follow up to #275 and because of the comment @driesvints made on that PR:

@doekenorg best that you send a follow up pr as its unlikely that Taylor will see your reply

I noticed you removed the $isValid parameter from the callback in your reformatting. Any particular reason you did this? I would like to ask to revert that because now I cannot actually add to the validation, but only completely replace it. If I don't know if the Guard deemed it valid I need to check those validations again myself inside the callback; which is a bit of a pain.

Say I want to use the normal behavior, but only use the token one time, I now have to re-add the checks to see if the token hasn't expired already. This seems a bit counter productive to me. It can even make the validation less secure, because If iI only implemented a check to see if the token hasn't been used before, I can potentially use a token that has expired long ago.

@taylorotwell
Copy link
Member

I don't understand what you mean - the callback is only invoked if the token was valid. Why would you want to invoke it with a token we already know is invalid?

@doekenorg
Copy link
Contributor Author

I don't understand what you mean - the callback is only invoked if the token was valid. Why would you want to invoke it with a token we already know is invalid?

This isn't true. The callback is not invoked if we didn't find an access token. But an AccessToken can exist, and be invalid at the same time. Then the callback should be called with false.

This way we can add to the validation by saying: according to my custom rules, this access token actually IS valid. Or: great you have a ticket that is valid for today, but you already used it; so it's not.

@taylorotwell
Copy link
Member

OK - well even so, this PR doesn't instruct the callable to be invoked on invalid tokens. There is still an $isValid check before the callable is invoked.

@doekenorg
Copy link
Contributor Author

Ah, you're right. It wasn't in my original PR so I overlooked it. Removed it.

@taylorotwell taylorotwell merged commit 40978a1 into laravel:2.x May 3, 2021
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

2 participants