launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06980
[Merge] lp:~rvb/maas/maas-remove-enlist-autoaccept into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-remove-enlist-autoaccept into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-remove-enlist-autoaccept/+merge/100609
This cleanup branch factors Node.get_visible_nodes and Node.get_editable_nodes into Node.get_nodes with a parameter (the checked permission).
--
https://code.launchpad.net/~rvb/maas/maas-remove-enlist-autoaccept/+merge/100609
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-remove-enlist-autoaccept into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/api.py 2012-04-03 14:18:26 +0000
@@ -513,7 +513,8 @@
match_ids = request.GET.getlist('id')
if match_ids == []:
match_ids = None
- nodes = Node.objects.get_visible_nodes(request.user, ids=match_ids)
+ nodes = Node.objects.get_nodes(
+ request.user, NODE_PERMISSION.VIEW, ids=match_ids)
return nodes.order_by('id')
@api_exported('list_allocated', 'GET')
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/models.py 2012-04-03 14:18:26 +0000
@@ -288,11 +288,13 @@
else:
return query.filter(system_id__in=ids)
- def get_visible_nodes(self, user, ids=None):
- """Fetch Nodes visible by a User_.
+ def get_nodes(self, user, perm, ids=None):
+ """Fetch Nodes on which the User_ has the given permission.
:param user: The user that should be used in the permission check.
:type user: User_
+ :param perm: The permission to check.
+ :type perm: basestring.
:param ids: If given, limit result to nodes with these system_ids.
:type ids: Sequence.
@@ -302,11 +304,21 @@
"""
if user.is_superuser:
- visible_nodes = self.all()
+ nodes = self.all()
else:
- visible_nodes = self.filter(
- models.Q(owner__isnull=True) | models.Q(owner=user))
- return self.filter_by_ids(visible_nodes, ids)
+ if perm == NODE_PERMISSION.VIEW:
+ nodes = self.filter(
+ models.Q(owner__isnull=True) | models.Q(owner=user))
+ elif perm == NODE_PERMISSION.EDIT:
+ nodes = self.filter(owner=user)
+ elif perm == NODE_PERMISSION.ADMIN:
+ nodes = self.none()
+ else:
+ raise NotImplementedError(
+ "Invalid permission check (invalid permission name: %s)." %
+ perm)
+
+ return self.filter_by_ids(nodes, ids)
def get_allocated_visible_nodes(self, token, ids):
"""Fetch Nodes that were allocated to the User_/oauth token.
@@ -329,22 +341,6 @@
nodes = self.filter(token=token, system_id__in=ids)
return nodes
- def get_editable_nodes(self, user, ids=None):
- """Fetch Nodes a User_ has ownership privileges on.
-
- An admin has ownership privileges on all nodes.
-
- :param user: The user that should be used in the permission check.
- :type user: User_
- :param ids: If given, limit result to nodes with these system_ids.
- :type ids: Sequence.
- """
- if user.is_superuser:
- visible_nodes = self.all()
- else:
- visible_nodes = self.filter(owner=user)
- return self.filter_by_ids(visible_nodes, ids)
-
def get_node_or_404(self, system_id, user, perm):
"""Fetch a `Node` by system_id. Raise exceptions if no `Node` with
this system_id exist or if the provided user has not the required
@@ -382,7 +378,7 @@
if constraints is None:
constraints = {}
available_nodes = (
- self.get_visible_nodes(for_user)
+ self.get_nodes(for_user, NODE_PERMISSION.VIEW)
.filter(status=NODE_STATUS.READY))
if constraints.get('name'):
@@ -408,7 +404,7 @@
:return: Those Nodes for which shutdown was actually requested.
:rtype: list
"""
- nodes = self.get_editable_nodes(by_user, ids=ids)
+ nodes = self.get_nodes(by_user, NODE_PERMISSION.EDIT, ids=ids)
get_papi().stop_nodes([node.system_id for node in nodes])
return nodes
@@ -430,7 +426,7 @@
:rtype: list
"""
from metadataserver.models import NodeUserData
- nodes = self.get_editable_nodes(by_user, ids=ids)
+ nodes = self.get_nodes(by_user, NODE_PERMISSION.EDIT, ids=ids)
if user_data is not None:
for node in nodes:
NodeUserData.objects.set_user_data(node, user_data)
=== modified file 'src/maasserver/tests/test_auth.py'
--- src/maasserver/tests/test_auth.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/tests/test_auth.py 2012-04-03 14:18:26 +0000
@@ -148,7 +148,9 @@
make_unallocated_node(),
]
self.assertItemsEqual(
- nodes, Node.objects.get_visible_nodes(factory.make_admin()))
+ nodes,
+ Node.objects.get_nodes(
+ factory.make_admin(), NODE_PERMISSION.VIEW))
def test_user_sees_own_nodes_and_unowned_nodes(self):
user = factory.make_user()
@@ -157,4 +159,4 @@
unowned_node = make_unallocated_node()
self.assertItemsEqual(
[own_node, unowned_node],
- Node.objects.get_visible_nodes(own_node.owner))
+ Node.objects.get_nodes(own_node.owner, NODE_PERMISSION.VIEW))
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/tests/test_models.py 2012-04-03 14:18:26 +0000
@@ -368,8 +368,9 @@
self.assertItemsEqual(
[node], Node.objects.filter_by_ids(Node.objects.all(), None))
- def test_get_visible_nodes_for_user_lists_visible_nodes(self):
- """get_visible_nodes lists the nodes a user has access to.
+ def test_get_nodes_for_user_lists_visible_nodes(self):
+ """get_nodes with perm=NODE_PERMISSION.VIEW lists the nodes a user
+ has access to.
When run for a regular user it returns unowned nodes, and nodes
owned by that user.
@@ -378,9 +379,9 @@
visible_nodes = [self.make_node(owner) for owner in [None, user]]
self.make_node(factory.make_user())
self.assertItemsEqual(
- visible_nodes, Node.objects.get_visible_nodes(user))
+ visible_nodes, Node.objects.get_nodes(user, NODE_PERMISSION.VIEW))
- def test_get_visible_nodes_admin_lists_all_nodes(self):
+ def test_get_nodes_admin_lists_all_nodes(self):
admin = factory.make_admin()
owners = [
None,
@@ -389,26 +390,29 @@
admin,
]
nodes = [self.make_node(owner) for owner in owners]
- self.assertItemsEqual(nodes, Node.objects.get_visible_nodes(admin))
+ self.assertItemsEqual(
+ nodes, Node.objects.get_nodes(admin, NODE_PERMISSION.VIEW))
- def test_get_visible_nodes_filters_by_id(self):
+ def test_get_nodes_filters_by_id(self):
user = factory.make_user()
nodes = [self.make_node(user) for counter in range(5)]
ids = [node.system_id for node in nodes]
wanted_slice = slice(0, 3)
self.assertItemsEqual(
nodes[wanted_slice],
- Node.objects.get_visible_nodes(user, ids=ids[wanted_slice]))
+ Node.objects.get_nodes(
+ user, NODE_PERMISSION.VIEW, ids=ids[wanted_slice]))
- def test_get_editable_nodes_for_user_lists_owned_nodes(self):
+ def test_get_nodes_with_edit_perm_for_user_lists_owned_nodes(self):
user = factory.make_user()
visible_node = self.make_node(user)
self.make_node(None)
self.make_node(factory.make_user())
self.assertItemsEqual(
- [visible_node], Node.objects.get_editable_nodes(user))
+ [visible_node],
+ Node.objects.get_nodes(user, NODE_PERMISSION.EDIT))
- def test_get_editable_nodes_admin_lists_all_nodes(self):
+ def test_get_nodes_with_edit_perm_admin_lists_all_nodes(self):
admin = factory.make_admin()
owners = [
None,
@@ -417,16 +421,23 @@
admin,
]
nodes = [self.make_node(owner) for owner in owners]
- self.assertItemsEqual(nodes, Node.objects.get_editable_nodes(admin))
-
- def test_get_editable_nodes_filters_by_id(self):
+ self.assertItemsEqual(
+ nodes, Node.objects.get_nodes(admin, NODE_PERMISSION.EDIT))
+
+ def test_get_nodes_with_admin_perm_returns_empty_list_for_user(self):
+ user = factory.make_user()
+ [self.make_node(user) for counter in range(5)]
+ self.assertItemsEqual(
+ [],
+ Node.objects.get_nodes(user, NODE_PERMISSION.ADMIN))
+
+ def test_get_nodes_with_admin_perm_returns_all_nodes_for_admin(self):
user = factory.make_user()
nodes = [self.make_node(user) for counter in range(5)]
- ids = [node.system_id for node in nodes]
- wanted_slice = slice(0, 3)
self.assertItemsEqual(
- nodes[wanted_slice],
- Node.objects.get_editable_nodes(user, ids=ids[wanted_slice]))
+ nodes,
+ Node.objects.get_nodes(
+ factory.make_admin(), NODE_PERMISSION.ADMIN))
def test_get_visible_node_or_404_ok(self):
"""get_node_or_404 fetches nodes by system_id."""
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-04-03 10:19:47 +0000
+++ src/maasserver/views.py 2012-04-03 14:18:26 +0000
@@ -176,7 +176,8 @@
context_object_name = "node_list"
def get_queryset(self):
- return Node.objects.get_visible_nodes(user=self.request.user)
+ return Node.objects.get_nodes(
+ user=self.request.user, perm=NODE_PERMISSION.VIEW)
def get_context_data(self, **kwargs):
context = super(NodeListView, self).get_context_data(**kwargs)
