copr-devel March 2013

copr-devel@lists.fedorahosted.org

10 participants
39 discussions

Discussion: Using copr for building SCLs
by Bohuslav Kabrda 26 Jun '13

26 Jun '13

Hi list, I'd like to start yet another discussion about some functionality of copr, here it is: One of nice use cases for copr would be building software collections [1]. These however require modified mock chroot (modified config_opts['root'] and config_opts['chroot_setup_cmd']). I'm trying to think of a way how to do this: - We would need to allow users to create their own chroots (or more precisely allow them to do certain modifications). - We would need to be able to send these chroots from frontend to backend. - We would need some mechanism for sane naming, so that we can distinguish between builds of collections for different releases and architectures. E.g. ruby193-x86_64 is not enough, we need to also store the information about os name (epel/fedora) and os version (epel 6, fedora 18, ...). My proposal: - Use chroot names like ruby193_fedora-18-x86_64. - Allow users to create these chroots by modifying config_opts['chroot_setup_cmd'] (config_opts['root'] would be autogenerated from the chroot name). - Store the modified information in the MockChroot table and create these on backend using communication channel proposed in [2] (parts of it already implemented in frontend). - Allow selecting these on copr creation/modification as more options for mock chroots, by default hidden under some kind of fancy clickable JS dropdown. Thoughts? -- Regards, Bohuslav "Slavek" Kabrda. [1] https://fedorahosted.org/SoftwareCollections/ [2] https://fedorahosted.org/copr/ticket/8

3 8

[copr] master: Enable raising legal flag on a copr. Fixes #46 - now make it show up in admin interface. (2aab1bf)
by bkabrda＠fedoraproject.org 29 Mar '13

29 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 2aab1bfaaa82ade6a7fcbaf621dab209371eae90 Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Fri Mar 29 14:14:52 2013 +0100 Enable raising legal flag on a copr. Fixes #46 - now make it show up in admin interface. >--------------------------------------------------------------- coprs_frontend/coprs/forms.py | 2 ++ coprs_frontend/coprs/helpers.py | 2 +- coprs_frontend/coprs/static/copr.css | 10 ++++++++++ .../coprs/templates/coprs/_coprs_forms.html | 14 ++++++++++++++ .../coprs/templates/coprs/detail/overview.html | 6 ++++++ .../coprs/views/coprs_ns/coprs_general.py | 19 ++++++++++++++++--- .../tests/test_logic/test_coprs_logic.py | 10 +++++++++- 7 files changed, 58 insertions(+), 5 deletions(-) diff --git a/coprs_frontend/coprs/forms.py b/coprs_frontend/coprs/forms.py index 7c7cf35..6b533cd 100644 --- a/coprs_frontend/coprs/forms.py +++ b/coprs_frontend/coprs/forms.py @@ -138,6 +138,8 @@ class BuildForm(wtf.Form): validators = [wtf.NumberRange(min = constants.MIN_BUILD_TIMEOUT, max = constants.MAX_BUILD_TIMEOUT)], default = constants.DEFAULT_BUILD_TIMEOUT) +class CoprLegalForm(wtf.Form): + comment = wtf.TextAreaField('Comment') class PermissionsApplierFormFactory(object): @staticmethod diff --git a/coprs_frontend/coprs/helpers.py b/coprs_frontend/coprs/helpers.py index fdc87af..cad7324 100644 --- a/coprs_frontend/coprs/helpers.py +++ b/coprs_frontend/coprs/helpers.py @@ -37,7 +37,7 @@ class PermissionEnum(object): class ActionTypeEnum(object): __metaclass__ = EnumType - vals = {'delete': 0, 'rename': 1} + vals = {'delete': 0, 'rename': 1, 'legal-flag': 2} class BackendResultEnum(object): __metaclass__ = EnumType diff --git a/coprs_frontend/coprs/static/copr.css b/coprs_frontend/coprs/static/copr.css index a2349a7..243b676 100644 --- a/coprs_frontend/coprs/static/copr.css +++ b/coprs_frontend/coprs/static/copr.css @@ -311,3 +311,13 @@ table.releases th.rightmost { table.releases tr.release-end { border-bottom: 3px solid #f2f2f2; } + +form.legal-flag, form.legal-flag input { + color: #888888; + margin-top: 5px; +} + +hr { + margin-top: 25px; + margin-bottom: 25px; +} diff --git a/coprs_frontend/coprs/templates/coprs/_coprs_forms.html b/coprs_frontend/coprs/templates/coprs/_coprs_forms.html index a0f55e3..e8856e1 100644 --- a/coprs_frontend/coprs/templates/coprs/_coprs_forms.html +++ b/coprs_frontend/coprs/templates/coprs/_coprs_forms.html @@ -75,4 +75,18 @@ <dd><input type="submit" value="Save"></dd> </form> {% endif %} + {% endmacro %} + +{% macro copr_legal_form(form, copr) %} +<form class="legal-flag" method="post" action={{ url_for('coprs_ns.copr_legal_flag', username=copr.owner.name, coprname=copr.name) }}> + If you think that this copr may be violating our policies, you can mark it for legal review. + You can add an optional comment describing what you think is wrong here. + + <dl> + {{ render_field(form.comment, rows=5, cols=50) }} + <dt> + <input type="submit" value="Raise Legal Flag"> + </dt> + </dl> + </form> {% endmacro %} diff --git a/coprs_frontend/coprs/templates/coprs/detail/overview.html b/coprs_frontend/coprs/templates/coprs/detail/overview.html index 6152ddf..b0bf4e2 100644 --- a/coprs_frontend/coprs/templates/coprs/detail/overview.html +++ b/coprs_frontend/coprs/templates/coprs/detail/overview.html @@ -1,4 +1,7 @@ {% extends "coprs/detail.html" %} + +{% from "coprs/_coprs_forms.html" import copr_legal_form with context %} + {% block overview_selected %}selected{% endblock %} {% block detail_body %} @@ -46,4 +49,7 @@ {% endfor %} </ul> {% endif %} + + <hr> + {{ copr_legal_form(form, copr) }} {% endblock %} diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py index cce699a..8c63eee 100644 --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py @@ -104,13 +104,15 @@ def copr_new(): @coprs_ns.route('/detail/<username>/<coprname>/') def copr_detail(username, coprname): query = coprs_logic.CoprsLogic.get(flask.g.user, username, coprname, with_mock_chroots=True) + form = forms.CoprLegalForm() try: copr = query.one() except sqlalchemy.orm.exc.NoResultFound: return page_not_found('Copr with name {0} does not exist.'.format(coprname)) return flask.render_template('coprs/detail/overview.html', - copr=copr) + copr=copr, + form=form) @coprs_ns.route('/detail/<username>/<coprname>/permissions/') def copr_permissions(username, coprname): @@ -256,7 +258,18 @@ def copr_delete(username, coprname): else: return flask.render_template('coprs/detail/delete.html', form=form, copr=copr) -(a)coprs_ns.route('/detail/<username>/<coprname>/legal_flag/') +(a)coprs_ns.route('/detail/<username>/<coprname>/legal_flag/', methods=['POST']) @login_required def copr_legal_flag(username, coprname): - pass + form = forms.CoprLegalForm() + copr = coprs_logic.CoprsLogic.get(flask.g.user, username, coprname).first() + + action = models.Action(action_type=helpers.ActionTypeEnum('legal-flag'), + object_type='copr', + object_id=copr.id, + message=form.comment.data, + created_on=int(time.time())) + db.session.add(action) + db.session.commit() + flask.flash('Admin was noticed about your report and will investigate the copr shortly.') + return flask.redirect(flask.url_for('coprs_ns.copr_detail', username=username, coprname=coprname)) diff --git a/coprs_frontend/tests/test_logic/test_coprs_logic.py b/coprs_frontend/tests/test_logic/test_coprs_logic.py index c3f365b..2f6d7d8 100644 --- a/coprs_frontend/tests/test_logic/test_coprs_logic.py +++ b/coprs_frontend/tests/test_logic/test_coprs_logic.py @@ -1,7 +1,7 @@ import pytest from coprs.exceptions import ActionInProgressException -from coprs.helpers import BackendResultEnum +from coprs.helpers import ActionTypeEnum, BackendResultEnum from coprs.logic.coprs_logic import CoprsLogic from tests.coprs_test_case import CoprsTestCase @@ -12,3 +12,11 @@ class TestCoprsLogic(CoprsTestCase): with pytest.raises(ActionInProgressException): CoprsLogic.update(self.u1, self.c1) self.db.session.rollback() + + def test_legal_flag_doesnt_block_copr_functionality(self, f_users, f_coprs, f_db): + self.db.session.add(self.models.Action(object_type='copr', + object_id=self.c1.id, + action_type=ActionTypeEnum('legal-flag'))) + self.db.session.commit() + # test will fail if this raises exception + CoprsLogic.raise_if_unfinished_blocking_action(None, self.c1, 'ha, failed')

