Bug #9799

[Crunch2] Ensure live logs for containers are accessible to non-admin users

Added by Tom Clegg almost 4 years ago. Updated almost 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Crunch
Target version:
Start date:
08/17/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5
Release:
Release relationship:
Auto

Description

Currently source:apps/workbench/test/integration/websockets_test.rb uses admin credentials. This can (and, I suspect, does) conveniently avoid detecting permission-related problems that prevent normal users from seeing any live logs (via Workbench, crunchstat-summary, arv-ws, etc).

Proposed approach:
  • Change the tests so they use "active" instead of "admin" user
  • (If needed) make logging work again (might require API / permission model changes)

Subtasks

Task #9814: Update testsResolvedTom Clegg

Task #9807: Review 9799-nonadmin-logsResolvedTom Clegg


Related issues

Related to Arvados - Bug #9678: [Crunch2] [Workbench] container_request log tab is empty, even when the container log tab shows informationResolved08/11/2016

Related to Arvados - Bug #9848: [Crunch2] [API] Ensure saved logs and outputs from containers are accessible to non-admin usersResolved08/24/2016

Related to Arvados - Bug #9679: [Crunch2] Provide feedback when a container is submitted to slurm but does not runResolved07/29/2016

Associated revisions

Revision 96cc8940
Added by Tom Clegg almost 4 years ago

Merge branch '9799-nonadmin-logs'

closes #9799

History

#1 Updated by Tom Clegg almost 4 years ago

  • Subject changed from [Crunch2] Ensure crunch2 live logs work for non-admin users to [Crunch2] Ensure live logs for containers are accessible to non-admin users

#2 Updated by Tom Clegg almost 4 years ago

  • Description updated (diff)
  • Category set to Crunch

#3 Updated by Tom Clegg almost 4 years ago

  • Description updated (diff)
  • Assigned To set to Tom Clegg
  • Target version set to 2016-08-31 sprint
  • Story points set to 1.0

#4 Updated by Tom Clegg almost 4 years ago

923e2bd7 added an "object_owner_uuid" column to the logs table, and adjusted the permission mechanism such that:
  • If log entry L is about object X (L.object_uuid X.uuid), and
  • If X is in project/group P (X.owner_uuid P.uuid) when L was created, and
  • User U can read P, then
  • U can read L.

This makes logs visible to the appropriate users in common cases.

However, it also comes with some subtle behavior that might not be desirable. Say we have another project P2 and another user U2:

can read P can read P2
U yes no
U2 no yes
Say someone moves X from P to P2 at time T. Now:
  • U cannot read X (as expected)
  • U cannot read new logs about X (as expected)
  • U2 can read X (as expected)
  • U2 can read new logs about X (as expected)
  • U can still read logs about X that were created before time T (surprise?)
  • U2 cannot read logs about X that were created before time T (surprise?)
Also, in the case of containers, it's not enough:
  • Container request CR is in project P
  • User U can read project P
  • U can read logs about CR
  • Dispatcher runs as user D (D != U)
  • Dispatcher creates logs about container C
  • C is owned by system user
  • Logs about C are owned by D
  • Only D and admins can see the logs created by dispatcher

The result is that live container logs are only visible to admins.

#5 Updated by Tom Clegg almost 4 years ago

Proposed approach: log entries for container C are effectively (e.g., by eventbus) broadcast to all container requests referencing that container.

In particular:
  • When a websocket client requests log entries for a container request with uuid=CR, websocket should send events with object_uuid = CR and events with object_uuid in (select container_uuid from container_requests where uuid=CR)
  • Ditto REST client

This would be less awkward if our event bus were UUID-channel-oriented and didn't support subscribing to "events matching filters XYZ regardless of object_uuid"

#6 Updated by Tom Clegg almost 4 years ago

Proposed approach 2:

Implement Container.readable_by() such that container C is readable if some container CR is readable and has CR.container_uuid==C.uuid.

The normal permission paths (for non-admins) are irrelevant for Containers.

#8 Updated by Tom Clegg almost 4 years ago

  • Status changed from New to In Progress

#10 Updated by Tom Clegg almost 4 years ago

  • Target version changed from 2016-08-31 sprint to 2016-09-14 sprint

#11 Updated by Tom Clegg almost 4 years ago

  • Story points changed from 1.0 to 0.5

#12 Updated by Lucas Di Pentima almost 4 years ago

Some questions:

  • At apps/workbench/app/models/proxy_work_unit.rb, method live_log_lines(limit): Couldn’t the API server query be improved to avoid the “select” block in Workbench’s ruby code? delegating that filtering to the Postgresql or API server, saving some bandwidth?
  • At services/api/app/models/arvados_model.rb, line 201:
    # Collect the UUIDs of all groups readable by any of the
    # authorized users. If one of these (or the UUID of one of the
    # authorized users themselves) is an object's owner_uuid, that
    # object is readable.
    owner_uuids = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }

Is there a possibility of owner_uuid having duplicated uuids coming from groups_i_can(:read)?

  • At services/api/app/models/arvados_model.rb, mehod readable_by(*users_list): Is there a style standard for method declarations? For example, readable_by() declaration in arvados_model uses parenthesis, but the same method on container and log models, don’t.

#13 Updated by Tom Clegg almost 4 years ago

Lucas Di Pentima wrote:

  • At apps/workbench/app/models/proxy_work_unit.rb, method live_log_lines(limit): Couldn’t the API server query be improved to avoid the “select” block in Workbench’s ruby code? delegating that filtering to the Postgresql or API server, saving some bandwidth?
I thought about this too, but I left it because
  • API doesn't yet have good support for filtering on serialized fields
  • We won't want this filter for very long -- in #9679 we want Workbench to do/say something about other events, like "state changed from Queued to Locked", even if they don't come with their own properties["text"]
  • In practice I don't think there will be a lot of discarded entries

Is there a possibility of owner_uuid having duplicated uuids coming from groups_i_can(:read)?

Yes -- added a "uniq" here to save a bit of extra noise in the queries (is that what you mean?)

  • At services/api/app/models/arvados_model.rb, mehod readable_by(*users_list): Is there a style standard for method declarations? For example, readable_by() declaration in arvados_model uses parenthesis, but the same method on container and log models, don’t.

Added parens, and added link to https://github.com/bbatsov/ruby-style-guide ("Use def with parentheses when there are parameters") .

#14 Updated by Lucas Di Pentima almost 4 years ago

LGTM, please merge.

#15 Updated by Tom Clegg almost 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

Applied in changeset arvados|commit:96cc8940e7926453f4728c5aec1374e7b99db201.

Also available in: Atom PDF