Bug #9799
closed[Crunch2] Ensure live logs for containers are accessible to non-admin users
Added by Tom Clegg over 8 years ago. Updated over 8 years ago.
100%
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)
Updated by Tom Clegg over 8 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
Updated by Tom Clegg over 8 years ago
- Description updated (diff)
- Category set to Crunch
Updated by Tom Clegg over 8 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
Updated by Tom Clegg over 8 years ago
- 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 |
- 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?)
- 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.
Updated by Tom Clegg over 8 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 withobject_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"
Updated by Tom Clegg over 8 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.
Updated by Tom Clegg over 8 years ago
9799-nonadmin-logs @ 4d27755 → https://ci.curoverse.com/job/developer-test-job/165/
Updated by Tom Clegg over 8 years ago
Updated by Tom Clegg over 8 years ago
- Target version changed from 2016-08-31 sprint to 2016-09-14 sprint
Updated by Lucas Di Pentima over 8 years ago
Some questions:
- At
apps/workbench/app/models/proxy_work_unit.rb
, methodlive_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
, mehodreadable_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.
Updated by Tom Clegg over 8 years ago
Lucas Di Pentima wrote:
I thought about this too, but I left it because
- At
apps/workbench/app/models/proxy_work_unit.rb
, methodlive_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?
- 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
, mehodreadable_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") .
Updated by Tom Clegg over 8 years ago
- Status changed from In Progress to Resolved
- % Done changed from 50 to 100
Applied in changeset arvados|commit:96cc8940e7926453f4728c5aec1374e7b99db201.