1 0

[copr] master: Generalize Action to be usable for different scenarios (42bdee5)
by bkabrda＠fedoraproject.org 29 Mar '13

29 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 42bdee5b1752ada33eb2f8b6af4bdc396d311201 Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Fri Mar 29 12:20:57 2013 +0100 Generalize Action to be usable for different scenarios - rename backend_message to message - add ended_on column >--------------------------------------------------------------- .../versions/451e9507b866_generalize_action.py | 30 ++++++++++++++++++++ coprs_frontend/coprs/logic/builds_logic.py | 2 +- coprs_frontend/coprs/logic/coprs_logic.py | 15 ++++++---- coprs_frontend/coprs/models.py | 8 +++-- .../coprs/views/backend_ns/backend_general.py | 5 ++- .../coprs/views/coprs_ns/coprs_general.py | 5 +++ .../test_backend_ns/test_backend_general.py | 21 +++++++++---- 7 files changed, 67 insertions(+), 19 deletions(-) diff --git a/coprs_frontend/alembic/versions/451e9507b866_generalize_action.py b/coprs_frontend/alembic/versions/451e9507b866_generalize_action.py new file mode 100644 index 0000000..203f4cc --- /dev/null +++ b/coprs_frontend/alembic/versions/451e9507b866_generalize_action.py @@ -0,0 +1,30 @@ +"""generalize_action + +Revision ID: 451e9507b866 +Revises: 2a75f0a06d90 +Create Date: 2013-03-29 12:13:33.303584 + +""" + +# revision identifiers, used by Alembic. +revision = '451e9507b866' +down_revision = '2a75f0a06d90' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('action', sa.Column('message', sa.Text(), nullable=True)) + op.add_column('action', sa.Column('ended_on', sa.Integer(), nullable=True)) + op.drop_column('action', u'backend_message') + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('action', sa.Column(u'backend_message', sa.TEXT(), nullable=True)) + op.drop_column('action', 'ended_on') + op.drop_column('action', 'message') + ### end Alembic commands ### diff --git a/coprs_frontend/coprs/logic/builds_logic.py b/coprs_frontend/coprs/logic/builds_logic.py index 834217d..bbcd639 100644 --- a/coprs_frontend/coprs/logic/builds_logic.py +++ b/coprs_frontend/coprs/logic/builds_logic.py @@ -58,7 +58,7 @@ class BuildsLogic(object): @classmethod def add(cls, user, pkgs, copr): - coprs_logic.CoprsLogic.raise_if_unfinished_action(user, copr, + coprs_logic.CoprsLogic.raise_if_unfinished_blocking_action(user, copr, 'Can\'t build while there is an operation in progress: {action}') users_logic.UsersLogic.raise_if_cant_build_in_copr(user, copr, 'You don\'t have permissions to build in this copr.') diff --git a/coprs_frontend/coprs/logic/coprs_logic.py b/coprs_frontend/coprs/logic/coprs_logic.py index 663c5cd..e768ae0 100644 --- a/coprs_frontend/coprs/logic/coprs_logic.py +++ b/coprs_frontend/coprs/logic/coprs_logic.py @@ -93,7 +93,7 @@ class CoprsLogic(object): @classmethod def update(cls, user, copr, check_for_duplicates = True): - cls.raise_if_unfinished_action(user, copr, + cls.raise_if_unfinished_blocking_action(user, copr, 'Can\'t change this Copr name, another operation is in progress: {action}') users_logic.UsersLogic.raise_if_cant_update_copr(user, copr, 'Only owners and admins may update their Coprs.') @@ -120,7 +120,7 @@ class CoprsLogic(object): if not copr.owner == user: raise exceptions.InsufficientRightsException('Only owners may delete their Coprs.') # TODO: do we want to dump the information somewhere, so that we can search it in future? - cls.raise_if_unfinished_action(user, copr, + cls.raise_if_unfinished_blocking_action(user, copr, 'Can\'t delete this Copr, another operation is in progress: {action}') action = models.Action(action_type=helpers.ActionTypeEnum('delete'), object_type='copr', @@ -147,19 +147,22 @@ class CoprsLogic(object): update({models.Copr.build_count: models.Copr.build_count + 1}) @classmethod - def unfinished_actions_for(cls, user, copr): + def unfinished_blocking_actions_for(cls, user, copr): + blocking_actions = [helpers.ActionTypeEnum('rename'), + helpers.ActionTypeEnum('delete')] actions = models.Action.query.filter(models.Action.object_type=='copr').\ filter(models.Action.object_id==copr.id).\ - filter(models.Action.backend_result==helpers.BackendResultEnum('waiting')) + filter(models.Action.backend_result==helpers.BackendResultEnum('waiting')).\ + filter(models.Action.action_type.in_(blocking_actions)) return actions @classmethod - def raise_if_unfinished_action(cls, user, copr, message): + def raise_if_unfinished_blocking_action(cls, user, copr, message): """This method raises ActionInProgressException if given copr has an unfinished action. Returns None otherwise. """ - unfinished_actions = cls.unfinished_actions_for(user, copr).all() + unfinished_actions = cls.unfinished_blocking_actions_for(user, copr).all() if unfinished_actions: raise exceptions.ActionInProgressException(message, unfinished_actions[0]) diff --git a/coprs_frontend/coprs/models.py b/coprs_frontend/coprs/models.py index 22e8efe..c092e53 100644 --- a/coprs_frontend/coprs/models.py +++ b/coprs_frontend/coprs/models.py @@ -269,7 +269,7 @@ class CoprChroot(db.Model, Serializer): cascade='all,delete,delete-orphan')) class Action(db.Model, Serializer): - """Representation of a custom action that needs backends cooperation""" + """Representation of a custom action that needs backends cooperation/admin attention/...""" id = db.Column(db.Integer, primary_key=True) # delete, rename, ...; see helpers.ActionTypeEnum action_type = db.Column(db.Integer, nullable=False) @@ -282,10 +282,12 @@ class Action(db.Model, Serializer): new_value = db.Column(db.String(255)) # backend result, see helpers.BackendResultEnum backend_result = db.Column(db.Integer, default=helpers.BackendResultEnum('waiting')) - # optional message from the backend - backend_message = db.Column(db.Text) + # optional message from the backend/whatever + message = db.Column(db.Text) # time created as returned by int(time.time()) created_on = db.Column(db.Integer) + # time ended as returned by int(time.time()) + ended_on = db.Column(db.Integer) def __str__(self): return self.__unicode__() diff --git a/coprs_frontend/coprs/views/backend_ns/backend_general.py b/coprs_frontend/coprs/views/backend_ns/backend_general.py index 7be60a2..1f9c4f1 100644 --- a/coprs_frontend/coprs/views/backend_ns/backend_general.py +++ b/coprs_frontend/coprs/views/backend_ns/backend_general.py @@ -46,7 +46,7 @@ def update_builds(): def waiting_actions(): actions = models.Action.query.filter(models.Action.backend_result==helpers.BackendResultEnum('waiting')).all() - return flask.jsonify({'actions': [action.to_dict(options={'__columns_except__': ['backend_result', 'backend_message']}) for action in actions]}) + return flask.jsonify({'actions': [action.to_dict(options={'__columns_except__': ['backend_result', 'message', 'ended_on']}) for action in actions]}) # TODO: this is very similar to update_builds, we should pull out the common functionality into a single function @backend_ns.route('/update_actions/', methods=['POST', 'PUT']) @@ -67,7 +67,8 @@ def update_actions(): for i, action in existing.items(): existing[i].backend_result = to_update[i]['backend_result'] - existing[i].backend_message = to_update[i]['backend_message'] + existing[i].message = to_update[i]['message'] + existing[i].ended_on = to_update[i]['ended_on'] db.session.commit() diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py index f5d84fe..cce699a 100644 --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py @@ -255,3 +255,8 @@ def copr_delete(username, coprname): return flask.redirect(flask.url_for('coprs_ns.coprs_by_owner', username=username)) else: return flask.render_template('coprs/detail/delete.html', form=form, copr=copr) + +(a)coprs_ns.route('/detail/<username>/<coprname>/legal_flag/') +@login_required +def copr_legal_flag(username, coprname): + pass diff --git a/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py b/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py index 5b021d8..2d3270c 100644 --- a/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py +++ b/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py @@ -143,7 +143,8 @@ class TestUpdateActions(CoprsTestCase): { "id": 1, "backend_result": 1, - "backend_message": "no problem" + "message": "no problem", + "ended_on": 123 } ] }""" @@ -153,17 +154,20 @@ class TestUpdateActions(CoprsTestCase): { "id": 1, "backend_result": 1, - "backend_message": null + "message": null, + "ended_on": 1234 }, { "id": 2, "backend_result": 2, - "backend_message": "problem!" + "message": "problem!", + "ended_on": 12345 }, { "id": 100, "backend_result": 123, - "backend_message": "wheeeee!" + "message": "wheeeee!", + "ended_on": 123456 } ] }""" @@ -178,7 +182,8 @@ class TestUpdateActions(CoprsTestCase): updated = self.models.Action.query.filter(self.models.Action.id==1).first() assert updated.backend_result == 1 - assert updated.backend_message == "no problem" + assert updated.message == "no problem" + assert updated.ended_on == 123 def test_update_more_existent_and_non_existent_builds(self, f_users, f_coprs, f_actions, f_db): r = self.tc.post('/backend/update_actions/', @@ -190,8 +195,10 @@ class TestUpdateActions(CoprsTestCase): updated = self.models.Action.query.filter(self.models.Action.id==1).first() assert updated.backend_result == 1 - assert updated.backend_message == None + assert updated.message == None + assert updated.ended_on == 1234 updated2 = self.models.Action.query.filter(self.models.Action.id==2).first() assert updated2.backend_result == 2 - assert updated2.backend_message == "problem!" + assert updated2.message == "problem!" + assert updated2.ended_on == 12345

