From b3cd08a64f5b51563b637e34f62793e2df681a1d Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 10:42:01 -0400 Subject: [PATCH 1/7] invitation can only be accepted by user with matching DOD ID --- atst/domain/invitations.py | 14 +++++++++++++- atst/routes/errors.py | 5 +++-- atst/routes/workspaces.py | 2 +- tests/domain/test_invitations.py | 19 ++++++++++++++----- tests/routes/test_workspaces.py | 16 ++++++++++++++++ 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/atst/domain/invitations.py b/atst/domain/invitations.py index ee518c4e..65df2390 100644 --- a/atst/domain/invitations.py +++ b/atst/domain/invitations.py @@ -8,6 +8,15 @@ from atst.domain.workspace_users import WorkspaceUsers from .exceptions import NotFoundError +class WrongUserError(Exception): + def __init__(self, user, invite): + self.user = user + self.invite = invite + + @property + def message(self): + return "User {} with DOD ID {} does not match expected DOD ID {} for invitation {}".format(self.user.id, self.user.dod_id, self.invite.user.dod_id, self.invite.id) + class InvitationError(Exception): def __init__(self, invite): self.invite = invite @@ -45,9 +54,12 @@ class Invitations(object): return invite @classmethod - def accept(cls, token): + def accept(cls, user, token): invite = Invitations._get(token) + if invite.user.dod_id != user.dod_id: + raise WrongUserError(user, invite) + if invite.is_expired: invite.status = InvitationStatus.REJECTED elif invite.is_pending: diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 4871f89d..49b5fbc9 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -3,7 +3,7 @@ from flask_wtf.csrf import CSRFError import werkzeug.exceptions as werkzeug_exceptions import atst.domain.exceptions as exceptions -from atst.domain.invitations import InvitationError +from atst.domain.invitations import InvitationError, WrongUserError as InvitationWrongUserError def make_error_pages(app): @@ -43,12 +43,13 @@ def make_error_pages(app): ) @app.errorhandler(InvitationError) + @app.errorhandler(InvitationWrongUserError) # pylint: disable=unused-variable def invalid_invitation(e): log_error(e) return ( render_template( - "error.html", message="The invitation link you clicked is invalid." + "error.html", message="The link you followed is invalid." ), 404, ) diff --git a/atst/routes/workspaces.py b/atst/routes/workspaces.py index 726e1bac..35807e00 100644 --- a/atst/routes/workspaces.py +++ b/atst/routes/workspaces.py @@ -363,7 +363,7 @@ def update_member(workspace_id, member_id): def accept_invitation(token): # TODO: check that the current_user DOD ID matches the user associated with # the invitation - invite = Invitations.accept(token) + invite = Invitations.accept(g.current_user, token) return redirect( url_for("workspaces.show_workspace", workspace_id=invite.workspace.id) diff --git a/tests/domain/test_invitations.py b/tests/domain/test_invitations.py index 2ec08a9f..2f677357 100644 --- a/tests/domain/test_invitations.py +++ b/tests/domain/test_invitations.py @@ -2,7 +2,7 @@ import datetime import pytest import re -from atst.domain.invitations import Invitations, InvitationError +from atst.domain.invitations import Invitations, InvitationError, WrongUserError from atst.models.invitation import Status from tests.factories import ( @@ -31,7 +31,7 @@ def test_accept_invitation(): ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) invite = Invitations.create(ws_role, workspace.owner, user) assert invite.is_pending - accepted_invite = Invitations.accept(invite.token) + accepted_invite = Invitations.accept(user, invite.token) assert accepted_invite.is_accepted @@ -43,7 +43,7 @@ def test_accept_expired_invitation(): user_id=user.id, expiration_time=expiration_time, status=Status.PENDING ) with pytest.raises(InvitationError): - Invitations.accept(invite.token) + Invitations.accept(user, invite.token) assert invite.is_rejected @@ -52,11 +52,20 @@ def test_accept_rejected_invite(): user = UserFactory.create() invite = InvitationFactory.create(user_id=user.id, status=Status.REJECTED) with pytest.raises(InvitationError): - Invitations.accept(invite.token) + Invitations.accept(user, invite.token) def test_accept_revoked_invite(): user = UserFactory.create() invite = InvitationFactory.create(user_id=user.id, status=Status.REVOKED) with pytest.raises(InvitationError): - Invitations.accept(invite.token) + Invitations.accept(user, invite.token) + + +def test_wrong_user_accepts_invitation(): + user = UserFactory.create() + wrong_user = UserFactory.create() + invite = InvitationFactory.create(user_id=user.id) + with pytest.raises(WrongUserError): + Invitations.accept(wrong_user, invite.token) + diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index d4f543de..da0d8a1a 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -355,3 +355,19 @@ def test_user_who_has_not_accepted_workspace_invite_cannot_view(client, user_ses user_session(user) response = client.get("/workspaces/{}/projects".format(workspace.id)) assert response.status_code == 404 + + +def test_user_accepts_invite_with_wrong_dod_id(client, user_session): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + different_user = UserFactory.create() + ws_role = WorkspaceRoleFactory.create( + user=user, workspace=workspace, status=WorkspaceRoleStatus.PENDING + ) + invite = InvitationFactory.create( + user_id=user.id, workspace_role_id=ws_role.id + ) + user_session(different_user) + response = client.get(url_for("workspaces.accept_invitation", token=invite.token)) + + assert response.status_code == 404 From cdef2d88030229d2c69c630364e0cf7a36a02f58 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 11:05:39 -0400 Subject: [PATCH 2/7] additional invitation test for new user --- tests/routes/test_workspaces.py | 34 +++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index da0d8a1a..af1402e3 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -15,6 +15,7 @@ from atst.models.workspace_user import WorkspaceUser from atst.models.workspace_role import Status as WorkspaceRoleStatus from atst.models.invitation import Status as InvitationStatus from atst.queue import queue +from atst.domain.users import Users def test_user_with_permission_has_budget_report_link(client, user_session): @@ -299,7 +300,7 @@ def test_update_member_environment_role_with_no_data(client, user_session): assert EnvironmentRoles.get(user.id, env1_id).role == "developer" -def test_new_member_accepts_valid_invite(client, user_session): +def test_existing_member_accepts_valid_invite(client, user_session): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create( @@ -325,7 +326,36 @@ def test_new_member_accepts_valid_invite(client, user_session): assert len(Workspaces.for_user(user)) == 1 -def test_new_member_accept_invalid_invite(client, user_session): +def test_new_member_accepts_valid_invite(client, user_session): + workspace = WorkspaceFactory.create() + user_info = UserFactory.dictionary() + + user_session(workspace.owner) + client.post( + url_for("workspaces.create_member", workspace_id=workspace.id), + data={ + "workspace_role": "developer", + **user_info, + } + ) + + user = Users.get_by_dod_id(user_info["dod_id"]) + token = user.invitations[0].token + + user_session(user) + response = client.get(url_for("workspaces.accept_invitation", token=token)) + + # user is redirected to the workspace view + assert response.status_code == 302 + assert ( + url_for("workspaces.show_workspace", workspace_id=workspace.id) + in response.headers["Location"] + ) + # the user has access to the workspace + assert len(Workspaces.for_user(user)) == 1 + + +def test_member_accepts_invalid_invite(client, user_session): workspace = WorkspaceFactory.create() user = UserFactory.create() ws_role = WorkspaceRoleFactory.create( From 87baa1f873136318c62d384b9fcd6b8426006ab2 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 12:06:42 -0400 Subject: [PATCH 3/7] more fine-grained errors for invalid invitations --- atst/domain/invitations.py | 41 ++++++++++++++++++++++---------- atst/routes/errors.py | 39 ++++++++++++++++++------------ tests/domain/test_invitations.py | 18 ++++++++++++-- tests/routes/test_workspaces.py | 28 ++++++++++++++++------ 4 files changed, 90 insertions(+), 36 deletions(-) diff --git a/atst/domain/invitations.py b/atst/domain/invitations.py index 65df2390..a3946399 100644 --- a/atst/domain/invitations.py +++ b/atst/domain/invitations.py @@ -15,7 +15,19 @@ class WrongUserError(Exception): @property def message(self): - return "User {} with DOD ID {} does not match expected DOD ID {} for invitation {}".format(self.user.id, self.user.dod_id, self.invite.user.dod_id, self.invite.id) + return "User {} with DOD ID {} does not match expected DOD ID {} for invitation {}".format( + self.user.id, self.user.dod_id, self.invite.user.dod_id, self.invite.id + ) + + +class ExpiredError(Exception): + def __init__(self, invite): + self.invite = invite + + @property + def message(self): + return "Invitation {} has expired.".format(self.invite.id) + class InvitationError(Exception): def __init__(self, invite): @@ -60,23 +72,28 @@ class Invitations(object): if invite.user.dod_id != user.dod_id: raise WrongUserError(user, invite) - if invite.is_expired: - invite.status = InvitationStatus.REJECTED - elif invite.is_pending: - invite.status = InvitationStatus.ACCEPTED + elif invite.is_expired: + Invitations._update_status(invite, InvitationStatus.REJECTED) + raise ExpiredError(invite) - db.session.add(invite) - db.session.commit() - - if invite.is_revoked or invite.is_rejected: + elif invite.is_accepted or invite.is_revoked or invite.is_rejected: raise InvitationError(invite) - WorkspaceUsers.enable(invite.workspace_role) - - return invite + elif invite.is_pending: + Invitations._update_status(invite, InvitationStatus.ACCEPTED) + WorkspaceUsers.enable(invite.workspace_role) + return invite @classmethod def current_expiration_time(cls): return datetime.datetime.now() + datetime.timedelta( minutes=Invitations.EXPIRATION_LIMIT_MINUTES ) + + @classmethod + def _update_status(cls, invite, new_status): + invite.status = new_status + db.session.add(invite) + db.session.commit() + + return invite diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 49b5fbc9..7cf58b46 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -3,27 +3,35 @@ from flask_wtf.csrf import CSRFError import werkzeug.exceptions as werkzeug_exceptions import atst.domain.exceptions as exceptions -from atst.domain.invitations import InvitationError, WrongUserError as InvitationWrongUserError +from atst.domain.invitations import ( + InvitationError, + ExpiredError as InvitationExpiredError, + WrongUserError as InvitationWrongUserError, +) + + +def log_error(e): + error_message = e.message if hasattr(e, "message") else str(e) + current_app.logger.error(error_message) + + +def handle_error(e, message="Not Found", code=404): + log_error(e) + return render_template("error.html", message=message), code def make_error_pages(app): - def log_error(e): - error_message = e.message if hasattr(e, "message") else str(e) - app.logger.error(error_message) - @app.errorhandler(werkzeug_exceptions.NotFound) @app.errorhandler(exceptions.NotFoundError) @app.errorhandler(exceptions.UnauthorizedError) # pylint: disable=unused-variable def not_found(e): - log_error(e) - return render_template("error.html", message="Not Found"), 404 + return handle_error(e) @app.errorhandler(exceptions.UnauthenticatedError) # pylint: disable=unused-variable def unauthorized(e): - log_error(e) - return render_template("error.html", message="Log in Failed"), 401 + return handle_error(e, message="Log in Failed", code=401) @app.errorhandler(CSRFError) # pylint: disable=unused-variable @@ -46,12 +54,13 @@ def make_error_pages(app): @app.errorhandler(InvitationWrongUserError) # pylint: disable=unused-variable def invalid_invitation(e): - log_error(e) - return ( - render_template( - "error.html", message="The link you followed is invalid." - ), - 404, + return handle_error(e, message="The link you followed is invalid.", code=404) + + @app.errorhandler(InvitationExpiredError) + # pylint: disable=unused-variable + def invalid_invitation(e): + return handle_error( + e, message="The invitation you followed has expired.", code=404 ) return app diff --git a/tests/domain/test_invitations.py b/tests/domain/test_invitations.py index 2f677357..5a2198d9 100644 --- a/tests/domain/test_invitations.py +++ b/tests/domain/test_invitations.py @@ -2,7 +2,12 @@ import datetime import pytest import re -from atst.domain.invitations import Invitations, InvitationError, WrongUserError +from atst.domain.invitations import ( + Invitations, + InvitationError, + WrongUserError, + ExpiredError, +) from atst.models.invitation import Status from tests.factories import ( @@ -42,7 +47,7 @@ def test_accept_expired_invitation(): invite = InvitationFactory.create( user_id=user.id, expiration_time=expiration_time, status=Status.PENDING ) - with pytest.raises(InvitationError): + with pytest.raises(ExpiredError): Invitations.accept(user, invite.token) assert invite.is_rejected @@ -69,3 +74,12 @@ def test_wrong_user_accepts_invitation(): with pytest.raises(WrongUserError): Invitations.accept(wrong_user, invite.token) + +def test_accept_invitation_twice(): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + ws_role = WorkspaceRoleFactory.create(user=user, workspace=workspace) + invite = Invitations.create(ws_role, workspace.owner, user) + Invitations.accept(user, invite.token) + with pytest.raises(InvitationError): + Invitations.accept(user, invite.token) diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index af1402e3..29bc4f87 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -1,3 +1,4 @@ +import datetime from flask import url_for from tests.factories import ( @@ -333,10 +334,7 @@ def test_new_member_accepts_valid_invite(client, user_session): user_session(workspace.owner) client.post( url_for("workspaces.create_member", workspace_id=workspace.id), - data={ - "workspace_role": "developer", - **user_info, - } + data={"workspace_role": "developer", **user_info}, ) user = Users.get_by_dod_id(user_info["dod_id"]) @@ -394,10 +392,26 @@ def test_user_accepts_invite_with_wrong_dod_id(client, user_session): ws_role = WorkspaceRoleFactory.create( user=user, workspace=workspace, status=WorkspaceRoleStatus.PENDING ) - invite = InvitationFactory.create( - user_id=user.id, workspace_role_id=ws_role.id - ) + invite = InvitationFactory.create(user_id=user.id, workspace_role_id=ws_role.id) user_session(different_user) response = client.get(url_for("workspaces.accept_invitation", token=invite.token)) assert response.status_code == 404 + + +def test_user_accepts_expired_invite(client, user_session): + workspace = WorkspaceFactory.create() + user = UserFactory.create() + ws_role = WorkspaceRoleFactory.create( + user=user, workspace=workspace, status=WorkspaceRoleStatus.PENDING + ) + invite = InvitationFactory.create( + user_id=user.id, + workspace_role_id=ws_role.id, + status=InvitationStatus.REJECTED, + expiration_time=datetime.datetime.now() - datetime.timedelta(seconds=1), + ) + user_session(user) + response = client.get(url_for("workspaces.accept_invitation", token=invite.token)) + + assert response.status_code == 404 From 0a176239b73c1f5fdf09e2310b45c1fa7daee275 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 13:41:04 -0400 Subject: [PATCH 4/7] add home link to error page and catch missing template exception in glob route --- atst/routes/__init__.py | 7 ++++++- templates/error.html | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 0149ace5..780ff0c3 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -2,6 +2,7 @@ import urllib.parse as url from flask import Blueprint, render_template, g, redirect, session, url_for, request from flask import current_app as app +from jinja2.exceptions import TemplateNotFound import pendulum import os @@ -10,6 +11,7 @@ from atst.domain.users import Users from atst.domain.authnid import AuthenticationContext from atst.domain.audit_log import AuditLog from atst.domain.auth import logout as _logout +from werkzeug.exceptions import NotFound bp = Blueprint("atst", __name__) @@ -77,7 +79,10 @@ def styleguide(): @bp.route("/") def catch_all(path): - return render_template("{}.html".format(path)) + try: + return render_template("{}.html".format(path)) + except TemplateNotFound: + raise NotFound() def _make_authentication_context(): diff --git a/templates/error.html b/templates/error.html index c8714c01..f58eac86 100644 --- a/templates/error.html +++ b/templates/error.html @@ -10,6 +10,10 @@

