https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422016-08-17T13:52:20ZArvadosArvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=419912016-08-17T13:52:20ZTom Cleggtom@curii.com
<ul><li><strong>Subject</strong> changed from <i>[Crunch2] Ensure crunch2 live logs work for non-admin users</i> to <i>[Crunch2] Ensure live logs for containers are accessible to non-admin users</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=419922016-08-17T13:56:08ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/41992/diff?detail_id=40666">diff</a>)</li><li><strong>Category</strong> set to <i>Crunch</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=420532016-08-17T19:40:47ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/42053/diff?detail_id=40731">diff</a>)</li><li><strong>Assigned To</strong> set to <i>Tom Clegg</i></li><li><strong>Target version</strong> set to <i>2016-08-31 sprint</i></li><li><strong>Story points</strong> set to <i>1.0</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=421202016-08-19T15:24:13ZTom Cleggtom@curii.com
<ul></ul><a class="changeset" title="- Added object_owner_uuid to logs table, which records the owner of the object being described ..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/923e2bd7d962f1a0feefc73ae4f4531c8235a591">923e2bd7</a> added an "object_owner_uuid" column to the logs table, and adjusted the permission mechanism such that:
<ul>
<li>If log entry L is about object X (L.object_uuid X.uuid), and</li>
<li>If X is in project/group P (X.owner_uuid P.uuid) when L was created, and</li>
<li>User U can read P, then</li>
<li>U can read L.</li>
</ul>
<p>This makes logs visible to the appropriate users in common cases.</p>
<p>However, it also comes with some subtle behavior that might not be desirable. Say we have another project P2 and another user U2:</p>
<table>
<tr>
<td></td>
<td>can read P</td>
<td>can read P2</td>
</tr>
<tr>
<td>U</td>
<td>yes</td>
<td>no</td>
</tr>
<tr>
<td>U2</td>
<td>no</td>
<td>yes</td>
</tr>
</table>
Say someone moves X from P to P2 at time T. Now:
<ul>
<li>U cannot read X (as expected)</li>
<li>U cannot read new logs about X (as expected)</li>
<li>U2 can read X (as expected)</li>
<li>U2 can read new logs about X (as expected)</li>
<li>U can still read logs about X that were created before time T (surprise?)</li>
<li>U2 cannot read logs about X that were created before time T (surprise?)</li>
</ul>
Also, in the case of containers, it's not enough:
<ul>
<li>Container request CR is in project P</li>
<li>User U can read project P</li>
<li>U can read logs about CR</li>
<li>Dispatcher runs as user D (D != U)</li>
<li>Dispatcher creates logs about container C</li>
<li>C is owned by system user</li>
<li>Logs about C are owned by D</li>
<li>Only D and admins can see the logs created by dispatcher</li>
</ul>
<p>The result is that live container logs are only visible to admins.</p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=422772016-08-24T15:48:42ZTom Cleggtom@curii.com
<ul></ul><p>Proposed approach: log entries for container C are effectively (e.g., by eventbus) broadcast to all container requests referencing that container.</p>
In particular:
<ul>
<li>When a websocket client requests log entries for a container request with uuid=CR, websocket should send events with <code>object_uuid = CR</code> <em>and</em> events with <code>object_uuid in (select container_uuid from container_requests where uuid=CR)</code></li>
<li>Ditto REST client</li>
</ul>
<p>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"</p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=422842016-08-24T18:52:24ZTom Cleggtom@curii.com
<ul></ul><p>Proposed approach 2:</p>
<p>Implement Container.readable_by() such that container C is readable if some container CR is readable and has CR.container_uuid==C.uuid.</p>
<p>The normal permission paths (for non-admins) are irrelevant for Containers.</p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=424362016-08-30T15:49:15ZTom Cleggtom@curii.com
<ul></ul><p>9799-nonadmin-logs @ <a class="changeset" title="9799: Fix show/hide "cancel container req" button: check ArvadosBase#editable?, and use CR priori..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/4d27755da78787d662259f8e63892b6c9591b971">4d27755</a> → <a class="external" href="https://ci.curoverse.com/job/developer-test-job/165/">https://ci.curoverse.com/job/developer-test-job/165/</a></p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=424372016-08-30T15:49:37ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=424922016-08-31T13:23:04ZTom Cleggtom@curii.com
<ul></ul><p>Now at <a class="changeset" title="9799: Update tests: non-admin user can see container assigned to committed CR." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/dcf7f7d492830f11b58e691d127849c6781c8244">dcf7f7d</a>, <a class="external" href="https://ci.curoverse.com/job/developer-test-job/172/">https://ci.curoverse.com/job/developer-test-job/172/</a></p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=425092016-08-31T19:32:50ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>2016-08-31 sprint</i> to <i>2016-09-14 sprint</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=425302016-08-31T19:43:19ZTom Cleggtom@curii.com
<ul><li><strong>Story points</strong> changed from <i>1.0</i> to <i>0.5</i></li></ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=426022016-09-01T16:02:06ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>Some questions:</p>
<ul>
<li>At <code>apps/workbench/app/models/proxy_work_unit.rb</code>, method <code>live_log_lines(limit)</code>: 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?</li>
</ul>
<ul>
<li>At <code>services/api/app/models/arvados_model.rb</code>, line 201:</li>
</ul>
<pre>
# 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) }
</pre>
<p>Is there a possibility of owner_uuid having duplicated uuids coming from groups_i_can(:read)?</p>
<ul>
<li>At <code>services/api/app/models/arvados_model.rb</code>, mehod <code>readable_by(*users_list)</code>: 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.</li>
</ul> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=427192016-09-05T15:16:24ZTom Cleggtom@curii.com
<ul></ul><p>Lucas Di Pentima wrote:</p>
<blockquote>
<ul>
<li>At <code>apps/workbench/app/models/proxy_work_unit.rb</code>, method <code>live_log_lines(limit)</code>: 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?</li>
</ul>
</blockquote>
I thought about this too, but I left it because
<ul>
<li>API doesn't yet have good support for filtering on serialized fields</li>
<li>We won't want this filter for very long -- in <a class="issue tracker-1 status-3 priority-4 priority-default closed parent" title="Bug: [Crunch2] Provide feedback when a container is submitted to slurm but does not run (Resolved)" href="https://dev.arvados.org/issues/9679">#9679</a> 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 <code>properties["text"]</code></li>
<li>In practice I don't think there will be a lot of discarded entries</li>
</ul>
<blockquote>
<p>Is there a possibility of owner_uuid having duplicated uuids coming from groups_i_can(:read)?</p>
</blockquote>
<p>Yes -- added a "uniq" here to save a bit of extra noise in the queries (is that what you mean?)</p>
<blockquote>
<ul>
<li>At <code>services/api/app/models/arvados_model.rb</code>, mehod <code>readable_by(*users_list)</code>: 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.</li>
</ul>
</blockquote>
<p>Added parens, and added link to <a class="external" href="https://github.com/bbatsov/ruby-style-guide">https://github.com/bbatsov/ruby-style-guide</a> ("Use <code>def</code> with parentheses when there are parameters") .</p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=427242016-09-05T16:06:33ZLucas Di Pentimalucas.dipentima@curii.com
<ul></ul><p>LGTM, please merge.</p> Arvados - Bug #9799: [Crunch2] Ensure live logs for containers are accessible to non-admin usershttps://dev.arvados.org/issues/9799?journal_id=427292016-09-05T20:25:05ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>50</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:96cc8940e7926453f4728c5aec1374e7b99db201.</p>