1 0

[copr] master: Fix #60 - copr admin can now give up his permissions even if there are permissions for more users for the copr (8fb8188)
by bkabrda＠fedoraproject.org 29 Mar '13

29 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 8fb8188a50af94855b0767f9bfddf63002c34efe Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Fri Mar 29 10:01:20 2013 +0100 Fix #60 - copr admin can now give up his permissions even if there are permissions for more users for the copr - also added flexmock to requirements >--------------------------------------------------------------- .../coprs/views/coprs_ns/coprs_general.py | 3 ++ .../test_views/test_coprs_ns/test_coprs_general.py | 25 ++++++++++++++++++++ requirements.txt | 1 + 3 files changed, 29 insertions(+), 0 deletions(-) diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py index 1b8353b..f5d84fe 100644 --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py @@ -219,6 +219,9 @@ def copr_update_permissions(username, coprname): if permissions_form.validate_on_submit(): # we don't change owner (yet) try: + # if admin is changing his permissions, his must be changed last + # so that we don't get InsufficientRightsException + permissions.sort(cmp=lambda x, y: -1 if y.user_id == flask.g.user.id else 1) for perm in permissions: new_builder = permissions_form['copr_builder_{0}'.format(perm.user_id)].data new_admin = permissions_form['copr_admin_{0}'.format(perm.user_id)].data diff --git a/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py b/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py index 63f3a06..6fa8e19 100644 --- a/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py +++ b/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py @@ -1,5 +1,7 @@ import flask +from flexmock import flexmock + from coprs.signals import copr_created from tests.coprs_test_case import CoprsTestCase @@ -371,6 +373,29 @@ class TestCoprUpdatePermissions(CoprsTestCase): follow_redirects = True) assert 'Copr permissions were updated' in r.data + def test_copr_admin_can_give_up_his_permissions(self, f_users, f_coprs, f_copr_permissions, f_db): + # if admin is giving up his permission and there are more permissions for + # this copr, then if the admin is altered first, he won't be permitted + # to alter the other permissions and the whole update would fail + with self.tc as c: + with c.session_transaction() as s: + s['openid'] = self.u1.openid_name + self.db.session.add_all([self.u2, self.c3, self.cp2, self.cp3]) + # mock out the order of CoprPermission objects, so that we are sure + # the admin is the first one and therefore this fails if + # the view doesn't reorder the permissions + flexmock(self.models.Copr, copr_permissions=[self.cp3, self.cp2]) + r = c.post('/coprs/detail/{0}/{1}/update_permissions/'.format(self.u2.name, self.c3.name), + data = {'copr_admin_1': '1', 'copr_admin_3': '1'}, + follow_redirects = True) + self.db.session.add_all([self.u1, self.c3]) + perm = self.models.CoprPermission.query.filter(self.models.CoprPermission.user_id==self.u1.id).\ + filter(self.models.CoprPermission.copr_id==self.c3.id).\ + first() + assert perm.copr_admin == 1 + assert 'Copr permissions were updated' in r.data + + class TestCoprDelete(CoprsTestCase): def test_delete(self, f_users, f_coprs, f_db): with self.tc as c: diff --git a/requirements.txt b/requirements.txt index d9b8d1f..9094607 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,4 +8,5 @@ flask-openid flask-script flask-whooshee flask-wtf +flexmock pytest

