Closed Oisins closed 3 years ago
I think that we shouldn't just let the underlying exception bubble out; it's cleaner to only raise "internal" exceptions defined in this project for anticipated error conditions.
I would be willing to merge a PR that adds an exception
attribute to AuthenticationResponse
that's set if the failure was due to an exception.
Since we've already got breaking changes lined up for 1.0, I'd also be willing to entertain a modification such that authenticate
returns information about the user on success and always signals failure via an exception. In this case, the original exception would be available as the __cause__
of whatever internal exception we raise.
Note that, either way, exposing the details of what went wrong shouldn't be exposed to the user; it should basically be split into two categories: problems related to the user input that the user can potentially fix and problems unrelated to the input that the user can't do anything about but try again later.
I agree with you about no letting lower level exceptions be raised. I created a Pull Request to add the additional Attribute.
Because the
ldap_manager.authenticate('username', 'password')
method only returnssuccess
orfail
, the application has no way of knowing if the issue is on the network or the user entered the wrong credentials. This is because all exceptions are caught on line 440 in__init__.py:
Would it be possible to reraise these exceptions or at least include them in the AuthenticationResponse object? Or am I just missing a way to get this information (except for the logs)?