Open davidism opened 2 years ago
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.
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.
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.
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.
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.
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
.
This is a constant source of confusion for users. They don't understand why
is_authenticated
is always true forUserMixin
even if it's not the logged in user. They get tripped up when they try to access attributes oncurrent_user
that aren't available onAnonymousUserMixin
.Whenever I'm writing my own quick login implementation, I set
g.user
toNone
if there's no logged in user, and thecurrent_user
proxy would be unbound (if current_user
would lookFalse
).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 fromuser_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
, likeAdminUser
andRegularUser
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.