An error occurred.

{% endif %} +{% if g.current_user %} +

Return home.

+{% endif %} + {% endblock %} From cb25dcdd3247bdfbce43b3cab131e0307748f208 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 14:00:13 -0400 Subject: [PATCH 5/7] invitation cannot be reused if wrong user accepts it --- atst/domain/invitations.py | 2 ++ tests/domain/test_invitations.py | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/atst/domain/invitations.py b/atst/domain/invitations.py index a3946399..00797ddd 100644 --- a/atst/domain/invitations.py +++ b/atst/domain/invitations.py @@ -70,6 +70,8 @@ class Invitations(object): invite = Invitations._get(token) if invite.user.dod_id != user.dod_id: + if invite.is_pending: + Invitations._update_status(invite, InvitationStatus.REJECTED) raise WrongUserError(user, invite) elif invite.is_expired: diff --git a/tests/domain/test_invitations.py b/tests/domain/test_invitations.py index 5a2198d9..059a8927 100644 --- a/tests/domain/test_invitations.py +++ b/tests/domain/test_invitations.py @@ -75,6 +75,16 @@ def test_wrong_user_accepts_invitation(): Invitations.accept(wrong_user, invite.token) +def test_user_cannot_accept_invitation_accepted_by_wrong_user(): + user = UserFactory.create() + wrong_user = UserFactory.create() + invite = InvitationFactory.create(user_id=user.id) + with pytest.raises(WrongUserError): + Invitations.accept(wrong_user, invite.token) + with pytest.raises(InvitationError): + Invitations.accept(user, invite.token) + + def test_accept_invitation_twice(): workspace = WorkspaceFactory.create() user = UserFactory.create() From 43eeba2f967553841c7946ef88eab0dfdbd364b4 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 14:43:05 -0400 Subject: [PATCH 6/7] remove TODO for checking user against invitation --- atst/routes/workspaces.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/atst/routes/workspaces.py b/atst/routes/workspaces.py index 35807e00..89731326 100644 --- a/atst/routes/workspaces.py +++ b/atst/routes/workspaces.py @@ -361,8 +361,6 @@ def update_member(workspace_id, member_id): @bp.route("/workspaces/invitation/", methods=["GET"]) def accept_invitation(token): - # TODO: check that the current_user DOD ID matches the user associated with - # the invitation invite = Invitations.accept(g.current_user, token) return redirect( From a5d34043d737f1e6f27c9a47880c470bf5ae5210 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 31 Oct 2018 15:04:18 -0400 Subject: [PATCH 7/7] workaround for user profile check in invitations test --- tests/routes/test_workspaces.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/routes/test_workspaces.py b/tests/routes/test_workspaces.py index 29bc4f87..be7097f2 100644 --- a/tests/routes/test_workspaces.py +++ b/tests/routes/test_workspaces.py @@ -327,7 +327,7 @@ def test_existing_member_accepts_valid_invite(client, user_session): assert len(Workspaces.for_user(user)) == 1 -def test_new_member_accepts_valid_invite(client, user_session): +def test_new_member_accepts_valid_invite(monkeypatch, client, user_session): workspace = WorkspaceFactory.create() user_info = UserFactory.dictionary() @@ -340,6 +340,9 @@ def test_new_member_accepts_valid_invite(client, user_session): user = Users.get_by_dod_id(user_info["dod_id"]) token = user.invitations[0].token + monkeypatch.setattr( + "atst.domain.auth.should_redirect_to_user_profile", lambda *args: False + ) user_session(user) response = client.get(url_for("workspaces.accept_invitation", token=token))