[GitHub] [superset] dpgaspar commented on a change in pull request #15149: feat(dao): admin can remove self from object owners

[GitBox <gi...@apache.org>]

dpgaspar commented on a change in pull request #15149:
URL: https://github.com/apache/superset/pull/15149#discussion_r651153610



##########
File path: tests/charts/api_tests.py
##########
@@ -564,12 +564,13 @@ def test_update_chart_new_owner(self):
         gamma = self.get_user("gamma")
         admin = self.get_user("admin")
         chart_id = self.insert_chart("title", [gamma.id], 1).id
-        chart_data = {"slice_name": "title1_changed"}
+        chart_data = {"slice_name": "title1_changed", "owners": [admin.id]}

Review comment:
       Would be nice to add a `gamma` user test also. Seems to be missing the same kind of test for reports and datasets
   

##########
File path: superset/commands/utils.py
##########
@@ -29,17 +29,25 @@
 from superset.extensions import db, security_manager
 
 
-def populate_owners(user: User, owner_ids: Optional[List[int]] = None) -> List[User]:
+def populate_owners(
+    user: User, owner_ids: Optional[List[int]], default_to_user: bool,
+) -> List[User]:
     """
     Helper function for commands, will fetch all users from owners id's
-    Can raise ValidationError
-    :param user: The current user
-    :param owner_ids: A List of owners by id's
+    :param user: current user
+    :param owner_ids: list of owners by id's
+    :param default_to_user: make user the owner if `owner_ids` is None or empty
+    :raises OwnersNotFoundValidationError: if at least one owner id can't be resolved
+    :returns: Final list of owners
     """
+    owner_ids = owner_ids or []
     owners = list()
-    if not owner_ids:
+    if not owner_ids and default_to_user:
         return [user]
-    if user.id not in owner_ids:
+    if user.id not in owner_ids and "admin" not in [
+        role.name.lower() for role in user.roles
+    ]:

Review comment:
       A bit out of scope, but there's an `is_user_admin()` on `superset.views.base`, seems misplaced and it probably would make more sense to place it on security manager. Also it uses `g.user`.
   Another concern is that we may have a previous problem here regarding anonymous users (public access)

##########
File path: superset/commands/utils.py
##########
@@ -29,17 +29,25 @@
 from superset.extensions import db, security_manager
 
 
-def populate_owners(user: User, owner_ids: Optional[List[int]] = None) -> List[User]:
+def populate_owners(
+    user: User, owner_ids: Optional[List[int]], default_to_user: bool,
+) -> List[User]:
     """
     Helper function for commands, will fetch all users from owners id's
-    Can raise ValidationError
-    :param user: The current user
-    :param owner_ids: A List of owners by id's
+    :param user: current user
+    :param owner_ids: list of owners by id's
+    :param default_to_user: make user the owner if `owner_ids` is None or empty
+    :raises OwnersNotFoundValidationError: if at least one owner id can't be resolved
+    :returns: Final list of owners
     """
+    owner_ids = owner_ids or []

Review comment:
       good catch




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org