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

@@ -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"

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`.

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"),

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`

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}
 

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.

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 }}")

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 = ""

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

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(

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")