From 58f93eff48be882790a34dfda7ed2ce5eace71a9 Mon Sep 17 00:00:00 2001 From: Chris Wagner Date: Sat, 22 Jul 2023 15:49:18 -0700 Subject: [PATCH] Change confirmation to default to NOT auto-login. (#820) This makes it consistent with /reset and with OWASP best practices. --- CHANGES.rst | 5 +++- docs/configuration.rst | 12 ++++----- docs/openapi.yaml | 4 +-- docs/spa.rst | 1 - flask_security/core.py | 9 ++++++- flask_security/views.py | 9 +++++-- tests/test_confirmable.py | 48 +++++++++++++++++++----------------- tests/test_unified_signin.py | 40 ++++++++++-------------------- tests/test_utils.py | 14 +++++++++++ 9 files changed, 79 insertions(+), 63 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index cd705e47..16ed7a35 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -24,7 +24,7 @@ Fixes - (:issue:`814`) /reset and /confirm and GENERIC_RESPONSES and additional form args don't mix. - (:issue:`281`) Reset password can be exploited and other OWASP improvements. - (:pr:`817`) Confirmation can be exploited and other OWASP improvements. -- (:pr:`xxx`) Convert to pyproject.toml, build, remove setup. +- (:pr:`819`) Convert to pyproject.toml, build, remove setup. Backwards Compatibility Concerns +++++++++++++++++++++++++++++++++ @@ -51,6 +51,9 @@ Backwards Compatibility Concerns the confirmation process. - Identity information (identity, email) is no longer sent as part of the URL redirect query params. + - The :py:data:`SECURITY_AUTO_LOGIN_AFTER_CONFIRM` configuration variable now defaults to ``False`` - meaning + after a successful email confirmation, the user must still sign in using the usual mechanisms. This is to + align better with OWASP best practices. Setting it to ``True`` will restore prior behavior. - The SECURITY_MSG_CONFIRMATION_EXPIRED message no longer contains the user's identity/email. - The response to GET /reset/ sets the HTTP header `Referrer-Policy` to `no-referrer` as suggested by OWASP. diff --git a/docs/configuration.rst b/docs/configuration.rst index 3f7ae99d..f363c889 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -901,12 +901,13 @@ Confirmable Default: ``None``. .. py:data:: SECURITY_AUTO_LOGIN_AFTER_CONFIRM - If ``False`` then on confirmation the user will be required to login again. + If ``True``, then the user corresponding to the confirmation token will be automatically signed in. + If ``False`` (the default) then the user will be requires to authenticate using the usual mechanism(s). Note that the confirmation token is not valid after being used once. - If ``True``, then the user corresponding to the - confirmation token will be automatically logged in. - Default: ``True``. + Default: ``False``. + + .. deprecated:: 5.3.0 .. py:data:: SECURITY_LOGIN_WITHOUT_CONFIRMATION Specifies if a user may login before confirming their email when @@ -916,8 +917,7 @@ Confirmable .. py:data:: SECURITY_REQUIRES_CONFIRMATION_ERROR_VIEW Specifies a redirect page if the users tries to login, reset password or us-signin with an unconfirmed account. - If an URL endpoint is specified, flashes an error messages and passes user email as an argument. - For us-signin, no argument is specified: it simply flashes the error message and redirects. + If an URL endpoint is specified, flashes an error messages and redirects. Default behavior is to reload the form with an error message without redirecting to an other page. Default: ``None``. diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 2fa19001..2f1405fc 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -584,8 +584,8 @@ paths: This is the result of getting a confirmation token and is usually the result of clicking the link from a confirmation email. This ALWAYS results in a 302 redirect. - By default (unless __SECURITY_AUTO_LOGIN_AFTER_CONFIRM__ == False), the user - denoted by the token is logged in as a side-effect. + By default (unless __SECURITY_AUTO_LOGIN_AFTER_CONFIRM__ == True), the user + denoted by the token must authenticate using normal mechanisms. responses: 302: description: > diff --git a/docs/spa.rst b/docs/spa.rst index 34402c8c..4fd45562 100644 --- a/docs/spa.rst +++ b/docs/spa.rst @@ -42,7 +42,6 @@ An example configuration:: SECURITY_CONFIRMABLE = True SECURITY_REGISTERABLE = True SECURITY_UNIFIED_SIGNIN = True - SECURITY_WEBAUTHN = True # These need to be defined to handle redirects # As defined in the API documentation - they will receive the relevant context diff --git a/flask_security/core.py b/flask_security/core.py index acc2c295..5f0d18fe 100644 --- a/flask_security/core.py +++ b/flask_security/core.py @@ -248,7 +248,7 @@ "CONFIRM_EMAIL_WITHIN": "5 days", "RESET_PASSWORD_WITHIN": "1 days", "LOGIN_WITHOUT_CONFIRMATION": False, - "AUTO_LOGIN_AFTER_CONFIRM": True, + "AUTO_LOGIN_AFTER_CONFIRM": False, "AUTO_LOGIN_AFTER_RESET": False, "EMAIL_SENDER": LocalProxy( lambda: current_app.config.get("MAIL_DEFAULT_SENDER", "no-reply@localhost") @@ -1457,6 +1457,13 @@ def init_app( DeprecationWarning, stacklevel=2, ) + if cv("AUTO_LOGIN_AFTER_CONFIRM", app=app): + warnings.warn( + "The auto-login after successful confirmation functionality" + "has been deprecated and will be removed in a future release.", + DeprecationWarning, + stacklevel=2, + ) if cv("USERNAME_ENABLE", app): if hasattr(self.datastore, "user_model") and not hasattr( self.datastore.user_model, "username" diff --git a/flask_security/views.py b/flask_security/views.py index f83ab2a0..eef481d9 100644 --- a/flask_security/views.py +++ b/flask_security/views.py @@ -433,7 +433,10 @@ def send_confirmation(): def confirm_email(token): - """View function which handles an email confirmation request.""" + """ + View function which handles an email confirmation request. + This is always a GET from an email - so for 'spa' must always redirect. + """ expired, invalid, user = confirm_email_token_status(token) @@ -454,7 +457,6 @@ def confirm_email(token): ) already_confirmed = user.confirmed_at is not None - if already_confirmed: m, c = get_message("ALREADY_CONFIRMED") @@ -483,6 +485,8 @@ def confirm_email(token): after_this_request(view_commit) m, c = get_message("EMAIL_CONFIRMED") + # ? The only case where user is logged in already would be if + # LOGIN_WITHOUT_CONFIRMATION if user != current_user: logout_user() if cv("AUTO_LOGIN_AFTER_CONFIRM"): @@ -490,6 +494,7 @@ def confirm_email(token): # and you have the LOGIN_WITHOUT_CONFIRMATION flag since in that case # you can be logged in and doing stuff - but another person could # get the email. + # Note also this goes against OWASP recommendations. response = _security.two_factor_plugins.tf_enter( user, False, "confirm", next_loc=propagate_next(request.url) ) diff --git a/tests/test_confirmable.py b/tests/test_confirmable.py index f42f5bc2..82dfb525 100644 --- a/tests/test_confirmable.py +++ b/tests/test_confirmable.py @@ -22,6 +22,7 @@ authenticate, capture_flashes, capture_registrations, + is_authenticated, logout, populate_data, ) @@ -91,6 +92,8 @@ def on_instructions_sent(app, **kwargs): assert response.headers.get("Referrer-Policy", None) == "no-referrer" response = clients.get(response.location) assert get_message("EMAIL_CONFIRMED") in response.data + # make sure not logged in + assert not is_authenticated(clients, get_message) # Test already confirmed response = clients.get("/confirm/" + token, follow_redirects=True) @@ -98,8 +101,6 @@ def on_instructions_sent(app, **kwargs): assert len(recorded_instructions_sent) == 2 # Test already confirmed when asking for confirmation instructions - logout(clients) - response = clients.get("/confirm") assert response.status_code == 200 @@ -290,25 +291,34 @@ def test_confirmation_different_user_when_logged_in_no_auto(client, get_message) @pytest.mark.registerable() @pytest.mark.settings(login_without_confirmation=True) def test_confirmation_different_user_when_logged_in(client, get_message): + # with default no-auto-login - first user should get logged out and second + # should be properly confirmed (but not logged in) e1 = "dude@lp.com" e2 = "lady@lp.com" with capture_registrations() as registrations: for e in e1, e2: data = dict(email=e, password="awesome sunset", next="") - client.post("/register", data=data) + response = client.post("/register", data=data) + assert is_authenticated(client, get_message) logout(client) token1 = registrations[0]["confirm_token"] token2 = registrations[1]["confirm_token"] - client.get("/confirm/" + token1, follow_redirects=True) - logout(client) - authenticate(client, email=e1) + response = client.get("/confirm/" + token1, follow_redirects=False) + assert "/login" in response.location + authenticate(client, email=e1, password="awesome sunset") + assert is_authenticated(client, get_message) response = client.get("/confirm/" + token2, follow_redirects=True) assert get_message("EMAIL_CONFIRMED") in response.data - assert b"Welcome lady@lp.com" in response.data + + # first user should have been logged out + assert not is_authenticated(client, get_message) + + authenticate(client, email=e2, password="awesome sunset") + assert is_authenticated(client, get_message) @pytest.mark.registerable() @@ -451,10 +461,11 @@ def test_spa_get_bad_token(app, client, get_message): assert len(flashes) == 1 +@pytest.mark.filterwarnings("ignore") @pytest.mark.two_factor() @pytest.mark.registerable() -@pytest.mark.settings(two_factor_required=True) -def test_two_factor(app, client): +@pytest.mark.settings(two_factor_required=True, auto_login_after_confirm=True) +def test_two_factor(app, client, get_message): """If two-factor is enabled, the confirm shouldn't login, but start the 2-factor setup. """ @@ -462,19 +473,17 @@ def test_two_factor(app, client): data = dict(email="mary@lp.com", password="password", next="") client.post("/register", data=data, follow_redirects=True) - # make sure not logged in - response = client.get("/profile") - assert response.status_code == 302 - assert "/login?next=%2Fprofile" in response.location + assert not is_authenticated(client, get_message) token = registrations[0]["confirm_token"] response = client.get("/confirm/" + token, follow_redirects=False) assert "tf-setup" in response.location +@pytest.mark.filterwarnings("ignore") @pytest.mark.two_factor() @pytest.mark.registerable() -@pytest.mark.settings(two_factor_required=True) +@pytest.mark.settings(two_factor_required=True, auto_login_after_confirm=True) def test_two_factor_json(app, client, get_message): with capture_registrations() as registrations: data = dict(email="dude@lp.com", password="password") @@ -484,12 +493,7 @@ def test_two_factor_json(app, client, get_message): assert len(response.json["response"]) == 2 assert all(k in response.json["response"] for k in ["csrf_token", "user"]) - # make sure not logged in - response = client.get("/profile", headers={"accept": "application/json"}) - assert response.status_code == 401 - assert response.json["response"]["errors"][0].encode("utf-8") == get_message( - "UNAUTHENTICATED" - ) + assert not is_authenticated(client, get_message) token = registrations[0]["confirm_token"] response = client.get("/confirm/" + token, headers={"Accept": "application/json"}) @@ -514,9 +518,7 @@ def test_email_not_identity(app, client, get_message): token = registrations[0]["confirm_token"] response = client.get("/confirm/" + token, headers={"Accept": "application/json"}) assert response.status_code == 302 - assert "/" in response.location - - logout(client) + assert not is_authenticated(client, get_message) # check that username must be unique data = dict(email="mary4@lp.com", username="mary", password="awesome sunset") diff --git a/tests/test_unified_signin.py b/tests/test_unified_signin.py index 13ce2a3d..f1cdfda9 100644 --- a/tests/test_unified_signin.py +++ b/tests/test_unified_signin.py @@ -26,6 +26,7 @@ capture_flashes, capture_reset_password_requests, get_form_action, + is_authenticated, logout, reset_fresh, setup_tf_sms, @@ -203,12 +204,10 @@ def authned(myapp, user, **extra_args): assert not clients.get_cookie("remember_token") assert "email" in auths[0][1] - response = clients.get("/profile", follow_redirects=False) - assert response.status_code == 200 + assert is_authenticated(clients, get_message) logout(clients) - response = clients.get("/profile", follow_redirects=False) - assert "/login?next=%2Fprofile" in response.location + assert not is_authenticated(clients, get_message) # login via SMS sms_sender = SmsSenderFactory.createSender("test") @@ -231,8 +230,7 @@ def authned(myapp, user, **extra_args): assert clients.get_cookie("remember_token") assert "sms" in auths[1][1] - response = clients.get("/profile", follow_redirects=False) - assert response.status_code == 200 + assert is_authenticated(clients, get_message) logout(clients) assert not clients.get_cookie("remember_token") @@ -294,8 +292,7 @@ def authned(myapp, user, **extra_args): assert "email" in auths[0][1] logout(client_nc) - response = client_nc.get("/profile", headers=headers, follow_redirects=False) - assert response.status_code == 401 + assert not is_authenticated(client_nc, get_message) # login via SMS sms_sender = SmsSenderFactory.createSender("test") @@ -459,9 +456,8 @@ def test_us_passwordless_confirm_json(app, client, get_message): outbox = app.mail.outbox matcher = re.findall(r"\w+:.*", outbox[0].body, re.IGNORECASE) link = matcher[0].split(":", 1)[1] - response = client.get(link, headers=headers, follow_redirects=True) - assert get_message("EMAIL_CONFIRMED") in response.data - logout(client) + response = client.get(link, headers=headers, follow_redirects=False) + assert response.location == "/login" # should be able to authenticate now. response = client.post( @@ -644,8 +640,7 @@ def authned(myapp, user, **extra_args): assert "email" in auths[0][1] # verify logged in - response = client.get("/profile", follow_redirects=False) - assert response.status_code == 200 + assert is_authenticated(client, get_message) @pytest.mark.settings( @@ -712,8 +707,7 @@ def test_verify_link_spa(app, client, get_message): qparams = dict(parse_qsl(split.query)) assert qparams["email"] == "matt@lp.com" - response = client.get("/profile", headers=headers) - assert response.status_code == 200 + assert is_authenticated(client, get_message) def test_setup(app, client, get_message): @@ -1308,8 +1302,7 @@ def test_confirmable(app, client, get_message): assert get_message("CONFIRMATION_REQUIRED") in response.data # Verify not authenticated - response = client.get("/profile", follow_redirects=False) - assert "/login?next=%2Fprofile" in response.location + assert not is_authenticated(client, get_message) @pytest.mark.registerable() @@ -1443,9 +1436,7 @@ def authned(myapp, user, **extra_args): assert response.status_code == 200 assert client.get_cookie("remember_token") assert "password" in auths[0][1] - - response = client.get("/profile", follow_redirects=False) - assert response.status_code == 200 + assert is_authenticated(client, get_message) @pytest.mark.settings( @@ -1591,8 +1582,7 @@ def test_tf_not(app, client, get_message): # assert "sms" in auths[1][1] # Verify authenticated - response = client.get("/profile", follow_redirects=False) - assert response.status_code == 200 + assert is_authenticated(client, get_message) @pytest.mark.settings(sms_service="bad") @@ -1840,11 +1830,7 @@ def test_us_tf_validity(app, client, get_message): ) assert response.status_code == 200 # verify logged in - response = client.get("/profile", follow_redirects=False) - - assert response.status_code == 200 - assert b"Welcome gal@lp.com" in response.data - + assert is_authenticated(client, get_message) logout(client) data["identity"] = "gal2@lp.com" diff --git a/tests/test_utils.py b/tests/test_utils.py index 3a045c9b..fe046e66 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -49,6 +49,20 @@ def json_authenticate(client, email="matt@lp.com", password="password", endpoint return client.post(ep, content_type="application/json", data=data) +def is_authenticated(client, get_message): + # Return True is 'client' is authenticated. + # Return False is not + # Raise ValueError not certain... + response = client.get("/profile", headers={"accept": "application/json"}) + if response.status_code == 200: + return True + if response.status_code == 401 and response.json["response"]["errors"][0].encode( + "utf-8" + ) == get_message("UNAUTHENTICATED"): + return False + raise ValueError("Failed to figure out if authenticated") + + def verify_token(client_nc, token, status=None): # Use passed auth token in API that requires auth and verify status. # Pass in a client_nc to get valid results.