BP-763: Add support for multiple authenticators

[airstandley]

Corresponds to Issue #121

• Extended deprecated_parameters decorator to allow a coercion function to convert between old and new styles of parameters.
• Adds support for handlers to have multiple Authenticators.
• Adds support for registries to have multiple Authenticators as their default authentication.
• Should mark authenticator parameters as deprecated using the deprecation utils

JIRA Tickets
BP-763

[airstandley]

Thanks to Rick's awesome deprecation wrapper, this should be backward-compatible until version 3.0.

[airstandley]

@RookieRick Let me know if I should change this to 2.0

I figured I'd play it safe since we are probably getting close to wanting a 2.0 release, but if you're comfortable planning on rolling this in with the marshal_schema changes then that sounds good to me.

[Sytten]

I don't agree with forcing the user to use a list, IMO the user should be able to pass only one authenticator and we convert it to a list in the background. Otherwise it's just more boilerplate for for no reason when you always use one...

[RookieRick]

A few comments/suggestions:

1. Love the extension to deprecated_parameters for coercing values!!
2. I share Sytten's concern around forcing a list, I think we could address this by:
   2.a.: Keep this "temp" bit permanently: https://github.com/plangrid/flask-rebar/pull/122/files#diff-6b96ea2e1cb127178bf3bfc56492682aR99-R101
   2.b. Use a slightly more convoluted lambda for the coercion, something like:
   lambda x: [x] if isinstance(x, Authenticator) else x if x is not None else []
   so if we get a single Authenticator we coerce to a list, if we get anything else, we pass it along on the assumption that it's a list of Authenticators and trust the programmer isn't passing us garbage (and of course replace None with default empty list as before)

Thoughts?

[RookieRick]

@RookieRick Let me know if I should change this to 2.0

I figured I'd play it safe since we are probably getting close to wanting a 2.0 release, but if you're comfortable planning on rolling this in with the marshal_schema changes then that sounds good to me.

I'm down to give extra time - honestly we could probably support it indefinitely 😂

[airstandley]

A few comments/suggestions:

1. Love the extension to deprecated_parameters for coercing values!!
2. I share Sytten's concern around forcing a list, I think we could address this by:
   2.a.: Keep this "temp" bit permanently: https://github.com/plangrid/flask-rebar/pull/122/files#diff-6b96ea2e1cb127178bf3bfc56492682aR99-R101
   2.b. Use a slightly more convoluted lambda for the coercion, something like:
   lambda x: [x] if isinstance(x, Authenticator) else x if x is not None else []
   so if we get a single Authenticator we coerce to a list, if we get anything else, we pass it along on the assumption that it's a list of Authenticators and trust the programmer isn't passing us garbage (and of course replace None with default empty list as before)

Thoughts?

I think that @Sytten makes a good argument that having to wrap a single Authenticator in a list/tuple is kind of annoying boilerplate. I'm down to lean into type checking to accept either an iterable of Authenticators, a single Authenticator, or None for every authenticators parameter.

However I think that all the class members that store authenticator(s) should store a list or otherwise things like the swagger generation code are going to become unbelievably convoluted.

Let me give that a shot and we'll see how it looks.

[RookieRick]

I think you'll still need to update that coercion lambda:
authenticator=("authenticators", "3.0", lambda x: [x] if x is not None else [])
As is, if someone were to pass a list of Authenticators but did so with the old parameter name, I believe it would get coerced into a list of lists which might produce... unexpected results 😂

[airstandley]

I think you'll still need to update that coercion lambda:
authenticator=("authenticators", "3.0", lambda x: [x] if x is not None else [])
As is, if someone were to pass a list of Authenticators but did so with the old parameter name, I believe it would get coerced into a list of lists which might produce... unexpected results 😂

I'm not sure that's necessary?
authenticator has never supported passing a list of Authenticators, so it seems reasonable it would break things to me.

[RookieRick]

I'm not sure that's necessary?
authenticator has never supported passing a list of Authenticators, so it seems reasonable it would break things to me.

It's quite possibly overkill.. The hypothetical scenario I had in mind was "existing Rebar user notices that you can now pass a list of Authenticators but fails to notice the change in parameter name and tries to change some existing code to pass authenticator=[auth1, auth2]" Definitely a programmer error in that case but my thinking was it's one we could anticipate and handle gently. TBH I'm cool with this one either way

[airstandley]

I'm not really a fan of "fixing programmer errors", but I do think that errors should be as explicit and easy to debug as possible. You do bring up a good point that if a programmer passed authenticator=[auth1, auth2] (which is a really easy typo) it could lead to some really confusing behaviour.

What's your thought on changing the lambda to be something like lambda x: [x] if isinstance(x, Authenticator) else [] if x is None else raise ValueError('authenticator must be an instance of Authenticator or None.'), so that we raise a clear error if someone doesn't read the docs closely or makes a typo and drops the 's'?

[RookieRick]

What's your thought on changing the lambda to be something like lambda x: [x] if isinstance(x, Authenticator) else [] if x is None else raise ValueError('authenticator must be an instance of Authenticator or None.'), so that we raise a clear error if someone doesn't read the docs closely or makes a typo and drops the 's'?

Works for me!