swarming: switch to a 'capability focused' ACL system

appengine/swarming/server/acl.py

Index: appengine/swarming/server/acl.py
diff --git a/appengine/swarming/server/acl.py b/appengine/swarming/server/acl.py
index c28e54f447af06bf3cb383e043ef32b033fc3be1..4ce91c4eb62a875a0525001ea2f94a5008c2e3e6 100644
--- a/appengine/swarming/server/acl.py
+++ b/appengine/swarming/server/acl.py
@@ -9,25 +9,40 @@ from components import utils
 from server import config
 
 
-def is_admin():
-  admins = config.settings().auth.admins_group
-  return auth.is_group_member(admins) or auth.is_admin()
+def _is_admin():
+  """The admins group is a super set of the privileged users group."""
+  group = config.settings().auth.admins_group
+  return auth.is_group_member(group) or auth.is_admin()
 
 
-def is_privileged_user():
-  priv_users = config.settings().auth.privileged_users_group
-  return auth.is_group_member(priv_users) or is_admin()
+def _is_privileged_user():
+  """The privileged users group is a super set of the users group."""
+  group = config.settings().auth.privileged_users_group
+  return auth.is_group_member(group) or _is_admin()
 
 
-def is_user():
-  users = config.settings().auth.users_group
-  return auth.is_group_member(users) or is_privileged_user()
+def _is_user():
+  group = config.settings().auth.users_group
+  return auth.is_group_member(group) or _is_privileged_user()
 
 
-def is_bootstrapper():
+def _is_view_all_bots():
+  group = config.settings().auth.view_all_bots_group
+  return auth.is_group_member(group) or _is_privileged_user()
+
+
+def _is_view_all_tasks():
+  group = config.settings().auth.view_all_tasks_group
+  return auth.is_group_member(group) or _is_privileged_user()
+
+
+def _is_bootstrapper():
   """Returns True if current user have access to bot code (for bootstrap)."""
   bot_group = config.settings().auth.bot_bootstrap_group
-  return is_admin() or auth.is_group_member(bot_group)
+  return _is_admin() or auth.is_group_member(bot_group)
+
+
+### Capabilities
 
 
 def is_ip_whitelisted_machine():
@@ -39,44 +54,111 @@ def is_ip_whitelisted_machine():
       auth.bots_ip_whitelist(), auth.get_peer_ip(), False)
 
 
-def is_bot():
-  # TODO(vadimsh): Get rid of this. Swarming jobs will use service accounts
-  # associated with the job when calling Swarming, not the machine IP.
-  return is_ip_whitelisted_machine() or is_admin()
+def can_access():
+  """Minimally authenticated user."""
+  return (
+      is_ip_whitelisted_machine() or _is_user() or
+      _is_view_all_bots() or _is_view_all_tasks())
+
+
+def can_config_view():

[Vadim Sh., 2017/07/21 22:01:20]

nit: please make it "can_<verb>_<subject>". Will be much more readable.

[M-A Ruel, 2017/07/24 15:42:51]

I did ponder between "can_<object>_<mutation>" and "can_<mutation>_<object>" and
preferred the other way but changed, as I prefer to optimize for readability and
whatever others prefer.

+  """Can view the configuration data."""
+  return _is_admin()
+
+
+def can_config_edit():
+  """Can edit the configuration data.
+
+  Only super users can edit the configuration data.
+  """
+  return _is_admin()
+
+
+def can_bot_view():
+  """Can view bot.
+
+  Bots can view other bots. This may change in the future.
+  """
+  return is_ip_whitelisted_machine() or _is_user() or _is_view_all_bots()
+
+
+def can_bot_create():
+  """Can create (bootstrap) a bot."""
+  return _is_admin() or _is_bootstrapper()
+
+
+def can_bot_edit():
+  """Can terminate, delete a bot.
+
+  Bots can terminate other bots. This may change in the future.
+  """
+  return is_ip_whitelisted_machine() or _is_privileged_user()
+
+
+def can_task_view():
+  """Can view tasks.
+
+  It is possible that the user can only see a subset of the tasks.
+  """
+  return is_ip_whitelisted_machine() or _is_view_all_tasks() or _is_user()
+
+
+def can_task_create():
+  """Can create a task.
+
+  Swarming is reentrant, a bot can create a new task as part of a task. This may
+  change in the future.
+  """
+  return is_ip_whitelisted_machine() or _is_user()
+
+
+def can_task_edit():
+  """Can 'edit' tasks, like cancelling.
+
+  Since bots can create tasks, they can also cancel them. This may change in the
+  future.
+  """
+  return is_ip_whitelisted_machine() or _is_user()
+
 
