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

Error messages can be misleading when multiple authenticators are present #189

Open
RookieRick opened this issue Jul 3, 2020 · 4 comments
Labels
enhancement New feature or request error-handling Concerns automatic error handling v2.1

Comments

@RookieRick
Copy link
Contributor

https://github.com/plangrid/flask-rebar/blob/master/flask_rebar/rebar.py#L123-L131
If an app uses multiple authenticators, we use a "first past the post" approach - authenticators will do their thing until one succeeds. If NONE of them succeed, the error message returned is always the first error encountered. This can make debugging tricky if:

  1. you have two (or more) authenticators
  2. they both fail
  3. you want to know why the SECOND one failed.

Maybe instead of just returning the message from the first authenticator we should collect all failure messages and return those in the body of the error response (if we wanted to make it backwards-compatible we could perhaps augment the response json from e.g.
{ "message": "No auth token provided." }
to e.g.,
{ "message": "No auth token provided.", "additional_messages": ["Nope!", "authenticator3 barfed too!"] }

@RookieRick RookieRick added enhancement New feature or request error-handling Concerns automatic error handling labels Jul 3, 2020
@akibrhast
Copy link

Hey @RookieRick , I ended up here cause I was strolling through the Flask discord channel. I am a completely newbie, so this might be a dumb question but I was wondering if this ticket has any relation to #79 ??

@RookieRick
Copy link
Contributor Author

@akibrhast No such thing as a dumb question in my book :D Been a while since I looked at that other one but skimming to refresh I think this one is distinct in that it's more about how our authenticators approach behaves whereas #79 is more general error handling (typically around malformed input/output)

@airstandley
Copy link
Contributor

Love the idea of providing the errors under a different key; wish I'd thought of that 😄

On a separate note: the main message should really just be a generic "Unauthorized" message, might be a change to roll into 2.0 since it breaks backward compatibility for anything that parses the message.

@RookieRick RookieRick added v2.0 breaking changes for next major release v2.1 and removed v2.0 breaking changes for next major release labels Oct 23, 2020
@RookieRick
Copy link
Contributor Author

OK, tagging this one as 2.1 because we can do it without breaking change following Andrew's suggestion of putting generic "Unauthorized" in message and adding "additional_messages" or "authenticator_results" or some-such as a bolt-on that doesn't break backward compat.
(And if returning generic "Unauthorized" in message breaks anything because people are parsing a human-readable message, to drive logic.. well... let them eat cake 🙈 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request error-handling Concerns automatic error handling v2.1
Projects
None yet
Development

No branches or pull requests

3 participants