1 0

[copr] master: Fix import and indentation in users_logic. (9c1f582)
by bkabrda＠fedoraproject.org 29 Mar '13

29 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 9c1f582f1abb8ba6c5853a83382e093d2418313d Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Fri Mar 29 08:32:38 2013 +0100 Fix import and indentation in users_logic. >--------------------------------------------------------------- coprs_frontend/coprs/logic/users_logic.py | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/coprs_frontend/coprs/logic/users_logic.py b/coprs_frontend/coprs/logic/users_logic.py index 3f6d048..13289b6 100644 --- a/coprs_frontend/coprs/logic/users_logic.py +++ b/coprs_frontend/coprs/logic/users_logic.py @@ -1,4 +1,4 @@ -import exceptions +from coprs import exceptions class UsersLogic(object): @classmethod @@ -12,8 +12,8 @@ class UsersLogic(object): @classmethod def raise_if_cant_build_in_copr(cls, user, copr, message): - """This method raises InsufficientRightsException if given user cant build in - given copr. Returns None otherwise. - """ - if not user.can_build_in(copr): - raise exceptions.InsufficientRightsException(message) + """This method raises InsufficientRightsException if given user cant build in + given copr. Returns None otherwise. + """ + if not user.can_build_in(copr): + raise exceptions.InsufficientRightsException(message)

