Scope of issue of 0.6.2 release potentially breaking clients (not a flask-login fault)

[potiuk]

I wanted to understand and discuss the scope of 0.6.2 potentially breaking some (misbehaving) clients.

Note. I am not complaining - I know the fault was on our side (partially at least), so this is not a request to change it, but I wanted to understand whether i understand the problem correctly, and maybe it will give others an opportunity to show their cases as it might be more widespread than maintainers of flask-login might be aware of.

Context:

After yesterday's 0.6.2, our tests on main started to fail. This was right after the new "Werkzeug" release broke our docker tests in CI (which we fixed by limiting Werkzeug while waiting for 0.6.2). Those tests of our are kind of "canary" tests - that detect if there is a dependency breaking Airlfow unknowingly (we have > 600 dependencies as I described yesterday in #686 (later marked as off-topic, but I think it was pretty relevant).

So exaclty those "canary" tests started to fail after 0.6.2 release with google_openid test failing - not being able to authenticate a user.

Example failure here: https://github.com/apache/airflow/runs/7520608757?check_suite_focus=true manifesting with

tests/providers/google/common/auth_backend/test_google_openid.py::TestGoogleOpenID::test_success: AttributeError: 'AnonymousUser' object has no attribute 'email'

The problem was that the user that was supposed to authenticate our custom authentication backend was not authenticated.
After a bit of investitation I found and fixed the root cause:

The PR is here: apache/airflow#25318 and the root cause was that our authentication backend set the user direclty in the context of Flask:

def _set_current_user(user):
    ctx = _request_ctx_stack.top
    ctx.user = user

This was very "low-level" and (as we found out with 0.6.2) really an internal detail of Flask Login. As far as I understand, the 0.6.2 changed the way how user is stored from (apparently deprecated way) to using g proxy

#691

The way I fixed in my PR is by using LoginManager:

def _set_current_user(user):
    current_app.appbuilder.sm.lm._update_request_context_with_user(user=user)

That seem to work in both 0.6.2. and 0.6.1 (and likey before)

Questions:

1. Do I understand it correctly?
2. Is the _update_request_context_with_user(user=user) the right way of setting the user for login manger (if we want to set it via external authentication). This method seem protected (_ ) but I could not find a better way.
3. Aren't you afraid that this might break other users as well ?

Just to add - I know we SHOULD NOT do it this way - but - apparently, we DID (mostly because - I guess - because there was no other clear way to do it when we did - I am guessing now of course - it was. We have some mechanisms to protect our users agains this to an extent (constraint mechanism mentioned above), also we cannot "hard-pin" our dependencies (also explained why yesterday) - but we cannot force it. So for our users the "innoncent" migration from 0.6.1 to 0.6.2 if they do not upgrade to latest "google provider" of airflow will result in broken authentication. It's not a big problem for us (we can manage). Just wondering if it is not a big issue for others.

[maxcountryman]

You used a variable which by name indicated it was part of an internal API--I don't know what you were expecting to happen. This is an erroneous way of consuming the library and flask-login has never made any guarantee that its internals would not change between point releases. That's the whole point of them being internal: they're safe for us to change because that's the contract. It's unfortunate, but this is a user error and there's nothing that can be done about users not respecting the design of the library. We will not make special provisions for misuse.

[potiuk]

Sure. Please don't make it personal.

This is something someone did 2 years ago. Not me. you sound like you are accusing me personally of making the mistake. Which I see is a mistake. And I specifically mentioned i am not asking for a provision of misuse, I am not even sure where you got the idea i am asking for one.

Please, pretty please carefuly read what I wrote and what I am asking for. Please try to be empathetic - I am not trying to accuse you of anything or attacking you or even asking you to change anything.

I really just want to know if the way I did it is good. I am simply not sure. The name of the method also seems to be not the right one at It starts with _ (by name) . So learning from that experience (another person mistake) I want to know if what I did is correct or not, and whether there are other ways that are guaranteed to be working in the future - no more no less.

If you read my comment I did acknowledge it was our collective mistake (not yours), and I did mention that we have ways to cope with it. I just want to know if my approach is correct now. I could not find an answer in the docs.

You seem to exaggerate and react a bit agressively to my polite question.

[maxcountryman]

It's never a good idea to use internal APIs if your goal is stability: you run the same risk.

I'm locking this because I frankly don't see this discussion going in a good direction and this should answer the question.