From 1a5800cbc5493ea58ab7455ceb2d7756420674d4 Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 7 Aug 2018 15:40:51 -0400 Subject: [PATCH 1/7] Requests domain module can determine if user can view request --- atst/domain/requests.py | 8 ++++++++ tests/domain/test_requests.py | 10 +++++++++- tests/factories.py | 9 ++++++--- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index e7b64e5d..843c7243 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -143,3 +143,11 @@ class Requests(object): return request.status == "incomplete" and all( section in existing_request_sections for section in all_request_sections ) + + @classmethod + def is_creator(cls, request_id, user_id): + try: + db.session.query(Request).filter_by(id=request_id, creator=user_id).one() + return True + except NoResultFound: + return False diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 1d696097..609dae90 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -5,7 +5,7 @@ from atst.domain.exceptions import NotFoundError from atst.domain.requests import Requests from atst.models.request_status_event import RequestStatus -from tests.factories import RequestFactory +from tests.factories import RequestFactory, UserFactory @pytest.fixture(scope="function") @@ -48,3 +48,11 @@ def test_dont_auto_approve_if_no_dollar_value_specified(new_request): request = Requests.submit(new_request) assert request.status == RequestStatus.PENDING_CCPO_APPROVAL + + +def test_can_check_if_user_created_request(session): + user_allowed = UserFactory.create() + user_denied = UserFactory.create() + request = RequestFactory.create(creator=user_allowed.id) + assert Requests.is_creator(request.id, user_allowed.id) + assert not Requests.is_creator(request.id, user_denied.id) diff --git a/tests/factories.py b/tests/factories.py index dc68eb5c..d731c42f 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,3 +1,5 @@ +import random +import string import factory from uuid import uuid4 @@ -46,7 +48,8 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): model = User id = factory.Sequence(lambda x: uuid4()) - email = "fake.user@mail.com" - first_name = "Fake" - last_name = "User" + email = factory.Faker("email") + first_name = factory.Faker("first_name") + last_name = factory.Faker("last_name") atat_role = factory.SubFactory(RoleFactory) + dod_id = factory.LazyFunction(lambda: "".join(random.choices(string.digits, k=10))) From 2cfc1424175108601c49baf5ba793caa558afadf Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 7 Aug 2018 16:15:34 -0400 Subject: [PATCH 2/7] simple implementation of request view authorization --- atst/routes/requests/requests_form.py | 16 ++++++++++++ tests/factories.py | 11 ++++++++ tests/routes/test_request_new.py | 37 ++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index 63a14224..391d49ed 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -3,6 +3,7 @@ from flask import g, redirect, render_template, url_for, request as http_request from . import requests_bp from atst.domain.requests import Requests from atst.routes.requests.jedi_request_flow import JEDIRequestFlow +from atst.models.permissions import Permissions @requests_bp.route("/requests/new/", methods=["GET"]) @@ -25,6 +26,9 @@ def requests_form_new(screen): ) @requests_bp.route("/requests/new//", methods=["GET"]) def requests_form_update(screen=1, request_id=None): + if request_id and not _can_view_request(request_id): + return redirect(url_for("atst.unauthorized")) + request = Requests.get(request_id) if request_id is not None else None jedi_flow = JEDIRequestFlow(screen, request, request_id=request_id) @@ -79,10 +83,12 @@ def requests_update(screen=1, request_id=None): request_id=jedi_flow.request_id, ) return redirect(where) + else: return render_template( "requests/screen-%d.html" % int(screen), **rerender_args ) + else: return render_template("requests/screen-%d.html" % int(screen), **rerender_args) @@ -94,5 +100,15 @@ def requests_submit(request_id=None): if request.status == "approved": return redirect("/requests?modal=True") + else: return redirect("/requests") + + +# TODO: generalize this, along with other authorizations, into a policy-pattern +# for authorization in the application +def _can_view_request(request_id): + return ( + Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST in g.current_user.atat_permissions + or Requests.is_creator(request_id, g.current_user.id) + ) diff --git a/tests/factories.py b/tests/factories.py index d731c42f..12c41fd4 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -9,6 +9,8 @@ from atst.models.pe_number import PENumber from atst.models.task_order import TaskOrder from atst.models.user import User from atst.models.role import Role +from atst.models.request_status_event import RequestStatusEvent +from atst.domain.roles import Roles class RequestStatusFactory(factory.alchemy.SQLAlchemyModelFactory): @@ -24,6 +26,7 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): status_events = factory.RelatedFactory( RequestStatusFactory, "request", new_status=RequestStatus.STARTED ) + body = {} class PENumberFactory(factory.alchemy.SQLAlchemyModelFactory): @@ -53,3 +56,11 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): last_name = factory.Faker("last_name") atat_role = factory.SubFactory(RoleFactory) dod_id = factory.LazyFunction(lambda: "".join(random.choices(string.digits, k=10))) + + +class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): + + class Meta: + model = RequestStatusEvent + + id = factory.Sequence(lambda x: uuid4()) diff --git a/tests/routes/test_request_new.py b/tests/routes/test_request_new.py index 0927f1be..a7030e6f 100644 --- a/tests/routes/test_request_new.py +++ b/tests/routes/test_request_new.py @@ -2,7 +2,8 @@ import re import pytest import urllib from tests.mocks import MOCK_USER, MOCK_REQUEST -from tests.factories import RequestFactory +from tests.factories import RequestFactory, UserFactory, RequestStatusEventFactory +from atst.domain.roles import Roles ERROR_CLASS = "alert--error" @@ -27,3 +28,37 @@ def test_submit_valid_request_form(monkeypatch, client, user_session): data="meaning=42", ) assert "/requests/new/2" in response.headers.get("Location") + + +def test_owner_can_view_request(client, user_session): + user = UserFactory.create() + user_session(user) + request = RequestFactory.create(creator=user.id) + status = RequestStatusEventFactory.create(request_id=request.id) + + response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) + + assert response.status_code == 200 + + +def test_non_owner_cannot_view_request(client, user_session): + user = UserFactory.create() + user_session(user) + request = RequestFactory.create() + status = RequestStatusEventFactory.create(request_id=request.id) + + response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) + + assert response.status_code == 401 + + +def test_ccpo_can_view_request(client, user_session): + ccpo = Roles.get("ccpo") + user = UserFactory.create(atat_role=ccpo) + user_session(user) + request = RequestFactory.create() + status = RequestStatusEventFactory.create(request_id=request.id) + + response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) + + assert response.status_code == 200 From 7b8934e0cb3ec9ea9396217e44be879d80f2606a Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 8 Aug 2018 11:26:54 -0400 Subject: [PATCH 3/7] add Flask error handlers --- atst/app.py | 2 ++ atst/routes/__init__.py | 12 ++---------- atst/routes/errors.py | 13 +++++++++++++ atst/routes/requests/requests_form.py | 4 ++-- templates/not_found.html | 12 ++++++++++++ tests/routes/test_request_new.py | 2 +- tests/test_auth.py | 7 ++----- 7 files changed, 34 insertions(+), 18 deletions(-) create mode 100644 atst/routes/errors.py create mode 100644 templates/not_found.html diff --git a/atst/app.py b/atst/app.py index c86ab9bf..d095a8bf 100644 --- a/atst/app.py +++ b/atst/app.py @@ -15,6 +15,7 @@ from atst.routes import bp from atst.routes.workspaces import bp as workspace_routes from atst.routes.requests import requests_bp from atst.routes.dev import bp as dev_routes +from atst.routes.errors import make_error_pages from atst.domain.authnid.crl.validator import Validator from atst.domain.auth import apply_authentication @@ -45,6 +46,7 @@ def make_app(config): Session(app) assets_environment.init_app(app) + make_error_pages(app) app.register_blueprint(bp) app.register_blueprint(workspace_routes) app.register_blueprint(requests_bp) diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 1884d28f..4e346fa9 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -1,4 +1,4 @@ -from flask import Blueprint, render_template, g, redirect, session, url_for, request +from flask import Blueprint, abort, render_template, g, redirect, session, url_for, request from flask import current_app as app import pendulum @@ -39,15 +39,7 @@ def login_redirect(): return redirect(url_for("atst.home")) else: - return redirect(url_for("atst.unauthorized")) - - -@bp.route("/unauthorized") -def unauthorized(): - template = render_template('unauthorized.html') - response = app.make_response(template) - response.status_code = 401 - return response + return abort(401) def _is_valid_certificate(request): diff --git a/atst/routes/errors.py b/atst/routes/errors.py new file mode 100644 index 00000000..261c654c --- /dev/null +++ b/atst/routes/errors.py @@ -0,0 +1,13 @@ +from flask import render_template + + +def make_error_pages(app): + @app.errorhandler(404) + def not_found(e): + return render_template("not_found.html"), 404 + + + @app.errorhandler(401) + def unauthorized(e): + return render_template('unauthorized.html'), 401 + diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index 391d49ed..688a4ce0 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -1,4 +1,4 @@ -from flask import g, redirect, render_template, url_for, request as http_request +from flask import abort, g, redirect, render_template, url_for, request as http_request from . import requests_bp from atst.domain.requests import Requests @@ -27,7 +27,7 @@ def requests_form_new(screen): @requests_bp.route("/requests/new//", methods=["GET"]) def requests_form_update(screen=1, request_id=None): if request_id and not _can_view_request(request_id): - return redirect(url_for("atst.unauthorized")) + abort(404) request = Requests.get(request_id) if request_id is not None else None jedi_flow = JEDIRequestFlow(screen, request, request_id=request_id) diff --git a/templates/not_found.html b/templates/not_found.html new file mode 100644 index 00000000..59cc223f --- /dev/null +++ b/templates/not_found.html @@ -0,0 +1,12 @@ +{% extends "error_base.html" %} + +{% block content %} + +
+ +