1 0

[copr] master: Start implementing admin section in frontend (604b0ff)
by bkabrda＠fedoraproject.org 28 Mar '13

28 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 604b0ff75466a076bf9b8b5dcc20bde5fa1cd088 Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Thu Mar 28 15:46:04 2013 +0100 Start implementing admin section in frontend >--------------------------------------------------------------- coprs_frontend/coprs/__init__.py | 3 ++ coprs_frontend/coprs/helpers.py | 4 ++ coprs_frontend/coprs/templates/admin/index.html | 5 +++ coprs_frontend/coprs/templates/admin/layout.html | 8 +++++ coprs_frontend/coprs/views/admin_ns/__init__.py | 3 ++ .../coprs/views/admin_ns/admin_general.py | 11 +++++++ coprs_frontend/coprs/views/misc.py | 30 ++++++++++++++------ coprs_frontend/tests/coprs_test_case.py | 6 ++-- .../test_views/test_admin/test_admin_general.py | 20 +++++++++++++ 9 files changed, 78 insertions(+), 12 deletions(-) diff --git a/coprs_frontend/coprs/__init__.py b/coprs_frontend/coprs/__init__.py index 6a9290c..103d7e1 100644 --- a/coprs_frontend/coprs/__init__.py +++ b/coprs_frontend/coprs/__init__.py @@ -25,6 +25,8 @@ import coprs.log import coprs.models import coprs.whoosheers +from coprs.views import admin_ns +from coprs.views.admin_ns import admin_general from coprs.views import api_ns from coprs.views.api_ns import api_general from coprs.views import coprs_ns @@ -35,6 +37,7 @@ from coprs.views.backend_ns import backend_general from coprs.views import misc app.register_blueprint(api_ns.api_ns) +app.register_blueprint(admin_ns.admin_ns) app.register_blueprint(coprs_ns.coprs_ns) app.register_blueprint(misc.misc) app.register_blueprint(backend_ns.backend_ns) diff --git a/coprs_frontend/coprs/helpers.py b/coprs_frontend/coprs/helpers.py index 15e942e..fdc87af 100644 --- a/coprs_frontend/coprs/helpers.py +++ b/coprs_frontend/coprs/helpers.py @@ -43,6 +43,10 @@ class BackendResultEnum(object): __metaclass__ = EnumType vals = {'waiting': 0, 'success': 1, 'failure': 2} +class RoleEnum(object): + __metaclass__ = EnumType + vals = {'user': 0, 'admin': 1} + class Paginator(object): def __init__(self, query, total_count, page = 1, per_page_override = None, urls_count_override = None): self.query = query diff --git a/coprs_frontend/coprs/templates/admin/index.html b/coprs_frontend/coprs/templates/admin/index.html new file mode 100644 index 0000000..25562a2 --- /dev/null +++ b/coprs_frontend/coprs/templates/admin/index.html @@ -0,0 +1,5 @@ +{% extends "admin/layout.html" %} + +{% block admin_body %} +Admin body +{% endblock %} diff --git a/coprs_frontend/coprs/templates/admin/layout.html b/coprs_frontend/coprs/templates/admin/layout.html new file mode 100644 index 0000000..336e077 --- /dev/null +++ b/coprs_frontend/coprs/templates/admin/layout.html @@ -0,0 +1,8 @@ +{% extends "layout.html" %} + +{% block title %}Coprs - Admin{% endblock %} + +{% block body %} +Admin menu +{% block admin_body %}{% endblock %} +{% endblock %} diff --git a/coprs_frontend/coprs/views/admin_ns/__init__.py b/coprs_frontend/coprs/views/admin_ns/__init__.py new file mode 100644 index 0000000..b7f5426 --- /dev/null +++ b/coprs_frontend/coprs/views/admin_ns/__init__.py @@ -0,0 +1,3 @@ +import flask + +admin_ns = flask.Blueprint('admin_ns', __name__, url_prefix = '/admin') diff --git a/coprs_frontend/coprs/views/admin_ns/admin_general.py b/coprs_frontend/coprs/views/admin_ns/admin_general.py new file mode 100644 index 0000000..8252c5e --- /dev/null +++ b/coprs_frontend/coprs/views/admin_ns/admin_general.py @@ -0,0 +1,11 @@ +import flask + +from coprs import helpers + +from coprs.views.admin_ns import admin_ns +from coprs.views.misc import login_required + +(a)admin_ns.route('/') +(a)login_required(role=helpers.RoleEnum('admin')) +def admin_index(): + return flask.render_template('admin/index.html') diff --git a/coprs_frontend/coprs/views/misc.py b/coprs_frontend/coprs/views/misc.py index 1c5a47c..7ffad6b 100644 --- a/coprs_frontend/coprs/views/misc.py +++ b/coprs_frontend/coprs/views/misc.py @@ -107,15 +107,27 @@ def api_login_required(f): return decorated_function -def login_required(f): - @functools.wraps(f) - def decorated_function(*args, **kwargs): - if flask.g.user is None: - return flask.redirect(flask.url_for('misc.login', - next = flask.request.url)) - return f(*args, **kwargs) - return decorated_function - +def login_required(role=helpers.RoleEnum('user')): + def view_wrapper(f): + @functools.wraps(f) + def decorated_function(*args, **kwargs): + if flask.g.user is None: + return flask.redirect(flask.url_for('misc.login', + next = flask.request.url)) + if role == helpers.RoleEnum('admin') and not flask.g.user.admin: + flask.flash('You are not allowed to access admin section.') + return flask.redirect(flask.url_for('coprs_ns.coprs_show')) + return f(*args, **kwargs) + return decorated_function + # hack: if login_required is used without params, the "role" parameter + # is in fact the decorated function, so we need to return + # the wrapped function, not the wrapper + # proper solution would be to use login_required() with parentheses + # everywhere, even if they're empty - TODO + if callable(role): + return view_wrapper(role) + else: + return view_wrapper # backend authentication def backend_authenticated(f): diff --git a/coprs_frontend/tests/coprs_test_case.py b/coprs_frontend/tests/coprs_test_case.py index 4f6043f..bea1f58 100644 --- a/coprs_frontend/tests/coprs_test_case.py +++ b/coprs_frontend/tests/coprs_test_case.py @@ -40,9 +40,9 @@ class CoprsTestCase(object): @pytest.fixture def f_users(self): - self.u1 = models.User(openid_name = u'http://user1.id.fedoraproject.org/', proven = False, mail = 'user1(a)foo.bar') - self.u2 = models.User(openid_name = u'http://user2.id.fedoraproject.org/', proven = False, mail = 'user2(a)spam.foo') - self.u3 = models.User(openid_name = u'http://user3.id.fedoraproject.org/', proven = False, mail = 'baz(a)bar.bar') + self.u1 = models.User(openid_name=u'http://user1.id.fedoraproject.org/', proven=False, admin=True, mail='user1(a)foo.bar') + self.u2 = models.User(openid_name=u'http://user2.id.fedoraproject.org/', proven = False, mail='user2(a)spam.foo') + self.u3 = models.User(openid_name=u'http://user3.id.fedoraproject.org/', proven=False, mail='baz(a)bar.bar') self.db.session.add_all([self.u1, self.u2, self.u3]) diff --git a/coprs_frontend/tests/test_views/test_admin/test_admin_general.py b/coprs_frontend/tests/test_views/test_admin/test_admin_general.py new file mode 100644 index 0000000..1223934 --- /dev/null +++ b/coprs_frontend/tests/test_views/test_admin/test_admin_general.py @@ -0,0 +1,20 @@ +from tests.coprs_test_case import CoprsTestCase + +class TestAdminLogin(CoprsTestCase): + # TODO: test on something better then page title - maybe see rendered templates? + text_to_check = 'Coprs - Admin' + def test_nonadmin_cant_login(self, f_users, f_db): + with self.tc as c: + with c.session_transaction() as s: + s['openid'] = self.u2.openid_name + + r = c.get('/admin/', follow_redirects=True) + assert self.text_to_check not in r.data + + def test_admin_can_login(self, f_users, f_db): + with self.tc as c: + with c.session_transaction() as s: + s['openid'] = self.u1.openid_name + + r = c.get('/admin/', follow_redirects=True) + assert self.text_to_check in r.data

