Skip to content

Commit

Permalink
Invert unauthenticated handling. (#881)
Browse files Browse the repository at this point in the history
Flask-Security used to Flask-Login's unauthorized handler - now all that logic is in Flask-Security and Flask-Login will call our _unauthn_handler as part of @login_required.

Added new signal 'user-unauthenticated' to replace Flask-Login user-unauthorized signal.

The CHANGES.rst lists all the minor backwards compatibility concerns.

Many of the test changes resulted from Flask-Login using urlencode for 'next' query parameters - whereas Werkzeug uses quote which doesn't escape '/'s.

Also - now that our minimum Flask is higher - we can once again do assert 'equals' for response.location
  • Loading branch information
jwag956 authored Nov 16, 2023
1 parent 2a234e2 commit a34c3b0
Show file tree
Hide file tree
Showing 21 changed files with 167 additions and 66 deletions.
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ repos:
- id: pyupgrade
args: [--py38-plus]
- repo: https://github.com/psf/black
rev: 23.10.1
rev: 23.11.0
hooks:
- id: black
- repo: https://github.com/pycqa/flake8
Expand Down
29 changes: 26 additions & 3 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ Version 5.4.0

Released xxx

Some of these changes continue the process of dis-entangling Flask-Security
from Flask-Login and have possible backwards compatibility issues.

Fixes
+++++

- (:issue:`845`) us-signin magic link should use fs_uniquifier (not email)
- (:pr:`873`) Update Spanish and Italian translations (gissimo)
- (:pr:`877`) Make AnonymousUser optional and deprecated
- (:issue:`875`) user_datastore.create_user has side effects on mutable inputs (NoRePercussions)
- (:pr:`xxx`) The long deprecated _unauthorized_callback/handler handler has been removed.
- (:pr:`878`) The long deprecated _unauthorized_callback/handler has been removed.
- (:pr:`xxx`) No longer rely on Flask-Login.unauthorized callback. See below for implications.

Notes
++++++
- Prior to this release, the **current_user** proxy always pointed to a user object.
- Historically, the **current_user** proxy (managed by Flask-Login) always pointed to a user object.
If the user wasn't authenticated, it pointed to an AnonymousUser object. With this release,
setting :py:data:`SECURITY_ANONYMOUS_USER_DISABLED` to `True` will force **current_user** to be set
to `None` if the requesting user isn't authenticated. It should be noted that this is in support
Expand All @@ -32,7 +36,26 @@ Backwards Compatibility Concerns
+++++++++++++++++++++++++++++++++

- Passing in an AnonymousUser class as part of Security initialization has been removed.
- The never-public method _get_unauthorized_response method has been removed.
- The never-public method _get_unauthorized_response has been removed.
- Bring unauthenticated handling completely into Flask-Security:
Prior to this release, Flask-Security's :meth:`.Security.unauthn_handler` - called when
a request wasn't properly authenticated - handled JSON requests then delegated
form responses to Flask-Login's unauthenticated_callback. That logic has been moved
into Flask-Security and Flask-Login is configured to call back into Flask-Security's
handler. While the logic is very similar the following differences might be observed:

- Flask-Login's FORCE_HOST_FOR_REDIRECTS configuration isn't honored
- Flask-Login's USE_SESSION_FOR_NEXT configuration isn't honored
- The flashed message is SECURITY_MSG_UNAUTHENTICATED rather than SECURITY_MSG_LOGIN.
Furthermore SECURITY_MSG_UNAUTHENTICATED was reworded to read better.
- Flask-Login uses urlencode to encode the `next` query param - which quotes the '/' character.
Werkzeug (which Flask-Security uses to build the URL) uses `quote`
which considers '/' a safe character and isn't encoded.
- The signal sent on an unauthenticated request has changed to :data:`user_unauthenticated`.
Flask-Login used to send a `user_unauthorized` signal.

- Flask-Security no longer configures anything related to Flask-Login's `fresh_login` logic.
This shouldn't be used - instead use Flask-Security's :meth:`flask_security.auth_required` decorator.


Version 5.3.2
Expand Down
11 changes: 8 additions & 3 deletions docs/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ Protecting Views

.. autofunction:: flask_security.auth_required

.. autofunction:: flask_security.login_required

.. autofunction:: flask_security.roles_required

.. autofunction:: flask_security.roles_accepted
Expand Down Expand Up @@ -256,6 +254,13 @@ sends the following signals.

.. versionadded:: 3.4.0

.. data:: user_unauthenticated

Sent when a user fails to authenticate. It is sent from the `default_unauthn_handler`.
It is passed the app (which is the sender).

.. versionadded:: 5.4.0

.. data:: user_registered

Sent when a user registers on the site. In addition to the app (which is the
Expand Down Expand Up @@ -369,4 +374,4 @@ sends the following signals.

.. versionadded:: 5.0.0

.. _Flask documentation on signals: https://flask.palletsprojects.com/en/2.0.x/signals/
.. _Flask documentation on signals: https://flask.palletsprojects.com/en/2.3.x/signals/
10 changes: 5 additions & 5 deletions docs/customizing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -537,9 +537,9 @@ tandem with Flask-Login, behaves as follows:

* If authentication fails as the result of a `@login_required`, `@auth_required("session", "token")`,
or `@token_auth_required` then if the request 'wants' a JSON
response, :meth:`.Security.render_json` is called with a 401 status code. If not
then flask_login.LoginManager.unauthorized() is called. By default THAT will redirect to
a login view.
response, :meth:`.Security.render_json` is called with a 401 status code.
If not then the `SECURITY_MSG_UNAUTHENTICATED` message is flashed and the request is
redirected to the `login` view.

* If authentication fails as the result of a `@http_auth_required` or `@auth_required("basic")`
then a 401 is returned along with the http header ``WWW-Authenticate`` set to
Expand Down Expand Up @@ -575,12 +575,12 @@ any `next` query param and in fact, an existing `?next=/xx` will override most o
As a complex example consider an unauthenticated user accessing a `@auth_required` endpoint, and the user has
two-factor authentication set up.:

* GET("/protected") - The `default_unauthn_handler` via Flask-Login will redirect to ``/login?next=/protected``
* GET("/protected") - The `default_unauthn_handler` will redirect to ``/login?next=/protected``
* The login form/template will pick any `?next=/xx` argument off the request URL and append it to form action.
* When the form is submitted if will do a POST("/login?next=/protected")
* Assuming correct authentication, the system will send out a 2-factor code and redirect to ``/tf-verify?next=/protected``
* The two_factor_validation_form/template also pulls any `?next=/xx` and appends to the form action.
* When the `tf-validate` form is submitted it will do a POST("/tf-validate?next=/protected").
* Assuming a correct code, the user is authenticated and is redirected. That redirection first
looks for a 'next' in the request.args then in request.form and finally will use the value of `SECURITY_POST_LOGIN_VIEW`.
looks for a 'next' in the request.args then in request.form and finally will use the value of :py:data:`SECURITY_POST_LOGIN_VIEW`.
In this example it will find the ``next=/protected`` in the request.args and redirect to ``/protected``.
1 change: 1 addition & 0 deletions flask_security/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
tf_security_token_sent,
tf_disabled,
user_authenticated,
user_unauthenticated,
user_confirmed,
user_registered,
user_not_registered,
Expand Down
32 changes: 16 additions & 16 deletions flask_security/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@
get_message,
get_request_attr,
is_user_authenticated,
localize_callback,
set_request_attr,
uia_email_mapper,
uia_username_mapper,
Expand Down Expand Up @@ -387,7 +386,7 @@
),
"UNAUTHORIZED": (_("You do not have permission to view this resource."), "error"),
"UNAUTHENTICATED": (
_("You are not authenticated. Please supply the correct credentials."),
_("You must sign in to view this resource."),
"error",
),
"REAUTHENTICATION_REQUIRED": (
Expand Down Expand Up @@ -713,7 +712,7 @@ def _on_identity_loaded(sender, identity):
identity.user = current_user


def _get_login_manager(app):
def _get_login_manager(app, security):
lm = LoginManager()
# Flask-Login is likely going in the direction of removing AnonymousUser
# however this might wreak havoc on applications that just assume that
Expand All @@ -723,19 +722,16 @@ def _get_login_manager(app):
else:
lm.anonymous_user = AnonymousUser

lm.localize_callback = localize_callback
lm.login_view = f'{cv("BLUEPRINT_NAME", app=app)}.login'
lm.user_loader(_user_loader)
lm.request_loader(_request_loader)
# Set Flask-Login handler so @login_required will have same behavior as
# @auth_required
lm.unauthorized_callback = security._unauthn_handler

if cv("FLASH_MESSAGES", app=app):
lm.login_message, lm.login_message_category = cv("MSG_LOGIN", app=app)
lm.needs_refresh_message, lm.needs_refresh_message_category = cv(
"MSG_REFRESH", app=app
)
else:
lm.login_message = None
lm.needs_refresh_message = None
# Note: since we redirect unauthenticated requests to us - we no longer need to
# mess with Flask-Login login_view, or message settings.
# We also (5.4.0) stop doing anything with need_fresh_login - we support time-based
# freshness.

lm.init_app(app)
return lm
Expand Down Expand Up @@ -1425,7 +1421,7 @@ def init_app(
" must have one and only one key."
)

self.login_manager = _get_login_manager(app)
self.login_manager = _get_login_manager(app, self)
self._phone_util = self.phone_util_cls(app)
self._mail_util = self.mail_util_cls(app)
self._password_util = self.password_util_cls(app)
Expand Down Expand Up @@ -1841,8 +1837,9 @@ def unauthn_handler(
Callback for failed authentication.
This is called by :func:`auth_required`, :func:`auth_token_required`
or :func:`http_auth_required` if authentication fails.
It is also called from Flask-Login's @login_required decorator.
:param cb: Callback function with signature (mechanisms, headers=None)
:param cb: Callback function with signature (mechanisms=None, headers=None)
:mechanisms: List of which authentication mechanisms were tried
:headers: dict of headers to return
Expand All @@ -1852,9 +1849,12 @@ def unauthn_handler(
``flask.errorhandler(<exception>)``
The default implementation will return a 401 response if the request was JSON,
otherwise lets ``flask_login.login_manager.unauthorized()`` handle redirects.
otherwise will redirect to the `login` view.
.. versionadded:: 3.3.0
.. versionchanged:: 5.4.0
No longer calls Flask-Login and has complete logic built-in.
"""
self._unauthn_handler = cb

Expand Down
26 changes: 18 additions & 8 deletions flask_security/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from werkzeug.routing import BuildError

from .proxies import _security, DecoratedView
from .signals import user_unauthenticated
from .utils import (
FsPermNeed,
config_value,
Expand All @@ -33,6 +34,7 @@
check_and_update_authn_fresh,
json_error_response,
set_request_attr,
simplify_url,
get_request_attr,
url_for_security,
)
Expand All @@ -59,30 +61,35 @@ def _get_unauthenticated_response(text=None, headers=None):
return Response(text, 401, headers)


def default_unauthn_handler(mechanisms, headers=None):
def default_unauthn_handler(mechanisms=None, headers=None):
"""Default callback for failures to authenticate
If caller wants JSON - return 401.
If caller wants BasicAuth - return 401 (the WWW-Authenticate header is set).
Otherwise - assume caller is html and redirect if possible to a login view.
We let Flask-Login handle this.
"""
user_unauthenticated.send(current_app._get_current_object()) # type: ignore
headers = headers or {}
msg = get_message("UNAUTHENTICATED")[0]
m, c = get_message("UNAUTHENTICATED")

if config_value("BACKWARDS_COMPAT_UNAUTHN"):
return _get_unauthenticated_response(headers=headers)
if _security._want_json(request):
payload = json_error_response(errors=msg)
payload = json_error_response(errors=m)
return _security._render_json(payload, 401, headers, None)

# Basic-Auth is often used to provide a browser based login form and then the
# browser will always add the BasicAuth credentials. For that to work we need to
# return 401 and not redirect to our login view.
if "WWW-Authenticate" in headers:
return Response(msg, 401, headers)
return _security.login_manager.unauthorized()
return Response(m, 401, headers)

do_flash(m, c)
# Simplify the original URL to be relative (if possible) and set as 'next' parameter
login_url = url_for_security("login", _external=True)
next_url = simplify_url(login_url, request.url)
redirect_url = url_for_security("login", next=next_url)
return redirect(redirect_url)


def default_reauthn_handler(within, grace):
Expand All @@ -108,7 +115,10 @@ def default_reauthn_handler(within, grace):

view = "us_verify" if config_value("UNIFIED_SIGNIN") else "verify"
do_flash(m, c)
redirect_url = url_for_security(view, next=request.url)
# Simplify the original URL to be relative (if possible) and set as 'next' parameter
view_url = url_for_security(view, _external=True)
next_url = simplify_url(view_url, request.url)
redirect_url = url_for_security(view, next=next_url)
return redirect(redirect_url)


Expand Down
2 changes: 2 additions & 0 deletions flask_security/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

user_authenticated = signals.signal("user-authenticated")

user_unauthenticated = signals.signal("user-unauthenticated")

user_registered = signals.signal("user-registered")

# For cases of RETURN_GENERIC_RESPONSES with existing email/username
Expand Down
20 changes: 20 additions & 0 deletions flask_security/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,26 @@ def propagate_next(
raise ValueError("No valid redirect URL found - configuration error")


def simplify_url(base_url: str, redirect_url: str) -> str:
"""
Reduces the scheme and host from the redirect_url so it can be passed
as a relative URL in a query (e.g. next) param.
For this method we aren't worrying about a valid url (e.g. if it points
externally) - that will be handled by later requests.
:param base_url: The URL to simplify 'against'.
:param redirect_url: The URL to reduce.
"""
b_url = urlsplit(base_url)
r_url = urlsplit(redirect_url)

if (not r_url.scheme or r_url.scheme == b_url.scheme) and (
not r_url.netloc or r_url.netloc == b_url.netloc
):
return urlunsplit(("", "", r_url.path, r_url.query, r_url.fragment))
return redirect_url


def get_config(app: "Flask") -> t.Dict[str, t.Any]:
"""Conveniently get the security configuration for the specified
application without the annoying 'SECURITY_' prefix.
Expand Down
3 changes: 1 addition & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
auth_token_required,
http_auth_required,
get_request_attr,
login_required,
roles_accepted,
roles_required,
permissions_accepted,
Expand Down Expand Up @@ -188,7 +187,7 @@ def profile():
return render_template("index.html", content="Profile Page")

@app.route("/post_login")
@login_required
@auth_required()
def post_login():
return render_template("index.html", content="Post Login")

Expand Down
6 changes: 3 additions & 3 deletions tests/test_changeable.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def authned(myapp, user, **extra_args):
# try to access protected endpoint - shouldn't work
response = client.get("/profile")
assert response.status_code == 302
assert "/login?next=%2Fprofile" in response.location
assert response.location == "/login?next=/profile"


def test_change_updates_remember(app, client):
Expand Down Expand Up @@ -253,7 +253,7 @@ def test_change_invalidates_auth_token(app, client):
# authtoken should now be invalid
response = client.get("/token", headers=headers)
assert response.status_code == 302
assert "/login?next=%2Ftoken" in response.location
assert response.location == "/login?next=/token"


def test_auth_uniquifier(app):
Expand Down Expand Up @@ -432,7 +432,7 @@ def test_basic_change(app, client_nc, get_message):
new_password_confirm="new strong password",
)
response = client_nc.post("/change", data=data)
assert b"You are not authenticated" in response.data
assert get_message("UNAUTHENTICATED") in response.data
assert "WWW-Authenticate" in response.headers

response = client_nc.post(
Expand Down
Loading

0 comments on commit a34c3b0

Please sign in to comment.