From f1845c3227f69e1054193a11f710e1c9fa1f072a Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Mon, 15 Jan 2018 13:36:29 -0600 Subject: [PATCH] Add JournalEntries when adding/removing roles --- tests/unit/manage/test_views.py | 73 +++++++++++++++++++++++++++++++-- warehouse/manage/views.py | 60 ++++++++++++++++++++++++--- 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/tests/unit/manage/test_views.py b/tests/unit/manage/test_views.py index 831c4205e6d2..cdb2c632de98 100644 --- a/tests/unit/manage/test_views.py +++ b/tests/unit/manage/test_views.py @@ -19,7 +19,7 @@ from warehouse.manage import views from warehouse.accounts.interfaces import IUserService -from warehouse.packaging.models import Role +from warehouse.packaging.models import JournalEntry, Role from ...common.db.packaging import ProjectFactory, RoleFactory, UserFactory @@ -125,6 +125,8 @@ def test_post_new_role(self, db_request): ) db_request.method = "POST" db_request.POST = pretend.stub() + db_request.remote_addr = "10.10.10.10" + db_request.user = UserFactory.create() form_obj = pretend.stub( validate=pretend.call_recorder(lambda: True), username=pretend.stub(data=user.username), @@ -160,6 +162,13 @@ def test_post_new_role(self, db_request): "form": form_obj, } + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "add Owner testuser" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + def test_post_duplicate_role(self, db_request): project = ProjectFactory.create(name="foobar") user = UserFactory.create(username="testuser") @@ -226,7 +235,8 @@ def test_change_role(self, db_request): new_role_name = "Maintainer" db_request.method = "POST" - db_request.user = pretend.stub() + db_request.user = UserFactory.create() + db_request.remote_addr = "10.10.10.10" db_request.POST = MultiDict({ "role_id": role.id, "role_name": new_role_name, @@ -250,6 +260,13 @@ def test_change_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "change Owner testuser to Maintainer" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + def test_change_role_invalid_role_name(self, pyramid_request): project = pretend.stub(name="foobar") @@ -282,7 +299,8 @@ def test_change_role_when_multiple(self, db_request): new_role_name = "Maintainer" db_request.method = "POST" - db_request.user = pretend.stub() + db_request.user = UserFactory.create() + db_request.remote_addr = "10.10.10.10" db_request.POST = MultiDict([ ("role_id", owner_role.id), ("role_id", maintainer_role.id), @@ -307,6 +325,13 @@ def test_change_role_when_multiple(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "remove Owner testuser" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + def test_change_missing_role(self, db_request): project = ProjectFactory.create(name="foobar") missing_role_id = str(uuid.uuid4()) @@ -360,6 +385,38 @@ def test_change_own_owner_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + def test_change_own_owner_role_when_multiple(self, db_request): + project = ProjectFactory.create(name="foobar") + user = UserFactory.create(username="testuser") + owner_role = RoleFactory.create( + user=user, project=project, role_name="Owner" + ) + maintainer_role = RoleFactory.create( + user=user, project=project, role_name="Maintainer" + ) + + db_request.method = "POST" + db_request.user = user + db_request.POST = MultiDict([ + ("role_id", owner_role.id), + ("role_id", maintainer_role.id), + ("role_name", "Maintainer"), + ]) + db_request.session = pretend.stub( + flash=pretend.call_recorder(lambda *a, **kw: None), + ) + db_request.route_path = pretend.call_recorder( + lambda *a, **kw: "/the-redirect" + ) + + result = views.change_project_role(project, db_request) + + assert db_request.session.flash.calls == [ + pretend.call("Cannot remove yourself as Owner", queue="error"), + ] + assert isinstance(result, HTTPSeeOther) + assert result.headers["Location"] == "/the-redirect" + class TestDeleteProjectRoles: @@ -371,7 +428,8 @@ def test_delete_role(self, db_request): ) db_request.method = "POST" - db_request.user = pretend.stub() + db_request.user = UserFactory.create() + db_request.remote_addr = "10.10.10.10" db_request.POST = MultiDict({"role_id": role.id}) db_request.session = pretend.stub( flash=pretend.call_recorder(lambda *a, **kw: None), @@ -392,6 +450,13 @@ def test_delete_role(self, db_request): assert isinstance(result, HTTPSeeOther) assert result.headers["Location"] == "/the-redirect" + entry = db_request.db.query(JournalEntry).one() + + assert entry.name == project.name + assert entry.action == "remove Owner testuser" + assert entry.submitted_by == db_request.user + assert entry.submitted_from == db_request.remote_addr + def test_delete_missing_role(self, db_request): project = ProjectFactory.create(name="foobar") missing_role_id = str(uuid.uuid4()) diff --git a/warehouse/manage/views.py b/warehouse/manage/views.py index 9eae7d15b351..a1e96159a214 100644 --- a/warehouse/manage/views.py +++ b/warehouse/manage/views.py @@ -1,4 +1,5 @@ # Licensed under the Apache License, Version 2.0 (the "License"); + # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # @@ -20,7 +21,7 @@ from warehouse.accounts.interfaces import IUserService from warehouse.accounts.models import User from warehouse.manage.forms import CreateRoleForm, ChangeRoleForm -from warehouse.packaging.models import Role +from warehouse.packaging.models import JournalEntry, Role @view_config( @@ -86,6 +87,14 @@ def manage_project_roles(project, request, _form_class=CreateRoleForm): request.db.add( Role(user=user, project=project, role_name=form.role_name.data) ) + request.db.add( + JournalEntry( + name=project.name, + action=f"add {role_name} {username}", + submitted_by=request.user, + submitted_from=request.remote_addr, + ), + ) request.session.flash( f"Added collaborator '{form.username.data}'", queue="success" @@ -131,19 +140,38 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): # This user has more than one role, so just delete all the ones # that aren't what we want. # - # This should be removed when fixing GH-2745. - ( + # This branch should be removed when fixing GH-2745. + roles = ( request.db.query(Role) .filter( Role.id.in_(role_ids), Role.project == project, Role.role_name != form.role_name.data ) - .delete(synchronize_session="fetch") + .all() ) - request.session.flash( - 'Successfully changed role', queue="success" + removing_self = any( + role.role_name == "Owner" and role.user == request.user + for role in roles ) + if removing_self: + request.session.flash( + "Cannot remove yourself as Owner", queue="error" + ) + else: + for role in roles: + request.db.delete(role) + request.db.add( + JournalEntry( + name=project.name, + action=f"remove {role.role_name} {role.user_name}", + submitted_by=request.user, + submitted_from=request.remote_addr, + ), + ) + request.session.flash( + 'Successfully changed role', queue="success" + ) else: # This user only has one role, so get it and change the type. try: @@ -160,6 +188,18 @@ def change_project_role(project, request, _form_class=ChangeRoleForm): "Cannot remove yourself as Owner", queue="error" ) else: + request.db.add( + JournalEntry( + name=project.name, + action="change {} {} to {}".format( + role.role_name, + role.user_name, + form.role_name.data, + ), + submitted_by=request.user, + submitted_from=request.remote_addr, + ), + ) role.role_name = form.role_name.data request.session.flash( 'Successfully changed role', queue="success" @@ -202,6 +242,14 @@ def delete_project_role(project, request): else: for role in roles: request.db.delete(role) + request.db.add( + JournalEntry( + name=project.name, + action=f"remove {role.role_name} {role.user_name}", + submitted_by=request.user, + submitted_from=request.remote_addr, + ), + ) request.session.flash("Successfully removed role", queue="success") return HTTPSeeOther(