1 0

[copr] master: It seems that pytest does not like having files with same names... (57dc405)
by bkabrda＠fedoraproject.org 28 Mar '13

28 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 57dc4058d4c6709bde5e95ddf67716f142482632 Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Thu Mar 28 15:30:52 2013 +0100 It seems that pytest does not like having files with same names... >--------------------------------------------------------------- .../{test_general.py => test_backend_general.py} | 0 .../{test_builds.py => test_coprs_builds.py} | 0 .../{test_general.py => test_coprs_general.py} | 0 3 files changed, 0 insertions(+), 0 deletions(-) diff --git a/coprs_frontend/tests/test_views/test_backend_ns/test_general.py b/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py similarity index 100% rename from coprs_frontend/tests/test_views/test_backend_ns/test_general.py rename to coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py diff --git a/coprs_frontend/tests/test_views/test_coprs_ns/test_builds.py b/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py similarity index 100% rename from coprs_frontend/tests/test_views/test_coprs_ns/test_builds.py rename to coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_builds.py diff --git a/coprs_frontend/tests/test_views/test_coprs_ns/test_general.py b/coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py similarity index 100% rename from coprs_frontend/tests/test_views/test_coprs_ns/test_general.py rename to coprs_frontend/tests/test_views/test_coprs_ns/test_coprs_general.py

