updated!

👍 for this change.

Thank you @weaverryan.

I disagree with this PR, I understand the issue but this is really a user error and returning a boolean won't improve security here. You'd have to keep track of any errors thrown and you cannot do return false; in the end as this would always make it fail. You cannot do return true; in the end either because It would still yield the same result as without a boolean.

Result:

• Additionally to throwing an exception, I would have to keep track of the result in a boolean because I have to return the success state.
• This results in me putting return true; in the bottom of the function because I won't bother keeping track if I already throw exceptions.

With the above stated, I consider this change to be useless as it only adds an additional requirement which makes it more complex to get it working in the first place. If a developer really makes a mistake, his (unti)test should catch that and fail.

Could this be reverted perhaps?

As the person who brought it up, I thank you for considering and making this change.

@iltar this looks like defensive programming. Of course it's not required, defensive programming is never required. But still, it allows one to write safer code by default. I see the benefit of it. If the downside is adding/managing a return true, I'd personally balance my preference for the defensive programming argument. Why don't you?

The current check is true !== that means that people who do test their code properly will have to add a non-meaningful return statement.

If the check were to be changed to false ===, people can opt-in and still return false. I don't see how this would make it safer though, if you don't throw an exception and forget to update your result, it will still fail. This will again come back to the point where this is a user error (probably because it lacks tests).

It also works differently than the usercheckers now. checkPreAuth and checkPostAuth also rely on throwing exceptions only. If this isn't reverted, the behavior of the user checks should imo be the same.

The return statement is meaningful: you return true to pass checkCredentials, else it fails. The method acts like an areCredentialsValid() method.

Additionally to throwing an exception, I would have to keep track of the result in a boolean because I have to return the success state.

Why would you need to keep track of the state? If you know access should be denied, return false; immediately. If you know access should be granted, return true immediately. This is the same as before: if you knew access should be denied, you threw an exception. If you know access should be granted, you returned (null). With both, you have an easy way to exit from the function.