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.

Sign up for GitHub

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

Jump to bottom

Validator: raise exception when no constraint is given #351

Merged
merged 2 commits into from
Apr 3, 2020
Merged

Validator: raise exception when no constraint is given #351

merged 2 commits into from
Apr 3, 2020

Conversation

Slamdunk
Copy link
Collaborator

Hi, today I expected a token to fail, but the assertion made as described in https://lcobucci-jwt.readthedocs.io/en/latest/validating-tokens/ didn't raise any error.
After a while I found no build-in Constraint is provided (and that's ok for me), but also no error is raised when this happen.

I'm scared that many users would fall into my mistake, a pretty dangerous mistake!

lcobucci
lcobucci requested changes Apr 3, 2020
View reviewed changes
Copy link
Owner

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

This is quite a good point, thanks for contributing :)

Would you please also update the documentation to have it mentioning that behaviour and add NoConstraintsGiven as a @throws to both methods (interface)?

src/Validation/Validator.php Outdated Show resolved Hide resolved
src/Validation/Validator.php Outdated Show resolved Hide resolved
test/unit/Validation/ValidatorTest.php Show resolved Hide resolved
test/unit/Validation/ValidatorTest.php Outdated Show resolved Hide resolved
@Slamdunk
Copy link
Collaborator Author

Slamdunk commented Apr 3, 2020

Everything done, expect the ...[] one 😸

@lcobucci lcobucci added this to the 4.0.0-alpha4 milestone Apr 3, 2020
@lcobucci lcobucci self-assigned this Apr 3, 2020
lcobucci
lcobucci approved these changes Apr 3, 2020
View reviewed changes
Copy link
Owner

@lcobucci lcobucci left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is quite good, thanks again!
I'll just rebase the branch to organise the history and have a more descriptive message (and have the mandatory signed commits).

Slamdunk and others added 2 commits April 3, 2020 12:42
@Slamdunk @lcobucci
Disallow empty constraint list for token validation
9d9f4b9 
Not raising any warnings for this scenario might give confidence that
everything is working, where in fact they aren't.

This makes it required for users to pass at least a constraint, making
things more explicit and removing the possibility of confusion.
@lcobucci
Make warnings a bit more visible
53e50f4 
Ensuring nobody will ever miss this.
@lcobucci lcobucci merged commit bbfed13 into lcobucci:master Apr 3, 2020
@lcobucci
Copy link
Owner

lcobucci commented Apr 3, 2020

@Slamdunk 🚢

@Slamdunk Slamdunk deleted the no_contraint_raise_error branch April 3, 2020 10:48
@Slamdunk Slamdunk mentioned this pull request Feb 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcobucci lcobucci lcobucci approved these changes

Assignees

@lcobucci lcobucci

Labels
Improvement
Projects
None yet
Milestone
4.0.0-beta1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Slamdunk @lcobucci