1 0

[copr] master: Move build creation authorization to logic. (6aacde2)
by bkabrda＠fedoraproject.org 28 Mar '13

28 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 6aacde2a7f1f3e2651d0786277480d1d9727daab Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Thu Mar 28 13:10:52 2013 +0100 Move build creation authorization to logic. >--------------------------------------------------------------- coprs_frontend/coprs/logic/builds_logic.py | 11 +++++------ coprs_frontend/coprs/logic/users_logic.py | 8 ++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/coprs_frontend/coprs/logic/builds_logic.py b/coprs_frontend/coprs/logic/builds_logic.py index 2a91f95..834217d 100644 --- a/coprs_frontend/coprs/logic/builds_logic.py +++ b/coprs_frontend/coprs/logic/builds_logic.py @@ -6,6 +6,7 @@ from coprs import models from coprs import signals from coprs.logic import coprs_logic +from coprs.logic import users_logic class BuildsLogic(object): @classmethod @@ -59,6 +60,8 @@ class BuildsLogic(object): def add(cls, user, pkgs, copr): coprs_logic.CoprsLogic.raise_if_unfinished_action(user, copr, 'Can\'t build while there is an operation in progress: {action}') + users_logic.UsersLogic.raise_if_cant_build_in_copr(user, copr, + 'You don\'t have permissions to build in this copr.') build = models.Build( pkgs=pkgs, copr=copr, @@ -66,15 +69,11 @@ class BuildsLogic(object): chroots=' '.join(map(lambda x: x.chroot_name, copr.active_mock_chroots)), user=user, submitted_on=int(time.time())) - # no need to check for authorization here - cls.new(user, build, copr, check_authorized=False) + cls.new(user, build, copr) return build @classmethod - def new(cls, user, build, copr, check_authorized = True): - if check_authorized: - if not user.can_build_in(copr): - raise exceptions.InsufficientRightsException('User {0} cannot build in copr {1}/{2}'.format(user.name, copr.owner.name, copr.name)) + def new(cls, user, build, copr): if not build.submitted_on: build.submitted_on = int(time.time()) if not build.user: diff --git a/coprs_frontend/coprs/logic/users_logic.py b/coprs_frontend/coprs/logic/users_logic.py index 8ff0eb2..3f6d048 100644 --- a/coprs_frontend/coprs/logic/users_logic.py +++ b/coprs_frontend/coprs/logic/users_logic.py @@ -9,3 +9,11 @@ class UsersLogic(object): # TODO: this is a bit inconsistent - shouldn't the user method be called can_update? if not user.can_edit(copr): raise exceptions.InsufficientRightsException(message) + + @classmethod + def raise_if_cant_build_in_copr(cls, user, copr, message): + """This method raises InsufficientRightsException if given user cant build in + given copr. Returns None otherwise. + """ + if not user.can_build_in(copr): + raise exceptions.InsufficientRightsException(message)

1 0

[copr] master: Move authorization checking for deleting coprs into logic (971c737)
by bkabrda＠fedoraproject.org 28 Mar '13

28 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 971c737b620cb4851bffddf3d234b6550ce7ced9 Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Thu Mar 28 12:58:59 2013 +0100 Move authorization checking for deleting coprs into logic >--------------------------------------------------------------- coprs_frontend/coprs/logic/coprs_logic.py | 3 ++- .../coprs/views/coprs_ns/coprs_general.py | 6 +----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/coprs_frontend/coprs/logic/coprs_logic.py b/coprs_frontend/coprs/logic/coprs_logic.py index 6922789..663c5cd 100644 --- a/coprs_frontend/coprs/logic/coprs_logic.py +++ b/coprs_frontend/coprs/logic/coprs_logic.py @@ -117,7 +117,8 @@ class CoprsLogic(object): @classmethod def delete(cls, user, copr, check_for_duplicates=True): - # for the time being, we authorize user to do this in view... + if not copr.owner == user: + raise exceptions.InsufficientRightsException('Only owners may delete their Coprs.') # TODO: do we want to dump the information somewhere, so that we can search it in future? cls.raise_if_unfinished_action(user, copr, 'Can\'t delete this Copr, another operation is in progress: {action}') diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py index 51bc7d1..1b8353b 100644 --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py @@ -238,15 +238,11 @@ def copr_update_permissions(username, coprname): def copr_delete(username, coprname): form = forms.CoprDeleteForm() copr = coprs_logic.CoprsLogic.get(flask.g.user, username, coprname).first() - # only owner can delete a copr - if flask.g.user != copr.owner: - flask.flash('Only owners may delete their Coprs.') - return flask.redirect(flask.url_for('coprs_ns.copr_detail', username=username, coprname=coprname)) if form.validate_on_submit(): try: coprs_logic.CoprsLogic.delete(flask.g.user, copr) - except exceptions.ActionInProgressException as e: + except (exceptions.ActionInProgressException, exceptions.InsufficientRightsException) as e: db.session.rollback() flask.flash(e) return flask.redirect(flask.url_for('coprs_ns.copr_detail', username=username, coprname=coprname))

1 0

[copr] master: Move authorization checking for updating permissions into logic (8fe7f0a)
by bkabrda＠fedoraproject.org 28 Mar '13

