Azure-Samples / ms-identity-python-webapp

A Python web application calling Microsoft graph that is secured using the Microsoft identity platform
MIT License
279 stars 133 forks source link

environment specific state mismatch error #94

Closed loopasam closed 1 year ago

loopasam commented 1 year ago

Hi, I'm facing a behaviour (bug?) that I have trouble to diagnose, I'll appreciate any pointers.

Setting:

Unexpected behaviour

In summary, users can login on our app, but they first have to log into the sample app. Both app are exposed sequentially at the same address

rayluo commented 1 year ago

The symptom sounds like your own app did not store sessions.

I copy pasted the exact code into our own internal Flask app (with minor modification)

This sample is (designed to be) a minimalist app that contains few lines of code, but then each line (sans comments, but including template and configuration) is required. Perhaps worth double checking. In particular, did you enable the server-side session?

loopasam commented 1 year ago

Thanks for the answer and the pointer. I do store the session on the file system, but I've just observed that I store 2 times as much as the sample app (4 files as opposed to the expected two, one for the auth_code_flow and one for the ID).

This likely points to a problem deeper on our web app, I'll investigate.

rayluo commented 1 year ago

Having more session files on the file system is not necessarily a problem. Flask-session keeps old session around. But if you clean up all those session files, run your app, and no new session file is created/updated after the login() function, that would indicate an issue somewhere.

loopasam commented 1 year ago

I think that the problem is actually coming from the bit of code I modified from the sample (surprise surprise).

I added this functionality, in order to protect all routes, except the "login" related ones:

@app.before_request
def check_login():
    if request.endpoint not in ['login', 'get_token', 'logout']:
        if not auth.get_user():
            return redirect(url_for('login'))

Once commented out, it seems to work as expected, not sure exactly about the root cause

rayluo commented 1 year ago

Actually, I like your @app.before_request idea, and we may even add it into our sample in a future commit.

Back to our topic, I added it into this sample and tested it, it still works.

That @app.before_request snippet is OK in itself, but could it be possible that your app has some Double Redirect behavior? Each time the login() is visited, a new state will be implicitly created, thus wiping out the previous state.

loopasam commented 1 year ago

I think that I've identified the source of the issue: A race condition was caused by either the web browser (or the middleware) requesting the favicon (/favicon.ico) when making the initial request for / (= index()). Both request were redirected to login() trying to then authentificate the user, resulting in a token clash.

It's inline with inconsistent behaviour accross web browsers and systems: Depending whether the favicon is already cached or not, it would be requested or not, causing the clash.

As you mentionned, the @app.before_request itself is actually fine, yet one has to handle the favicon case. It could look like that:

@app.before_request
def check_login():
    if request.endpoint not in ['login', 'get_token', 'logout']:
        if request.path != '/favicon.ico':
            if not auth.get_user():
                return redirect(url_for('login'))

get_token: Indeed coming from the previous sample (/getAToken), now I'm using the latest sample app (using identity)

More generally, it could indeed be useful to provide an example pattern on how to protect all routes within an app

rayluo commented 1 year ago

Thanks for sharing your investigation result, Samuel! No wonder. That favicon race condition would indeed effectively cause the Double Redirect.

In such a case, the if request.path != '/favicon.ico': is flimsy. What if an app also serves a bunch of other images, css files, etc. on its index page?

You will probably have to do it the other way around, to somehow create a @requires_login decorator and explicitly uses it on each endpoint that would require login.

loopasam commented 1 year ago

I agree that the favicon filtering is not ideal, but as the request is not coming from the Flask app, it is a reasonable work-around for us now. In our case, we don't want to expose any routes (not even the static files), a decorator is too risky (= if you forget to add it, the route is exposed). Thanks for the help!