[Flask-Middleware/flask-security#791] Fix for token-authentication on session-authentcation-only endpoint

[N247S]

I copied the bit from the _load_user function in the login_manager where the user is obtained from the active session. After that i tried to make it function as close to the _check_http_auth() and _check_token() functions as I could to ensure simular behavior.

From what I could test, it works for this particular issue. It feels a bit less hacky compared to the fs_authn_via flag-check I originally proposed.

[jwag956]

Thanks for the proposal... I like it. One observation is that this will cause 2 database lookups per request - I think that can be solved with a similar 'short-circuit' that I did for token auth - see core.py::_request_loader in core.py::_user_loader.

[N247S]

Do you mean the context_stack bit or the global-vars bit?

I get what you mean with the database stuff, maybe it can check the user_id against the current_user first? I am not sure if the fs_uniquifier is the fixed _'userid property' (or if that can be something else)?

[jwag956]

I was just thinking of replicating: if get_request_attr("fs_authn_via") == "token": return g._login_user

replacing "token" with "session" at the top of _user_loader. It's a bit tricky checking current_user equal to userid - since accessing current_user actually caused a load if it isn't set.

[N247S]

That is true.

Should I do that as a prior check, or as the only check before the identity_changed singal-call?

[jwag956]

I think at the top - just as in _request_loader() - if that is set then the signal should have already been sent.. (we could add that to the test).

[N247S]

Oke, so it got messy to do this only in github, so I cloned it to do it properly. So hopefully it passes the tests (as it did not in my local environment due to missing dependencies & tools).

I had to modify the decorator a bit to make it work properly but it still uses the same 'fast-bail' checks. I added 2 tests, 1 for checking this bug-fix, and a second based on your suggestion which was checking if the identity is loaded correctly.

One thing of concern about the current situation is the flag fs_authn_via which is set in the user_loader. This callback is actually publicly accessible through the login_manager meaning people can call it directly outside the context of session-loading (not recommended I think, but still 'public API').

This flag is currently also be set in auth_required, and could also be set in a new (not yet added) decorated session_auth_required. When it is removed from _user_loader the flag can still be checked as 'not-set-yet' or 'session'. If any user is available in the global-vars its identifier can of course be checked against the sessions '_user_id' meaning that flag should not really matter?

Just for clarity for example:

def _check_session():
    user_id = session.get("_user_id", None)
    if user_id is None:
        return False

    user = None
    if hasattr(g, "_login_user"):
        user = g._login_user
        if user.fs_uniquifier is not user_id:
            user = None
            # what should be done on user mismatch?
            # return False

    if user is None:
        if _security.login_manager._user_callback is None:
            return False
        user = _security.login_manager._user_callback(user_id)

    if user is None or not user.active:
        return False
    return True

One thing I realized writing this is, what is actually done on user mismatch? (e.g. basic-auth > user1; session-auth > user2; token-auth > user3)

[codecov[bot]]

Codecov Report

Patch coverage: 94.44% and no project coverage change.

Comparison is base (0ba3f2d) 97.52% compared to head (191533f) 97.53%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #794 +/- ## ======================================= Coverage 97.52% 97.53% ======================================= Files 34 34 Lines 4453 4464 +11 ======================================= + Hits 4343 4354 +11 Misses 110 110 ``` | [Impacted Files](https://app.codecov.io/gh/Flask-Middleware/flask-security/pull/794?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flask-Middleware) | Coverage Δ | | |---|---|---| | [flask\_security/decorators.py](https://app.codecov.io/gh/Flask-Middleware/flask-security/pull/794?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flask-Middleware#diff-Zmxhc2tfc2VjdXJpdHkvZGVjb3JhdG9ycy5weQ==) | `95.95% <93.75%> (+0.20%)` | :arrow_up: | | [flask\_security/\_\_init\_\_.py](https://app.codecov.io/gh/Flask-Middleware/flask-security/pull/794?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flask-Middleware#diff-Zmxhc2tfc2VjdXJpdHkvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [flask\_security/core.py](https://app.codecov.io/gh/Flask-Middleware/flask-security/pull/794?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flask-Middleware#diff-Zmxhc2tfc2VjdXJpdHkvY29yZS5weQ==) | `98.60% <100.00%> (-0.01%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[N247S]

Seems like all tests were succesfull. Not sure what the last 2 fails mean? It says Success: no issues found in 60 source files, but 5 lines later it says Error: Process completed with exit code 255.. No clue what that means.

Ps. sorry for the incorrect closure of the issue, mistapped on my phone.

[jwag956]

Fixed in pr #813