From 5dd08b3bd651b2035fbbbe69a20fed2edba5100d Mon Sep 17 00:00:00 2001 From: MarkLark86 Date: Thu, 31 Aug 2023 09:59:49 +1000 Subject: [PATCH] [CPNHUB-248] Multi auth handling changes (#128) * Send PasswordReset to server if user not found in firebase * Set company auth_provider from mgmt_api * Dont send CEM updates for non google accounts * Add AUTH_PROVIDERS for Google & Azure * Fix email_sent check used while developing * fix(build): Upgrade to latest eve-elastic * add/fix tests * fix lint * remove example test * fix(ci): Improve npm install times --- .github/workflows/tests.yml | 8 ++++-- client/src/reset-password.js | 12 ++++++-- server/cp/auth.py | 44 ++++++++++++++++++++++++++--- server/cp/mgmt_api_signals.py | 9 ++++++ server/cp/password_reset_form.py | 9 ++++++ server/cp/signals.py | 29 +++++++++++++++---- server/requirements.txt | 32 ++++++++++++--------- server/settings.py | 12 ++++++++ server/settings_mgmtapi.py | 4 +++ server/tests/test_example.py | 5 ---- server/tests/test_signals.py | 44 ++++++++++++++++++++++++++++- server/theme/cp_reset_password.html | 8 ++++-- server/theme/swagger.yaml | 6 ++++ 13 files changed, 185 insertions(+), 37 deletions(-) create mode 100644 server/cp/mgmt_api_signals.py create mode 100644 server/cp/password_reset_form.py delete mode 100644 server/tests/test_example.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 5bbfd0a..1b1d962 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -45,14 +45,16 @@ jobs: working-directory: client steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - - uses: actions/setup-node@v1 + - uses: actions/setup-node@v3 with: node-version: '14.x' + - run: git config --global url."https://git@".insteadOf git:// + - name: install - run: npm install + run: npm ci || npm install - name: build run: npm run build diff --git a/client/src/reset-password.js b/client/src/reset-password.js index 42057f1..4d0cc57 100644 --- a/client/src/reset-password.js +++ b/client/src/reset-password.js @@ -5,6 +5,7 @@ const form = document.getElementById('reset-password-form'); const url = new URL(window.nextUrl); const params = new URLSearchParams(url.search); const sendButton = document.getElementById('send-email'); +const emailSentCheckbox = document.getElementById('email_sent_checkbox'); form.onsubmit = (event) => { event.preventDefault(); @@ -22,12 +23,19 @@ form.onsubmit = (event) => { sendButton.disabled = true; sendPasswordResetEmail(auth, email, {url: url.toString()}) .then(() => { + // set `email_sent` to true, so server knows password reset was handled by firebase + emailSentCheckbox.checked = true; form.submit(); return true; }) .catch((reason) => { - console.error(reason); - sendButton.disabled = false; // allow another request if there was an error + if (reason.code === 'auth/user-not-found') { + // User not registered with OAuth, try attempting normal password reset + form.submit(); + } else { + console.error(reason); + sendButton.disabled = false; // allow another request if there was an error + } }); return false; diff --git a/server/cp/auth.py b/server/cp/auth.py index ada5090..6e1d9ef 100644 --- a/server/cp/auth.py +++ b/server/cp/auth.py @@ -1,12 +1,18 @@ +from typing import Optional import flask import logging import google.oauth2.id_token from flask_babel import gettext from google.auth.transport import requests -from newsroom.auth.utils import sign_user_by_email + +from newsroom.types import AuthProviderType +from newsroom.auth import get_company, get_user_by_email +from newsroom.auth.utils import sign_user_by_email, get_company_auth_provider, send_token from newsroom.auth.views import logout as _logout +from .password_reset_form import PasswordResetForm + TIMEOUT = 5 logger = logging.getLogger(__name__) @@ -31,7 +37,7 @@ def token(): return flask.redirect(flask.url_for("auth.login", token_error=1)) email = claims["email"] - return sign_user_by_email(email) + return sign_user_by_email(email, validate_login_attempt=True) return flask.redirect(flask.url_for("auth.login")) @@ -49,6 +55,36 @@ def reset_password_confirmation(): @blueprint.route("/cp_reset_password", methods=["GET", "POST"]) def reset_password(): - if flask.request.method == "POST": + form = PasswordResetForm() + + def render_reset_page(error_str: Optional[str] = None): + if error_str is not None: + flask.flash(error_str, "danger") + + return flask.render_template("cp_reset_password.html", form=form) + + if form.validate_on_submit(): + if form.email_sent.data is True: + # Password reset was handled by firebase from the front-end + return flask.redirect(flask.url_for("cp.auth.reset_password_confirmation")) + + # Password reset was not handled in the front-end, continue with standard password reset procedure + user = get_user_by_email(form.email.data) + if not user: + return render_reset_page(gettext("User not found")) + elif not user.get("is_enabled"): + return render_reset_page(gettext("User not enabled")) + + company = get_company(user) + auth_provider = get_company_auth_provider(company) + + if auth_provider["auth_type"] != AuthProviderType.PASSWORD.value: + return render_reset_page(gettext("Password reset for your account is not supported through Newshub")) + + # Send standard Newshub reset password email + if not send_token(user, "reset_password"): + return render_reset_page(gettext("An error occurred while sending reset password email")) + return flask.redirect(flask.url_for("cp.auth.reset_password_confirmation")) - return flask.render_template("cp_reset_password.html") + + return render_reset_page() diff --git a/server/cp/mgmt_api_signals.py b/server/cp/mgmt_api_signals.py new file mode 100644 index 0000000..a9119b7 --- /dev/null +++ b/server/cp/mgmt_api_signals.py @@ -0,0 +1,9 @@ +from newsroom.signals import company_create + + +def on_company_create(sender, company, **kwargs): + company.setdefault("auth_provider", "gip") + + +def init_app(app): + company_create.connect(on_company_create) diff --git a/server/cp/password_reset_form.py b/server/cp/password_reset_form.py new file mode 100644 index 0000000..10ac6e5 --- /dev/null +++ b/server/cp/password_reset_form.py @@ -0,0 +1,9 @@ +from flask_wtf import FlaskForm +from wtforms import StringField, BooleanField +from wtforms.validators import DataRequired, Length, Email +from flask_babel import lazy_gettext + + +class PasswordResetForm(FlaskForm): + email = StringField(lazy_gettext("Email"), validators=[DataRequired(), Length(1, 64), Email()]) + email_sent = BooleanField("email_sent", validators=[]) diff --git a/server/cp/signals.py b/server/cp/signals.py index e81d18a..7cf2196 100644 --- a/server/cp/signals.py +++ b/server/cp/signals.py @@ -1,5 +1,8 @@ +from typing import Optional +from newsroom.types import User, Company import cp +from superdesk import get_resource_service from newsroom.signals import publish_item, user_created, user_updated, user_deleted, push from cp.cem import send_notification @@ -33,18 +36,21 @@ def copy_correction_to_body_html(item): def on_user_created(sender, user, **kwargs): - send_notification("new user", user) + if user_auth_is_gip(user): + send_notification("new user", user) def on_user_updated(sender, user, updates=None, **kwargs): - if updates and updates.get("password"): - send_notification("password change", user) - else: - send_notification("update user", user) + if user_auth_is_gip(user): + if updates and updates.get("password"): + send_notification("password change", user) + else: + send_notification("update user", user) def on_user_deleted(sender, user, **kwargs): - send_notification("delete user", user) + if user_auth_is_gip(user): + send_notification("delete user", user) def on_push(sender, item, **kwargs): @@ -52,6 +58,17 @@ def on_push(sender, item, **kwargs): item["language"] = fix_language(item["language"]) +def user_auth_is_gip(user: User) -> bool: + if not user.get("company"): + return False + + company: Optional[Company] = get_resource_service("companies").find_one(req=None, _id=user["company"]) + if not company: + return False + + return company.get("auth_provider") == "gip" + + def init_app(app): publish_item.connect(on_publish_item) user_created.connect(on_user_created) diff --git a/server/requirements.txt b/server/requirements.txt index 8a3c405..f3b988b 100644 --- a/server/requirements.txt +++ b/server/requirements.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.10 +# This file is autogenerated by pip-compile with Python 3.8 # by the following command: # # pip-compile requirements.in @@ -17,7 +17,7 @@ asn1crypto==1.5.1 # oscrypto # pyhanko # pyhanko-certvalidator -async-timeout==4.0.2 +async-timeout==4.0.3 # via redis authlib==0.14.3 # via superdesk-core @@ -34,9 +34,9 @@ blinker==1.4 # raven # sentry-sdk # superdesk-core -boto3==1.28.14 +boto3==1.28.38 # via superdesk-core -botocore==1.31.14 +botocore==1.31.38 # via # boto3 # s3transfer @@ -48,7 +48,7 @@ cachetools==5.3.1 # google-auth celery[redis]==5.2.7 # via superdesk-core -cerberus==1.3.4 +cerberus==1.3.5 # via # eve # superdesk-core @@ -68,7 +68,7 @@ charset-normalizer==3.2.0 # via requests ciso8601==1.0.8 # via eve-elastic -click==8.1.6 +click==8.1.7 # via # celery # click-didyoumean @@ -86,7 +86,7 @@ click-repl==0.3.0 # via celery croniter==0.3.37 # via superdesk-core -cryptography==41.0.2 +cryptography==41.0.3 # via # authlib # jwcrypto @@ -102,7 +102,7 @@ deprecated==1.2.14 # limits draftjs-exporter[lxml]==2.1.7 # via superdesk-core -ecs-logging==2.0.2 +ecs-logging==2.1.0 # via elastic-apm elastic-apm[flask]==6.18.0 # via superdesk-core @@ -110,7 +110,7 @@ elasticsearch==7.13.4 # via eve-elastic eve==1.1.2 # via superdesk-core -eve-elastic==7.3.1 +eve-elastic==7.3.2 # via # newsroom-core # superdesk-core @@ -172,7 +172,7 @@ icalendar==4.0.9 # via superdesk-planning idna==3.4 # via requests -importlib-resources==6.0.0 +importlib-resources==6.0.1 # via limits isodate==0.6.1 # via python3-saml @@ -262,11 +262,11 @@ pymongo==3.11.4 # flask-pymongo # mongolock # superdesk-core -pyparsing==3.1.0 +pyparsing==3.1.1 # via # httplib2 # pyrtf3 -pypdf==3.13.0 +pypdf==3.15.4 # via xhtml2pdf pypng==0.20220715.0 # via qrcode @@ -291,6 +291,7 @@ python3-saml==1.15.0 # via newsroom-core pytz==2023.3 # via + # babel # celery # eve-elastic # flask-babel @@ -330,7 +331,7 @@ rsa==4.9 # via # google-auth # oauth2client -s3transfer==0.6.1 +s3transfer==0.6.2 # via boto3 sentry-sdk[flask]==1.5.12 # via newsroom-core @@ -362,6 +363,7 @@ tinycss2==1.2.1 typing-extensions==4.7.1 # via # limits + # pypdf # qrcode # superdesk-core tzlocal==2.1 @@ -370,7 +372,7 @@ tzlocal==2.1 # superdesk-core unidecode==0.4.21 # via superdesk-core -uritools==4.0.1 +uritools==4.0.2 # via pyhanko-certvalidator urllib3==1.25.11 # via @@ -410,6 +412,8 @@ xhtml2pdf==0.2.11 # via newsroom-core xmlsec==1.3.13 # via python3-saml +zipp==3.16.2 + # via importlib-resources # The following packages are considered to be unsafe in a requirements file: # setuptools diff --git a/server/settings.py b/server/settings.py index 6d58810..8a526af 100644 --- a/server/settings.py +++ b/server/settings.py @@ -8,7 +8,9 @@ BLUEPRINTS as DEFAULT_BLUEPRINTS, CELERY_BEAT_SCHEDULE as DEFAULT_CELERY_BEAT_SCHEDULE, CLIENT_URL, + AUTH_PROVIDERS, ) +from newsroom.types import AuthProviderType SERVER_PATH = pathlib.Path(__file__).resolve().parent @@ -276,3 +278,13 @@ AGENDA_SECTION = lazy_gettext("Calendar") SAVED_SECTION = lazy_gettext("Bookmarks") + +AUTH_PROVIDERS.extend([{ + "_id": "gip", + "name": lazy_gettext("Google"), + "auth_type": AuthProviderType.GOOGLE_OAUTH.value, +}, { + "_id": "azure", + "name": lazy_gettext("Azure"), + "auth_type": AuthProviderType.SAML.value, +}]) diff --git a/server/settings_mgmtapi.py b/server/settings_mgmtapi.py index d5ad832..b516ce2 100644 --- a/server/settings_mgmtapi.py +++ b/server/settings_mgmtapi.py @@ -6,3 +6,7 @@ ABS_PATH = Path(__file__).resolve().parent MGMT_API_ENABLED = True + +INSTALLED_APPS = [ + "cp.mgmt_api_signals", +] diff --git a/server/tests/test_example.py b/server/tests/test_example.py deleted file mode 100644 index 2a986bb..0000000 --- a/server/tests/test_example.py +++ /dev/null @@ -1,5 +0,0 @@ -def test_example(): - """ - Empty test to avoid pytest exit code in case of `no tests run` - """ - assert True diff --git a/server/tests/test_signals.py b/server/tests/test_signals.py index 51821a4..0ed9d10 100644 --- a/server/tests/test_signals.py +++ b/server/tests/test_signals.py @@ -44,7 +44,14 @@ def test_cem_notification_on_user_changes(app): "CEM_PLATFORM": "Test", } ) - user = {"_id": bson.ObjectId(), "email": "foo@example.com"} + company_id = bson.ObjectId() + app.data.insert("companies", [{ + "_id": company_id, + "name": "Example Company", + "is_enabled": True, + "auth_provider": "gip", + }]) + user = {"_id": bson.ObjectId(), "email": "foo@example.com", "company": company_id} with responses.RequestsMock(assert_all_requests_are_fired=True) as rsps: rsps.add( @@ -59,6 +66,7 @@ def test_cem_notification_on_user_changes(app): matchers.json_params_matcher( { "object_id": str(user["_id"]), + "company": str(company_id), "type": "new user", "platform": "Test", } @@ -76,6 +84,7 @@ def test_cem_notification_on_user_changes(app): matchers.json_params_matcher( { "object_id": str(user["_id"]), + "company": str(company_id), "type": "update user", "platform": "Test", } @@ -93,6 +102,7 @@ def test_cem_notification_on_user_changes(app): matchers.json_params_matcher( { "object_id": str(user["_id"]), + "company": str(company_id), "type": "password change", "platform": "Test", } @@ -110,6 +120,7 @@ def test_cem_notification_on_user_changes(app): matchers.json_params_matcher( { "object_id": str(user["_id"]), + "company": str(company_id), "type": "delete user", "platform": "Test", } @@ -120,6 +131,37 @@ def test_cem_notification_on_user_changes(app): signals.on_user_deleted(None, user=user) +def test_cem_notification_for_non_google_auth(app, mocker): + sub = mocker.patch("cp.signals.send_notification") + app.config.update( + { + "CEM_URL": "https://example.com", + "CEM_APIKEY": "somekey", + "CEM_PLATFORM": "Test", + } + ) + company_id = bson.ObjectId() + app.data.insert("companies", [{ + "_id": company_id, + "name": "Example Company", + "is_enabled": True, + "auth_provider": "azure", + }]) + user = {"_id": bson.ObjectId(), "email": "foo@example.com", "company": company_id} + + signals.on_user_created(None, user=user, foo=1) + assert len(sub.mock_calls) == 0 + + signals.on_user_updated(None, user=user, foo=1) + assert len(sub.mock_calls) == 0 + + signals.on_user_updated(None, user=user, updates={"password": "bar"}) + assert len(sub.mock_calls) == 0 + + signals.on_user_deleted(None, user=user) + assert len(sub.mock_calls) == 0 + + def test_language_agenda(): item = {"language": "en-CA"} signals.init_app(None) diff --git a/server/theme/cp_reset_password.html b/server/theme/cp_reset_password.html index 183b1f6..495b432 100644 --- a/server/theme/cp_reset_password.html +++ b/server/theme/cp_reset_password.html @@ -15,10 +15,12 @@
{{ gettext("Reset Password") }}
+ {{ form.csrf_token }}
- - + + {{ form.email(class="form-control", id="email", required="true")}}
+ {{ form.email_sent(id="email_sent_checkbox", hidden="hidden") }}
@@ -26,6 +28,8 @@
{{ gettext("Reset Password") }}
+ {% include "login_messages.html" %} + diff --git a/server/theme/swagger.yaml b/server/theme/swagger.yaml index ec2cc0a..fff1722 100644 --- a/server/theme/swagger.yaml +++ b/server/theme/swagger.yaml @@ -811,6 +811,12 @@ components: type: string is_enabled: type: boolean + auth_domain: + type: string + description: Authentication domain (for SSO) + auth_provider: + type: string + description: Authentication provider for this companies users contact_name: type: string contact_email: