#780 [frontend] allow group members to delete all projects in the group
Merged 5 months ago by praiskup. Opened 5 months ago by frostyx.
copr/ frostyx/copr delete-group-project  into  master

@@ -368,10 +368,18 @@ 

          Raise InsufficientRightsException if given copr cant be deleted

          by given user. Return None otherwise.

          """

+         if user.admin:

+             return

+ 

+         if copr.group:

+             return UsersLogic.raise_if_not_in_group(user, copr.group)

+ 

+         if user == copr.user:

+             return

+ 

+         raise exceptions.InsufficientRightsException(

+             "Only owners may delete their projects.")

  

-         if not user.admin and user != copr.user:

-             raise exceptions.InsufficientRightsException(

-                 "Only owners may delete their projects.")

  

  class CoprPermissionsLogic(object):

      @classmethod

@@ -1,5 +1,6 @@ 

  import json

  import datetime

+ import pytest

  

  from flask_whooshee import Whooshee

  

@@ -9,10 +10,12 @@ 

  from coprs import app

  from coprs.logic.actions_logic import ActionsLogic

  from coprs.logic.coprs_logic import CoprsLogic, CoprChrootsLogic

+ from coprs.logic.users_logic import UsersLogic

  

  from coprs import models

  from coprs.whoosheers import CoprWhoosheer

  from tests.coprs_test_case import CoprsTestCase

+ from coprs.exceptions import InsufficientRightsException

  

  

  class TestCoprsLogic(CoprsTestCase):

@@ -70,6 +73,30 @@ 

  

          assert obtained == expected

  

+     def test_raise_if_cant_delete(self, f_users, f_fas_groups, f_coprs):

+         # Project owner should be able to delete his project

+         CoprsLogic.raise_if_cant_delete(self.u2, self.c2)

+ 

+         # Admin should be able to delete everything

+         CoprsLogic.raise_if_cant_delete(self.u1, self.c2)

+ 

+         # A user can't remove someone else's project

+         with pytest.raises(InsufficientRightsException):

+             CoprsLogic.raise_if_cant_delete(self.u2, self.c1)

+ 

+         # Group member should be able to remove group project

+         self.u2.openid_groups = {"fas_groups": ["somegroup"]}

+         self.u3.openid_groups = {"fas_groups": ["somegroup"]}

+ 

+         self.c2.group = UsersLogic.get_group_by_fas_name_or_create("somegroup")

+         CoprsLogic.raise_if_cant_delete(self.u3, self.c2)

+ 

+         # Once a member is kicked from a group, he can't delete

+         # a project even though he originally created it

+         self.u2.openid_groups = {"fas_groups": []}

+         with pytest.raises(InsufficientRightsException):

+             CoprsLogic.raise_if_cant_delete(self.u2, self.c2)

+ 

      def test_copr_logic_add_sends_create_gpg_key_action(self, f_users, f_mock_chroots, f_db):

          name = u"project_1"

          selected_chroots = [self.mc1.name]

See #779

Another possiblity would be to print an error message like this

"This project is owned by <user> who has exclusive permissions
 to delete it."

But I feel like everybody in a group should be able to do
whatever action for their group projects.

what about to return right after raise_if_not_iun_group, and avoid elif?

And use if user == copr.user: return as well.

rebased onto 5d36113354bee0204f24e851d1b3b79cdb321219

5 months ago

what about to return right after raise_if_not_iun_group, and avoid elif?
And use if user == copr.user: return as well.

Good thinking. PTAL.

I wonder what should happen here:

  1. user1 joins the group1
  2. user1 creates the @group1/copr1 copr
  3. admin1 kicks user1 from group1
  4. user1 attempts to delete @group1/copr1

In this PR code, it seems it will succeed. I think it should not.

In other words, for group coprs, the copr.user makes no sense at all.

tagging with needs-work for Miro's point

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work

5 months ago

1 new commit added

  • [frontend] do not consider permissions for project owner on group projects
5 months ago

In other words, for group coprs, the copr.user makes no sense at all.

I've addressed Miro's point and added some tests

Metadata Update from @frostyx:
- Pull-request untagged with: needs-work

5 months ago

Pull-Request has been merged by praiskup

5 months ago