[MCC-961880] Add MAuthASGIMiddleware

[ejinotti-mdsol]

Adds a generic ASGI middleware for use with popular async frameworks like FastAPI.

@mdsol/architecture-enablement @omarmartinez325

[jcarres-mdsol]

I guess it is the path of less friction but I'm a little bit concerned with the approach.
AFAIK we never added rails specific code to mauth-ruby or spring code to mauth-java, because those ecosystems had abstractions common to all frameworks, there is no such thing for Python? Each framework has its own middleware kind of thing?

I guess creating another Python package even if hosted here in the same repo just for this would be overkill?

[ykitamura-mdsol]

there is no such thing for Python?

probably we can make this as ASGI middleware: https://fastapi.tiangolo.com/advanced/middleware/

Flask is different (WSGI) so I think we should keep it as-is.

[ejinotti-mdsol]

hmm @jcarres-mdsol makes a good point, I just hadn't noticed it. it is also pretty easy to make a middleware for WSGI.

I think we should do generic WSGI/ASGI middlewares for a future version 2.0.. as for this PR we can either:

1. still just merge anyway
2. remove the custom fastapi authenticator and merge just for the python version changes
3. close the PR

FWIW, i prob would not bother to use this in that PoC we are doing and would instead wait for generic middlewares from 2.0. As for doing 2.0, i could prob get to that later this week.

[ejinotti-mdsol]

Actually:

4. i can just convert this PR to do an ASGI middleware, then we can do WSGI middleware later in 2.0 for flask etc

think i will go with (4) unless someone has a preference

[ykitamura-mdsol]

4. i can just convert this PR to do an ASGI middleware, then we can do WSGI middleware later in 2.0 for flask etc

sounds good to me 👍

FYI, the historical reason why we have Flask Mauth library is that we have combined this library into mauth-client-python: https://github.com/mdsol/flask-mauth
probably we should have changed this into WSGI middleware at that time but we didn't...

[ykitamura-mdsol]

although there was no 1.1.15 so i went with 1.1.14

🤔

https://python-poetry.org/history/#1115---2022-08-22

[ejinotti-mdsol]

@ykitamura-mdsol yea, you are correct. i missed 1.1.15 hiding between the RC tags when looking at the tag list on their github. should've just looked at their nicely formatted doc site 🤣

[ykitamura-mdsol]

it might be good to have a "NoAppStatus" middleware also like the one we have in ruby or is there a way to not authenticate app_status calls?

[ejinotti-mdsol]

So in FastAPI you can mount an app with different middlewares onto the main app.. but it gets pretty weird in that you would need the main app to be open and have /app_status and then your actual routes in the sub-app that has the middleware. Something like:

open_app = FastAPI()
protected_app = FastAPI()
protected_app.add_middleware(MAuthASGIMiddleware)

@open_app.get("/app_status")
async def app_status():
    return {"msg": "ok"}

@protected_app.get("/")
async def root():
    return {"msg": "protected root"}

open_app.mount("/apis", protected_app)

But I can add an option to the middleware constructor for a list of exempt paths since that might be nicer.

[danielloganking]

LGTM

[mahmed-mdsol]

LGTM! Just one comment around shallow/deep copying of the exemption list, otherwise looks ready to go for me.

[mahmed-mdsol]

Do we want to keep a reference or make a shallow copy of the set? As is, if a set is passed in, changes to that set will (potentially accidentally) change this exemption set in the middleware, which feels like it might be surprising behavior.

Suggested change

	self.exempt = exempt
	self.exempt = exempt.copy()

(Also not sure what best practices are, but I typically like to set the argument to None and set the default here, since setting it in the arg list won't prevent someone from passing None in. e.g.

    def __init__(self, app: ASGI3Application, exempt: set = None) -> None:
        self._validate_configs()
        self.app = app
        self.exempt = exempt.copy() if exempt else set()

)

[ejinotti-mdsol]

for making a copy, yea good idea. will do.

for None as the default.. hmm does that mean we should change the typing notation to Union[set, None]?? theoretically the type hints should prevent them from passing a non-set for that arg. how about a compromise: raise TypeError if it's not a set?

[ejinotti-mdsol]

actually nevermind the compromise, i see the proper way to do type hints when using None default is to use Optional:

https://stackoverflow.com/a/49724148