sitepoint-editors / Rauth

A basic annotation-based ACL package for PHP
MIT License
41 stars 4 forks source link

Reason #1

Closed Swader closed 8 years ago

Swader commented 8 years ago

Todo: implement reasons.

I.e. when authorize() returns false, the user should be able to call $rauth->getReason() or something and get coherent feedback about why an authorization attempt failed. This needs to take into account bans, mismatches due to mode, and anything else additionally implemented. So biggest problem is finding a good reason format.

AndrewCarterUK commented 8 years ago

Could authorize() throw an authorization exception on failure, with the reason being the exception message? You could even have different exceptions for different reasons.

Swader commented 8 years ago

It could but:

AndrewCarterUK commented 8 years ago

I guess there is a use case for knowing all of the reasons that an auth failed. Maybe you need a Reason object and an auth exception could return an array containing at least one.

Swader commented 8 years ago

Hmm. That's two new objects and complicates the main class by quite a lot, due to having to create the reasons when they occur and push them into the exception to be thrown. It would be a clean solution, but I'm afraid of potential overkill.

Swader commented 8 years ago

This is what I'm currently thinking:

My current concern is that AuthException would then have the mode set on itself, but the reasons would be mode-less. That's two objects saying a part of the whole "what went wrong message". This lends itself nicely to #6 because third party modes can throw their own exceptions / modes then and Reasons aren't coupled to modes, but feels wrong.

Swader commented 8 years ago

Alternative is adding another property into Reason: $mode. However, this seems superfluous given that a set of reasons will all have the same mode if in the same exception.

Also considered having a separate exception for each mode (NoneException, AndException, etc) but this would quickly get out of hand, especially when trying to build the catch blocks, unless one builds a single catch block that catches the parent AuthException, in which case it's the same as if we just add $mode into AuthException and grab that - no need for extra classes.

Swader commented 8 years ago

Rubberducking further, perhaps $errorType should be removed from Reason altogether, and the AuthException should get a $type property which can be set to: ban, and, or, or none. This gives sufficient context to the exception and all reasons inside it, and the reason objects still contain the group, has, and needs, identifying the exact problem areas.

Swader commented 8 years ago

Closed via #8 but still up for improvement. Make a new issue / PR for any additional tweaks.