remove anonymous user

[davidism]

This is a constant source of confusion for users. They don't understand why is_authenticated is always true for UserMixin even if it's not the logged in user. They get tripped up when they try to access attributes on current_user that aren't available on AnonymousUserMixin.

Whenever I'm writing my own quick login implementation, I set g.user to None if there's no logged in user, and the current_user proxy would be unbound (if current_user would look False).

You can return any object you want from user_loader. If an application needs an anonymous user system, they can return an anonymous user object from user_loader. They'll most likely need that anyway as even anonymous users might take actions that need to be associated together, and a single anonymous user wouldn't work for that.

I think removing this might also make it more obvious that you can return more than one type from user_loader, like AdminUser and RegularUser for example.

I realize this is a larger departure from current behavior than other refactors we've been doing. But I think it's worth doing for a simpler API.

[maxcountryman]

Agree; this wouldn't technically remove any functionality, but it would reduce cognitive overhead. I'm in favor of that. It's probably worth documenting how anonymous users can be implemented via user_loader if we go this route.

[mmiko]

I think this issue is about 3 independent (sub)issues, that might be better to address separately. Let me explain:

1. I have to agree with the fact that having UserMixin and AnonymousUserMixin as two independent classes is a quite unfortunate design. But I'd rather suggest switching AnonymousUserMixin to an AnonymousUser that inherits from the UserMixin for the sake of consistency.

2. Having is_authenticated in UserMixin for an unknown reason by default tied to the is_active property is really counter-intuitive. This should be fixed, or better explained, but this has nothing to do with 1).

3. If users are confused about things, they most likely haven't read the docs, docs are poorly written, or there is indeed a design issue. Removing a built-in rarely helps with the first two cases and in this case does not solve the design issue at hand. And this will definitely not make it any more or any less obvious that you can or should return a UserMixin based class from a user_loader or a request_loader if you have no clue what a Mixin class is and how it should be used.

NB, docs state:

By default, when a user is not actually logged in, current_user is set to an AnonymousUserMixin object.

So users are explicitly told, that this is the default object. If a user is confused, she just haven't read or understood the docs and in a case of a library that handles app security this is a thing that leads to security issues down the road. So maybe it's a lesser evil.

---

All in all, as proposed, the easiest solution to part 3 would be to just not store the AnonymousUserMixin instance as a default logged in user and just keep it as a None. However, as this library relies internally on side effects, it's not easy to estimate what breaks with this change. So I'd gravitate towards fixing the issue within UserMixin. Possibly introducing AnonymousUser based on it, as this will not break code that correctly relies on checking is_anonymous flag, however, if you are checking by type this will break.

Also the documentation does not provide any clues on how to use UserMixin, so introducing a section about how UserMixin works and how it should be used/extended, or at least what API is user required to reimplement would help.

[davidism]

I don't care about breaking changes in this case, because of how much confusion this already causes. I'll deprecate AnonymousUserMixin first if possible, then remove it. I'm all for more docs as well, though.

---

AnonymousUser inheriting from UserMixin doesn't really solve the problem. It's still not intuitive why user.is_authenticated is true for all user objects. It also doesn't fix the issue where current_user.attribute can fail because the anonymous user doesn't have the same attributes as the full user.

[davidism]

I haven't experimented yet, but my current idea to deprecate this will be to have a LOGIN_NO_ANONYMOUS config at first. If enabled, it will cause current_user to be None when not logged in. The is_anonymous and is_authenticated properties should warn something like "These properties are not intuitive and will go away, enable LOGIN_NO_ANONYMOUS and check 'not current_user' instead." Subclassing AnonymousUserMixin can also show a warning.

[jwag956]

I happened to be working getting Flask-Security-Too to be compatible with this change. As a test I set:
login_manager.anonymous_user = lambda: None

and started fixing issues :-)
This was pretty simple and I easily got my many many unit tests to pass. My current idea is to default the next release to do this with a backwards compat configuration.

I never realized that is_authenticated actually looks at is_active - that might cause very subtle bugs.. need to work on that next.

[davidism]

It didn't prior to 0.6. There was an issue, a PR, and then someone pointed out that it's sort of confusing now. I'd really like to simplify things like that, it seems like is_active is something that should be checked or not in user_loader, not automatically checked or even provided by UserMixin.

[grosscol]

I concur about removing AnonymousUser as I don't have a case for a singleton anonymous user. In those instances, I would disable login for the application altogether. E.g.

# auth_routes.py
def authentication_required():
    """
    This serves as the before request function when all routes in blueprint should require auth.
    """
    if request.method == 'OPTIONS':
        return None
    elif current_app.config.get('LOGIN_DISABLED'):
        return None
    elif current_user.is_authenticated:
        return None
    else:
        resp = {'message': 'Authentication required.'}
        return(make_response(jsonify(resp), 401))

# in create_app() of  __init__.py

    # Protect data endpoint blueprints without modifying them with before_request.
    app.before_request_funcs = {
        'variant_routes': [auth_routes.authentication_required],
        'region_routes': [auth_routes.authentication_required],
    }

I assume there are use cases for tracking each user as a transient anonymous user that is only identified by their ip, session hash, or something of the sort. The AnonymousUserMixin as it exists doesn't facilitate those use cases.