Not Found

+ +
+ +{% endblock %} + diff --git a/tests/routes/test_request_new.py b/tests/routes/test_request_new.py index a7030e6f..1a722ca4 100644 --- a/tests/routes/test_request_new.py +++ b/tests/routes/test_request_new.py @@ -49,7 +49,7 @@ def test_non_owner_cannot_view_request(client, user_session): response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) - assert response.status_code == 401 + assert response.status_code == 404 def test_ccpo_can_view_request(client, user_session): diff --git a/tests/test_auth.py b/tests/test_auth.py index 69cb3166..7e2b483d 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -27,8 +27,7 @@ def test_successful_login_redirect(client, monkeypatch): def test_unsuccessful_login_redirect(client, monkeypatch): resp = client.get("/login-redirect") - assert resp.status_code == 302 - assert "unauthorized" in resp.headers["Location"] + assert resp.status_code == 401 assert "user_id" not in session @@ -55,7 +54,6 @@ def test_routes_are_protected(client, app): UNPROTECTED_ROUTES = ["/", "/login-dev", "/login-redirect", "/unauthorized"] - # this implicitly relies on the test config and test CRL in tests/fixtures/crl @@ -72,8 +70,7 @@ def test_crl_validation_on_login(client): "HTTP_X_SSL_CLIENT_CERT": bad_cert.decode(), }, ) - assert resp.status_code == 302 - assert "unauthorized" in resp.headers["Location"] + assert resp.status_code == 401 assert "user_id" not in session # good cert is not on the test CRL, passes From d5ed99089c94807aae1378edc54421917b867802 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 8 Aug 2018 12:09:23 -0400 Subject: [PATCH 4/7] raise exceptions, map to http error codes --- atst/domain/exceptions.py | 14 ++++++++++++++ atst/routes/__init__.py | 8 ++++++-- atst/routes/errors.py | 10 ++++++++-- atst/routes/requests/requests_form.py | 20 ++++++++++++-------- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/atst/domain/exceptions.py b/atst/domain/exceptions.py index 802997d4..b92e64c1 100644 --- a/atst/domain/exceptions.py +++ b/atst/domain/exceptions.py @@ -14,3 +14,17 @@ class AlreadyExistsError(Exception): @property def message(self): return "{} already exists".format(self.resource_name) + + +class UnauthorizedError(Exception): + def __init__(self, user, action): + self.user = user + self.action = action + + @property + def message(self): + return "User {} not authorized to {}".format(self.user.id, self.action) + + +class UnauthenticatedError(Exception): + pass diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index 4e346fa9..965b4b37 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -1,10 +1,11 @@ -from flask import Blueprint, abort, render_template, g, redirect, session, url_for, request +from flask import Blueprint, render_template, g, redirect, session, url_for, request from flask import current_app as app import pendulum from atst.domain.requests import Requests from atst.domain.users import Users from atst.domain.authnid.utils import parse_sdn +from atst.domain.exceptions import UnauthenticatedError bp = Blueprint("atst", __name__) @@ -29,6 +30,9 @@ def catch_all(path): return render_template("{}.html".format(path)) +# TODO: this should be partly consolidated into a domain function that takes +# all the necessary UWSGI environment values as args and either returns a user +# or raises the UnauthenticatedError @bp.route('/login-redirect') def login_redirect(): if request.environ.get('HTTP_X_SSL_CLIENT_VERIFY') == 'SUCCESS' and _is_valid_certificate(request): @@ -39,7 +43,7 @@ def login_redirect(): return redirect(url_for("atst.home")) else: - return abort(401) + raise UnauthenticatedError() def _is_valid_certificate(request): diff --git a/atst/routes/errors.py b/atst/routes/errors.py index 261c654c..0d0211b8 100644 --- a/atst/routes/errors.py +++ b/atst/routes/errors.py @@ -1,13 +1,19 @@ from flask import render_template +import atst.domain.exceptions as exceptions + def make_error_pages(app): - @app.errorhandler(404) + @app.errorhandler(exceptions.NotFoundError) + @app.errorhandler(exceptions.UnauthorizedError) + # pylint: disable=unused-variable def not_found(e): return render_template("not_found.html"), 404 - @app.errorhandler(401) + @app.errorhandler(exceptions.UnauthenticatedError) + # pylint: disable=unused-variable def unauthorized(e): return render_template('unauthorized.html'), 401 + return app diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index 688a4ce0..ee03c155 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -1,9 +1,10 @@ -from flask import abort, g, redirect, render_template, url_for, request as http_request +from flask import g, redirect, render_template, url_for, request as http_request from . import requests_bp from atst.domain.requests import Requests from atst.routes.requests.jedi_request_flow import JEDIRequestFlow from atst.models.permissions import Permissions +from atst.domain.exceptions import UnauthorizedError @requests_bp.route("/requests/new/", methods=["GET"]) @@ -26,8 +27,8 @@ def requests_form_new(screen): ) @requests_bp.route("/requests/new//", methods=["GET"]) def requests_form_update(screen=1, request_id=None): - if request_id and not _can_view_request(request_id): - abort(404) + if request_id: + _check_can_view_request(request_id) request = Requests.get(request_id) if request_id is not None else None jedi_flow = JEDIRequestFlow(screen, request, request_id=request_id) @@ -107,8 +108,11 @@ def requests_submit(request_id=None): # TODO: generalize this, along with other authorizations, into a policy-pattern # for authorization in the application -def _can_view_request(request_id): - return ( - Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST in g.current_user.atat_permissions - or Requests.is_creator(request_id, g.current_user.id) - ) +def _check_can_view_request(request_id): + if Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST in g.current_user.atat_permissions: + pass + elif Requests.is_creator(request_id, g.current_user.id): + pass + else: + raise UnauthorizedError(g.current_user, "view request {}".format(request_id)) + From 337dd2414b41e28a308a0c6954fa0d590bc4856a Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 8 Aug 2018 14:05:59 -0400 Subject: [PATCH 5/7] catch bad request_id in request form GET --- atst/domain/requests.py | 4 ++-- tests/routes/test_request_new.py | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index 843c7243..c4b29cd9 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -1,4 +1,4 @@ -from sqlalchemy import exists, and_ +from sqlalchemy import exists, and_, exc from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.orm.attributes import flag_modified @@ -149,5 +149,5 @@ class Requests(object): try: db.session.query(Request).filter_by(id=request_id, creator=user_id).one() return True - except NoResultFound: + except (NoResultFound, exc.DataError): return False diff --git a/tests/routes/test_request_new.py b/tests/routes/test_request_new.py index 1a722ca4..6a0ccb86 100644 --- a/tests/routes/test_request_new.py +++ b/tests/routes/test_request_new.py @@ -62,3 +62,10 @@ def test_ccpo_can_view_request(client, user_session): response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) assert response.status_code == 200 + + +def test_nonexistent_request(client, user_session): + user_session() + response = client.get("/requests/new/1/foo", follow_redirects=True) + + assert response.status_code == 404 From 2b56ce6260aee9eb2ce4242c391ac7a398e7abb5 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 8 Aug 2018 14:11:11 -0400 Subject: [PATCH 6/7] use existing Requests function for creator check --- atst/domain/requests.py | 21 ++++++++------------- atst/routes/requests/requests_form.py | 2 +- tests/domain/test_requests.py | 6 +++--- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/atst/domain/requests.py b/atst/domain/requests.py index c4b29cd9..891266ed 100644 --- a/atst/domain/requests.py +++ b/atst/domain/requests.py @@ -42,11 +42,14 @@ class Requests(object): @classmethod def exists(cls, request_id, creator_id): - return db.session.query( - exists().where( - and_(Request.id == request_id, Request.creator == creator_id) - ) - ).scalar() + try: + return db.session.query( + exists().where( + and_(Request.id == request_id, Request.creator == creator_id) + ) + ).scalar() + except exc.DataError: + return False @classmethod def get(cls, request_id): @@ -143,11 +146,3 @@ class Requests(object): return request.status == "incomplete" and all( section in existing_request_sections for section in all_request_sections ) - - @classmethod - def is_creator(cls, request_id, user_id): - try: - db.session.query(Request).filter_by(id=request_id, creator=user_id).one() - return True - except (NoResultFound, exc.DataError): - return False diff --git a/atst/routes/requests/requests_form.py b/atst/routes/requests/requests_form.py index ee03c155..ad415775 100644 --- a/atst/routes/requests/requests_form.py +++ b/atst/routes/requests/requests_form.py @@ -111,7 +111,7 @@ def requests_submit(request_id=None): def _check_can_view_request(request_id): if Permissions.REVIEW_AND_APPROVE_JEDI_WORKSPACE_REQUEST in g.current_user.atat_permissions: pass - elif Requests.is_creator(request_id, g.current_user.id): + elif Requests.exists(request_id, g.current_user.id): pass else: raise UnauthorizedError(g.current_user, "view request {}".format(request_id)) diff --git a/tests/domain/test_requests.py b/tests/domain/test_requests.py index 609dae90..d82c8ce4 100644 --- a/tests/domain/test_requests.py +++ b/tests/domain/test_requests.py @@ -50,9 +50,9 @@ def test_dont_auto_approve_if_no_dollar_value_specified(new_request): assert request.status == RequestStatus.PENDING_CCPO_APPROVAL -def test_can_check_if_user_created_request(session): +def test_exists(session): user_allowed = UserFactory.create() user_denied = UserFactory.create() request = RequestFactory.create(creator=user_allowed.id) - assert Requests.is_creator(request.id, user_allowed.id) - assert not Requests.is_creator(request.id, user_denied.id) + assert Requests.exists(request.id, user_allowed.id) + assert not Requests.exists(request.id, user_denied.id) From dec8d920762e905b6ca395e2e9d2448bf4199b88 Mon Sep 17 00:00:00 2001 From: dandds Date: Wed, 8 Aug 2018 14:45:51 -0400 Subject: [PATCH 7/7] update tests to rely on newest RequestFactory --- tests/routes/test_request_new.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/routes/test_request_new.py b/tests/routes/test_request_new.py index 6a0ccb86..e402e622 100644 --- a/tests/routes/test_request_new.py +++ b/tests/routes/test_request_new.py @@ -2,7 +2,7 @@ import re import pytest import urllib from tests.mocks import MOCK_USER, MOCK_REQUEST -from tests.factories import RequestFactory, UserFactory, RequestStatusEventFactory +from tests.factories import RequestFactory, UserFactory from atst.domain.roles import Roles @@ -34,7 +34,6 @@ def test_owner_can_view_request(client, user_session): user = UserFactory.create() user_session(user) request = RequestFactory.create(creator=user.id) - status = RequestStatusEventFactory.create(request_id=request.id) response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) @@ -45,7 +44,6 @@ def test_non_owner_cannot_view_request(client, user_session): user = UserFactory.create() user_session(user) request = RequestFactory.create() - status = RequestStatusEventFactory.create(request_id=request.id) response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True) @@ -57,7 +55,6 @@ def test_ccpo_can_view_request(client, user_session): user = UserFactory.create(atat_role=ccpo) user_session(user) request = RequestFactory.create() - status = RequestStatusEventFactory.create(request_id=request.id) response = client.get("/requests/new/1/{}".format(request.id), follow_redirects=True)