+def can_tasks_edit():
+  """Can 'edit' a batch of tasks, like cancelling."""
+  return _is_privileged_user()
 
-def is_bot_or_user():
-  # TODO(vadimsh): Get rid of this. Swarming jobs will use service accounts
-  # associated with the job when calling Swarming, not the machine ID itself.
-  return is_bot() or is_user()
 
+def can_id_task_view(identity):

[Vadim Sh., 2017/07/21 22:01:20]

I think this should be merged with 'can_task_view' and should accept a task
being viewed.

def can_view_task(task_request):
  ...

So handlers will look like

@auth.require(acl.can_access)
def result(self, request):
  ....
  if not acl.can_view_task(task):
    ...

If we want fine-grain ACLs like that we need to come up with a crystal clear
data model: what our resources are and what can be done with them.

Ideally, we'll have only 

can_create_task()
can_view_task(task)
can_edit_task(task)

can_create_bot()
can_view_bot(bot)
can_edit_bot(bot)

And listings will be build on top of can_view_bot/can_view_task.  I'm not sure
it is possible to do efficiently in practice, especially considering that some
requests just count number of tasks/bots, without fetching them :(

So, I think, we might need to add new kinds of resource "all tasks" and "all
bots" and have:

can_view_all_tasks()
can_view_all_bots()

And we'll have in the end:

can_access()

can_view_config()
can_edit_config()

can_create_bot()
can_edit_bot(bot)
can_view_bot(bot)
can_view_all_bots()

can_create_task()
can_edit_task(task)
can_view_task(task)
can_view_all_tasks()

Listings and countings will be allowed only for can_*_all_*.

---

If we are serious about sharing single Swarming host by multiple teams, we may
also consider adding bot pool as a resource instead of a bot. Then we'll have:

can_view_pool(pool)  // instead of can_view_bot
can_edit_pool(pool)   // instead of can_create_bot()
can_list_pool(pool)
can_list_all_pools()    // instead of can_list_bots

Though this will require a lot more refactorings to support in UI. Current UI is
very single-tenant :(

+  """Can this user view a task."""
+  return _is_privileged_user() or auth.get_current_identity() == identity
 
-def is_bot_or_privileged_user():
-  # TODO(vadimsh): Get rid of this. Swarming jobs will use service accounts
-  # associated with the job when calling Swarming, not the machine ID itself.
-  return is_bot() or is_privileged_user()
 
+def can_id_task_edit(identity):
+  """Can 'edit' tasks, like cancelling.
 
-def is_bot_or_admin():
-  """Returns True if current user can execute user-side and bot-side calls."""
-  # TODO(vadimsh): Get rid of this. Swarming jobs will use service accounts
-  # associated with the job when calling Swarming, not the machine ID itself.
-  return is_bot() or is_admin()
+  Since bots can create tasks, they can also cancel them. This may change in the
+  future.
+  """
+  return (
+      is_ip_whitelisted_machine() or _is_privileged_user() or
+      auth.get_current_identity() == identity)
 
 
 def can_schedule_high_priority_tasks():
   """Returns True if the current user can schedule high priority tasks."""
-  return is_bot() or is_privileged_user()
+  return is_ip_whitelisted_machine() or _is_privileged_user()
 
 
 def get_user_type():

[Vadim Sh., 2017/07/21 22:01:20]

We should get rid of this, it doesn't work well in "capabilities based" acl
system, since callers may have a "mix" of capabilities, not fitting in any of
the categories.

[M-A Ruel, 2017/07/24 15:42:51]

Done.

   """Returns a string describing the current access control for the user."""
-  if is_admin():
+  if _is_admin():
     return 'admin'
-  if is_privileged_user():
+  if _is_privileged_user():
     return 'privileged user'
-  if is_user():
+  if _is_user():
     return 'user'
+  if _is_view_all_bots():
+    return 'bots_viewer'
+  if _is_view_all_tasks():
+    return 'tasks_viewer'
 
 
 def bootstrap_dev_server_acls():