28 Mar '13

Repository : http://git.fedorahosted.org/cgit/copr.git On branch : master >--------------------------------------------------------------- commit 8fe7f0ac8991df619754a60f253a690f7b129e1a Author: Bohuslav Kabrda <bkabrda(a)redhat.com> Date: Thu Mar 28 12:50:09 2013 +0100 Move authorization checking for updating permissions into logic - refactor the raise_if_cant_update_copr method into newly introduced UsersLogic class >--------------------------------------------------------------- coprs_frontend/coprs/logic/coprs_logic.py | 16 ++++--------- coprs_frontend/coprs/logic/users_logic.py | 11 +++++++++ .../coprs/views/coprs_ns/coprs_general.py | 24 ++++++++++---------- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/coprs_frontend/coprs/logic/coprs_logic.py b/coprs_frontend/coprs/logic/coprs_logic.py index 2b4b2d9..6922789 100644 --- a/coprs_frontend/coprs/logic/coprs_logic.py +++ b/coprs_frontend/coprs/logic/coprs_logic.py @@ -6,6 +6,7 @@ from coprs import helpers from coprs import models from coprs import signals from coprs import whoosheers +from coprs.logic import users_logic class CoprsLogic(object): """Used for manipulating Coprs. All methods accept user object as a first argument, as this may be needed in future.""" @@ -94,7 +95,8 @@ class CoprsLogic(object): def update(cls, user, copr, check_for_duplicates = True): cls.raise_if_unfinished_action(user, copr, 'Can\'t change this Copr name, another operation is in progress: {action}') - cls.raise_if_cant_update(user, copr, 'Only owners and admins may update their Coprs.') + users_logic.UsersLogic.raise_if_cant_update_copr(user, copr, + 'Only owners and admins may update their Coprs.') existing = cls.exists_for_user(copr.owner, copr.name).first() if existing: @@ -160,16 +162,6 @@ class CoprsLogic(object): if unfinished_actions: raise exceptions.ActionInProgressException(message, unfinished_actions[0]) - @classmethod - def raise_if_cant_update(cls, user, copr, message): - """This method raises InsufficientRightsException if given user cant update - given copr. Returns None otherwise. - """ - # TODO: this is a bit inconsistent - shouldn't the user method be called can_update? - if not user.can_edit(copr): - raise exceptions.InsufficientRightsException(message) - - class CoprPermissionsLogic(object): @classmethod def get(cls, user, copr, searched_user): @@ -190,6 +182,8 @@ class CoprPermissionsLogic(object): @classmethod def update_permissions(cls, user, copr, copr_permission, new_builder, new_admin): + users_logic.UsersLogic.raise_if_cant_update_copr(user, copr, + 'Only owners and admins may update their Coprs permissions.') models.CoprPermission.query.filter(models.CoprPermission.copr_id == copr.id).\ filter(models.CoprPermission.user_id == copr_permission.user_id).\ update({'copr_builder': new_builder, diff --git a/coprs_frontend/coprs/logic/users_logic.py b/coprs_frontend/coprs/logic/users_logic.py new file mode 100644 index 0000000..8ff0eb2 --- /dev/null +++ b/coprs_frontend/coprs/logic/users_logic.py @@ -0,0 +1,11 @@ +import exceptions + +class UsersLogic(object): + @classmethod + def raise_if_cant_update_copr(cls, user, copr, message): + """This method raises InsufficientRightsException if given user cant update + given copr. Returns None otherwise. + """ + # TODO: this is a bit inconsistent - shouldn't the user method be called can_update? + if not user.can_edit(copr): + raise exceptions.InsufficientRightsException(message) diff --git a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py index adba1d0..51bc7d1 100644 --- a/coprs_frontend/coprs/views/coprs_ns/coprs_general.py +++ b/coprs_frontend/coprs/views/coprs_ns/coprs_general.py @@ -216,20 +216,20 @@ def copr_update_permissions(username, coprname): permissions = copr.copr_permissions permissions_form = forms.PermissionsFormFactory.create_form_cls(permissions)() - # only owner can update copr permissions - if not flask.g.user.can_edit(copr): - flask.flash('Only owners and admins may update their Coprs permissions.') - return flask.redirect(flask.url_for('coprs_ns.copr_detail', username = copr.owner.name, coprname = copr.name)) - if permissions_form.validate_on_submit(): # we don't change owner (yet) - for perm in permissions: - new_builder = permissions_form['copr_builder_{0}'.format(perm.user_id)].data - new_admin = permissions_form['copr_admin_{0}'.format(perm.user_id)].data - coprs_logic.CoprPermissionsLogic.update_permissions(flask.g.user, copr, perm, new_builder, new_admin) - - db.session.commit() - flask.flash('Copr permissions were updated successfully.') + try: + for perm in permissions: + new_builder = permissions_form['copr_builder_{0}'.format(perm.user_id)].data + new_admin = permissions_form['copr_admin_{0}'.format(perm.user_id)].data + coprs_logic.CoprPermissionsLogic.update_permissions(flask.g.user, copr, perm, new_builder, new_admin) + # for now, we don't check for actions here, as permissions operation don't collide with any actions + except exceptions.InsufficientRightsException as e: + db.session.rollback() + flask.flash(e) + else: + db.session.commit() + flask.flash('Copr permissions were updated successfully.') return flask.redirect(flask.url_for('coprs_ns.copr_detail', username = copr.owner.name, coprname = copr.name))

1 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

copr-devel March 2013