From 2e9883389a2c83a33f3cdc74ba469ebac8c81c5d Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Thu, 28 Dec 2023 12:51:53 -0800 Subject: [PATCH] oauth improvements. (#895) Previously POST_LOGIN_VIEW was used as a successful authentication redirect - SPA redirects need to be unique and separate so when using SPA and forms things don't get really confusing. A new configuration POST_OAUTH_LOGIN_VIEW was added. Add code to oauthstart to return/redirect if caller already authenticated. Improve documentation around the SPA redirects that have to be defined. closes #884 --- .pre-commit-config.yaml | 4 ++-- CHANGES.rst | 11 +++++++---- docs/conf.py | 2 -- docs/configuration.rst | 35 +++++++++++++++++++++++++++++++---- docs/openapi.yaml | 11 +++++++++-- docs/spa.rst | 8 +++++++- examples/oauth/server/app.py | 2 +- flask_security/core.py | 15 ++++++++------- flask_security/oauth_glue.py | 30 +++++++++++++++++++++++++++--- flask_security/utils.py | 7 ++++++- tests/test_oauthglue.py | 28 +++++++++++++++++++++++++--- tests/test_recoverable.py | 4 ++-- 12 files changed, 125 insertions(+), 32 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4f575d5d..f607f5c8 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,7 +19,7 @@ repos: - id: pyupgrade args: [--py38-plus] - repo: https://github.com/psf/black - rev: 23.12.0 + rev: 23.12.1 hooks: - id: black - repo: https://github.com/pycqa/flake8 @@ -30,7 +30,7 @@ repos: - flake8-bugbear - flake8-implicit-str-concat - repo: https://github.com/Riverside-Healthcare/djLint - rev: v1.34.0 + rev: v1.34.1 hooks: - id: djlint-jinja files: "\\.html" diff --git a/CHANGES.rst b/CHANGES.rst index 85f0fbb6..9d6e2f37 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,8 +8,8 @@ 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. +Among other changes, this continues the process of dis-entangling Flask-Security +from Flask-Login and may require some application changes due to backwards incompatible changes. Features ++++++++ @@ -27,7 +27,8 @@ Fixes - (:pr:`881`) No longer rely on Flask-Login.unauthorized callback. See below for implications. - (:pr:`855`) Improve translations for two-factor method selection (gissimo) - (:pr:`866`) Improve German translations (sr-verde) -- (:pr:`xxx`) Improve method translations for unified signin and two factor. Remove support for Flask-Babelex. +- (:pr:`889`) Improve method translations for unified signin and two factor. Remove support for Flask-Babelex. +- (:issue:`884`) Oauth re-used POST_LOGIN_VIEW which caused confusion. See below for implications. Notes ++++++ @@ -45,6 +46,8 @@ Backwards Compatibility Concerns - Passing in an AnonymousUser class as part of Security initialization has been removed. - The never-public method _get_unauthorized_response has been removed. +- Oauth - a new configuration variable :py:data:`SECURITY_POST_OAUTH_LOGIN_VIEW` was introduced + and it replaces :py:data:`SECURITY_POST_LOGIN_VIEW` in the oauthresponse logic. - 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 @@ -56,7 +59,7 @@ Backwards Compatibility Concerns - 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. + - 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`. diff --git a/docs/conf.py b/docs/conf.py index d2fec979..385a2900 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -96,10 +96,8 @@ nitpicky = True nitpick_ignore = [ - ("py:attr", "LoginManager.unauthorized"), ("py:class", "mongoengine.connection"), ("py:class", "ResponseValue"), - ("py:class", "function"), ("py:class", "AuthenticatorSelectionCriteria"), ("py:class", "UserVerificationRequirement"), ("py:class", "OAuth"), diff --git a/docs/configuration.rst b/docs/configuration.rst index ea43855f..527e1873 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -249,7 +249,17 @@ These configuration keys are used globally across all features. have GET endpoints that validate the passed token and redirect to an action form. For Single-Page-Applications style UIs which need to control their own internal URL routing these redirects need to not contain forms, but contain relevant information as query parameters. - Setting this to ``spa`` will enable that behavior. + Setting this to ``"spa"`` will enable that behavior. + + When this is enabled, the following must also be defined: + + - :py:data:`SECURITY_POST_OAUTH_LOGIN_VIEW` (if :py:data:`SECURITY_OAUTH_ENABLE` is True) + - :py:data:`SECURITY_LOGIN_ERROR_VIEW` + - :py:data:`SECURITY_CONFIRM_ERROR_VIEW` + - :py:data:`SECURITY_POST_CONFIRM_VIEW` + - :py:data:`SECURITY_RESET_ERROR_VIEW` + - :py:data:`SECURITY_RESET_VIEW` + Default: ``None`` which is existing html-style form redirects. @@ -262,6 +272,10 @@ These configuration keys are used globally across all features. Flask application. In order to test redirects, the `netloc` of the redirect URL needs to be rewritten. Setting this to e.g. `localhost:8080` does that. + .. tip:: + Be aware that when this is set, any of the `*_VIEW` configuration variables that are set + to URLs and not endpoints, will be redirected to this host. + Default: ``None``. .. versionadded:: 3.3.0 @@ -882,7 +896,7 @@ Confirmable Specifies the view to redirect to if a confirmation error occurs. This value can be set to a URL or an endpoint name. If this value is ``None``, the user is presented the default view - to resend a confirmation link. In the case of ``SECURITY_REDIRECT_BEHAVIOR`` == ``spa`` + to resend a confirmation link. In the case of ``SECURITY_REDIRECT_BEHAVIOR`` == ``"spa"`` query params in the redirect will contain the error. Default: ``None``. @@ -996,7 +1010,7 @@ Recoverable .. py:data:: SECURITY_RESET_VIEW Specifies the view/URL to redirect to after a GET reset-password link. - This is only valid if :py:data:`SECURITY_REDIRECT_BEHAVIOR` == ``spa``. + This is only valid if :py:data:`SECURITY_REDIRECT_BEHAVIOR` == ``"spa"``. Query params in the redirect will contain the ``token``. Default: ``None``. @@ -1422,7 +1436,7 @@ This feature is DEPRECATED as of 5.0.0. Please use unified signin feature instea * GET on oauthresponse where there was an OAuth protocol error. * GET on oauthresponse where the returned identity isn't registered. - This is only valid if :py:data:`SECURITY_REDIRECT_BEHAVIOR` == ``spa``. + This is only valid if :py:data:`SECURITY_REDIRECT_BEHAVIOR` == ``"spa"``. Query params in the redirect will contain the error. Default: ``None``. @@ -1672,6 +1686,18 @@ Social Oauth Default: ``"/login/oauthresponse"`` +.. py:data:: SECURITY_POST_OAUTH_LOGIN_VIEW + + Specifies the view/URL to redirect to after a successful authentication (login) + using social oauth. + This is only valid if :py:data:`SECURITY_REDIRECT_BEHAVIOR` == ``"spa"``. + Query params in the redirect will contain `identity` and `email`. + + Default: ``None``. + + .. versionadded:: 5.4.0 + + Feature Flags ------------- @@ -1717,6 +1743,7 @@ A list of all URLs and Views: * :py:data:`SECURITY_POST_CONFIRM_VIEW` * :py:data:`SECURITY_POST_RESET_VIEW` * :py:data:`SECURITY_POST_CHANGE_VIEW` +* :py:data:`SECURITY_POST_OAUTH_LOGIN_VIEW` * :py:data:`SECURITY_UNAUTHORIZED_VIEW` * :py:data:`SECURITY_RESET_VIEW` * :py:data:`SECURITY_RESET_ERROR_VIEW` diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 0a15b20f..07a3827b 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1785,7 +1785,14 @@ paths: 302: description: > Redirect to OAuth provider. The redirect URL will pass along, if provided, - the value of the `next` query argument. + the value of the `next` query argument. If the caller is already + authenticated redirect will be to ``SECURITY_POST_LOGIN_VIEW`` + 400: + description: Caller is already authenticated + content: + application/json: + schema: + $ref: "#/components/schemas/DefaultJsonErrorResponse" /login/oauthresponse/{name}: parameters: - name: name @@ -1804,7 +1811,7 @@ paths: headers: Location: description: | - On spa-success: SECURITY_POST_LOGIN_VIEW?identity={identity}&email={email} + On spa-success: SECURITY_POST_OAUTH_ LOGIN_VIEW?identity={identity}&email={email} On spa-oauth-error: SECURITY_LOGIN_ERROR_VIEW?error={msg} diff --git a/docs/spa.rst b/docs/spa.rst index 4fd45562..8972c8bd 100644 --- a/docs/spa.rst +++ b/docs/spa.rst @@ -43,12 +43,14 @@ An example configuration:: SECURITY_REGISTERABLE = True SECURITY_UNIFIED_SIGNIN = True - # These need to be defined to handle redirects + # These need to be defined to handle redirects - these are part of the apps UI # As defined in the API documentation - they will receive the relevant context SECURITY_POST_CONFIRM_VIEW = "/confirmed" SECURITY_CONFIRM_ERROR_VIEW = "/confirm-error" SECURITY_RESET_VIEW = "/reset-password" SECURITY_RESET_ERROR_VIEW = "/reset-password-error" + SECURITY_LOGIN_ERROR_VIEW = "/login-error" + SECURITY_POST_OAUTH_LOGIN_VIEW = "/post-oauth-login" SECURITY_REDIRECT_BEHAVIOR = "spa" # CSRF protection is critical for all session-based browser UIs @@ -79,6 +81,10 @@ The UI might want to run on port 8080. In order to test redirects you need to se SECURITY_REDIRECT_HOST = 'localhost:8080' +.. tip:: + The `logout` endpoint doesn't take a body - be sure to add `content_type="application/json"` + header to your POST("/logout") request so that no redirection is done. + Client side authentication options ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Depending on your SPA architecture and vision you can choose between cookie or token based authentication. diff --git a/examples/oauth/server/app.py b/examples/oauth/server/app.py index bd6244bd..2626e749 100644 --- a/examples/oauth/server/app.py +++ b/examples/oauth/server/app.py @@ -110,7 +110,7 @@ def create_app(): db.init_app(app) Security(app, user_datastore, mail_util_cls=FlashMailUtil) - @app.route("/") + @app.route("/home") @auth_required() def home(): return render_template_string("Hello {{ current_user.email }}") diff --git a/flask_security/core.py b/flask_security/core.py index c1fd491d..e08e3c88 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -183,16 +183,17 @@ "LOGOUT_METHODS": ["GET", "POST"], "POST_LOGIN_VIEW": "/", "POST_LOGOUT_VIEW": "/", - "CONFIRM_ERROR_VIEW": None, - "POST_REGISTER_VIEW": None, - "POST_CONFIRM_VIEW": None, + "LOGIN_ERROR_VIEW": None, # spa + "POST_OAUTH_LOGIN_VIEW": None, # spa + "CONFIRM_ERROR_VIEW": None, # spa + "POST_CONFIRM_VIEW": None, # spa + "RESET_VIEW": None, # spa + "RESET_ERROR_VIEW": None, # spa "POST_RESET_VIEW": None, "POST_CHANGE_VIEW": None, "POST_VERIFY_VIEW": None, + "POST_REGISTER_VIEW": None, "UNAUTHORIZED_VIEW": None, - "RESET_ERROR_VIEW": None, - "RESET_VIEW": None, - "LOGIN_ERROR_VIEW": None, "REQUIRES_CONFIRMATION_ERROR_VIEW": None, "REDIRECT_HOST": None, "REDIRECT_BEHAVIOR": None, @@ -1286,7 +1287,7 @@ def __init__( self.passwordless: bool = False self.webauthn: bool = False - # Redirect URLs + # Redirect URLs (we should stop setting these attributes and use CV) self.login_error_view: str = "" self.post_change_view: str = "" self.post_login_view: str = "" diff --git a/flask_security/oauth_glue.py b/flask_security/oauth_glue.py index 324bc993..514e12ce 100644 --- a/flask_security/oauth_glue.py +++ b/flask_security/oauth_glue.py @@ -22,6 +22,7 @@ pass from flask import abort, after_this_request, redirect, request +from flask_login import current_user from .decorators import unauth_csrf from .proxies import _security @@ -32,6 +33,8 @@ get_message, get_post_login_redirect, get_url, + is_user_authenticated, + json_error_response, propagate_next, slash_url_suffix, url_for_security, @@ -81,18 +84,37 @@ def oauthstart(name: str) -> "ResponseValue": TODO: remember me? """ assert _security.oauthglue + if is_user_authenticated(current_user): + # Just redirect current_user to POST_LOGIN_VIEW. + # For json - return an error. + # This endpoint is POST only. + # This does NOT use get_post_login_redirect() so that it doesn't look at + # 'next' - which can cause infinite redirect loops + # (see test_common::test_authenticated_loop) + if _security._want_json(request): + payload = json_error_response( + errors=get_message("ANONYMOUS_USER_REQUIRED")[0] + ) + return _security._render_json(payload, 400, None, None) + else: + return redirect(get_url(cv("POST_LOGIN_VIEW"))) # we never want to return here or to the redirect location. values = dict(next=request.args.get("next", "/")) return _security.oauthglue.get_redirect(name, **values) def oauthresponse(name: str) -> "ResponseValue": - """Callback from oauth provider - response is provider specific""" + """ + Callback from oauth provider - response is provider specific + Since this is a callback from oauth provider - there is no form, + however the ?next= parameter is returned as we specified in oauthstart + N.B. all responses MUST be redirects. + """ assert _security.oauthglue oauth_provider = _security.oauthglue.oauth_provider(name) if not oauth_provider: # this shouldn't really be able to happen - abort(404) + abort(404) # TODO - redirect... to where? # This parses the Flask Request try: token = oauth_provider.authorize_access_token() @@ -123,7 +145,9 @@ def oauthresponse(name: str) -> "ResponseValue": login_user(user) if _security.redirect_behavior == "spa": return redirect( - get_url(cv("POST_LOGIN_VIEW"), qparams=user.get_redirect_qparams()) + get_url( + cv("POST_OAUTH_LOGIN_VIEW"), qparams=user.get_redirect_qparams() + ) ) return redirect(get_post_login_redirect()) # Seems ok to show identity - the only identity it could be is the callers diff --git a/flask_security/utils.py b/flask_security/utils.py index 7776b913..6f9a8825 100644 --- a/flask_security/utils.py +++ b/flask_security/utils.py @@ -448,6 +448,12 @@ def get_url(endpoint_or_url: str, qparams: t.Optional[t.Dict[str, str]] = None) """Returns a URL if a valid endpoint is found. Otherwise, returns the provided value. + .. warning:: + If an endpoint ISN'T provided, then it is assumed that the URL + is external to Flask and if the spa configuration REDIRECT_HOST + is set will redirect to that host. This could be an issue in + development. + :param endpoint_or_url: The endpoint name or URL to default to :param qparams: additional query params to add to end of url :return: URL @@ -458,7 +464,6 @@ def get_url(endpoint_or_url: str, qparams: t.Optional[t.Dict[str, str]] = None) # This is an external URL (no endpoint defined in app) # For (mostly) testing - allow changing/adding the url - for example # add a different host:port for cases where the UI is running - # add a different host:port for cases where the UI is running # separately. if config_value("REDIRECT_HOST"): url = transform_url( diff --git a/tests/test_oauthglue.py b/tests/test_oauthglue.py index 82542e1a..8b40b920 100644 --- a/tests/test_oauthglue.py +++ b/tests/test_oauthglue.py @@ -17,10 +17,12 @@ from tests.test_utils import ( authenticate, + check_location, get_csrf_token, get_form_action, get_form_input, init_app_with_options, + is_authenticated, logout, setup_tf_sms, ) @@ -213,10 +215,10 @@ def test_tf(app, sqlalchemy_datastore, get_message): @pytest.mark.settings( oauth_enable=True, - redirect_host="localhost:8081", + redirect_host="myui.com:8090", redirect_behavior="spa", login_error_view="/login-error", - post_login_view="/post-login", + post_oauth_login_view="/post-login", csrf_ignore_unauth_endpoints=False, ) @pytest.mark.app_settings(wtf_csrf_enabled=True) @@ -240,7 +242,7 @@ def test_spa(app, sqlalchemy_datastore, get_message): assert response.status_code == 302 split = urlsplit(response.location) - assert "localhost:8081" == split.netloc + assert "myui.com:8090" == split.netloc assert "/post-login" == split.path qparams = dict(parse_qsl(split.query)) assert qparams["email"] == "matt@lp.com" @@ -266,3 +268,23 @@ def test_spa(app, sqlalchemy_datastore, get_message): assert "/login-error" == split.path qparams = dict(parse_qsl(split.query)) assert qparams["error"] == get_message("OAUTH_HANDSHAKE_ERROR").decode() + + +@pytest.mark.settings(oauth_enable=True, post_login_view="/post-login") +def test_already_auth(app, sqlalchemy_datastore, get_message): + headers = {"Accept": "application/json", "Content-Type": "application/json"} + init_app_with_options( + app, sqlalchemy_datastore, **{"security_args": {"oauth": MockOAuth()}} + ) + client = app.test_client() + authenticate(client) + assert is_authenticated(client, get_message) + + # json + response = client.post("/login/oauthstart/github", headers=headers) + assert response.status_code == 400 + + # forms + response = client.post("/login/oauthstart/github", follow_redirects=False) + assert response.status_code == 302 + check_location(app, response.location, "/post-login") diff --git a/tests/test_recoverable.py b/tests/test_recoverable.py index 9e10c211..b0288a83 100644 --- a/tests/test_recoverable.py +++ b/tests/test_recoverable.py @@ -438,7 +438,7 @@ def test_custom_reset_templates(client): @pytest.mark.settings( - redirect_host="localhost:8081", + redirect_host="myui.com:8090", redirect_behavior="spa", reset_view="/reset-redirect", ) @@ -459,7 +459,7 @@ def test_spa_get(app, client): response = client.get("/reset/" + token) assert response.status_code == 302 split = urlsplit(response.headers["Location"]) - assert "localhost:8081" == split.netloc + assert "myui.com:8090" == split.netloc assert "/reset-redirect" == split.path qparams = dict(parse_qsl(split.query)) # we shouldn't be showing PII