https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-10-20T20:34:27ZArvadosArvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=166712014-10-20T20:34:27ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/16671/diff?detail_id=15510">diff</a>)</li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=166722014-10-20T20:35:52ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/16672/diff?detail_id=15511">diff</a>)</li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=166752014-10-20T20:47:49ZPeter Amstutzpeter.amstutz@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/16675/diff?detail_id=15512">diff</a>)</li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=166812014-10-20T21:50:47ZTim Piercetwp@curoverse.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/16681/diff?detail_id=15519">diff</a>)</li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=167202014-10-21T18:56:24ZWard Vandewegeward@curii.com
<ul><li><strong>Target version</strong> changed from <i>Bug Triage</i> to <i>Arvados Future Sprints</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=168912014-10-24T17:42:14ZWard Vandewegeward@curii.com
<ul><li><strong>Story points</strong> set to <i>1.0</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=171932014-10-29T19:41:55ZTim Piercetwp@curoverse.com
<ul><li><strong>Target version</strong> changed from <i>Arvados Future Sprints</i> to <i>2014-11-19 sprint</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=171942014-10-29T19:42:26ZTim Piercetwp@curoverse.com
<ul><li><strong>Assigned To</strong> set to <i>Tim Pierce</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=182392014-11-19T20:07:50ZTim Piercetwp@curoverse.com
<ul><li><strong>Target version</strong> changed from <i>2014-11-19 sprint</i> to <i>2014-12-10 sprint</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=187872014-12-04T16:40:42ZTim Piercetwp@curoverse.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=188462014-12-05T22:42:34ZRadhika Chippadaradhika@curoverse.com
<ul></ul><p>Review feedback:</p>
<ul>
<li>Can you call "no_collection_uuids" as "ensure_no_collection_uuids_in_script_params" or something like that?</li>
</ul>
<ul>
<li>May be "return true if recursive_hash_search v, pattern" can be "return recursive_hash_search v, pattern" (same comment for Array)</li>
</ul>
<ul>
<li>Do we want to log these occurrences?</li>
</ul>
<ul>
<li>I like "thing". May be "thing1" and "thing2" even better than "thing" and "pattern" :). Just kidding.</li>
</ul>
<ul>
<li>I am a little concerned about looking for uuid pattern matching in script_parameters. Not sure if I can have a plain text or something like that (like a description) in there where I might enter a collection uuid? Just wondering if we just want to look for collection uuid pattern in script_parameter -> input only? Ignore this question, if it is not meaningful concern for this context.</li>
</ul>
<ul>
<li>Would it make sense to have a fixture with two script parameters (like previous_job_run_with_arvados_sdk_version) and then update the second value (but not changing the uuid) and verify that the update fails? Of course, you said there will be migration coming up soon. So, I am not sure if this will have much value in the long run.</li>
</ul>
<pre>ex: script_parameters:
input: xxxxx-xxxxx-blah
an_integer: "1" </pre>
<ul>
<li>It seems like it would be desirable that the uuid pattern matching is a reusable method? I see two places where uuid pattern matching is implemented in the api server code.</li>
</ul>
<ul>
<li>It appears that the test "job validation succeeds when no collection uuid in script_parameters" is covered by other tests in this file (those that use the job_attrs)?</li>
</ul> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=189052014-12-08T21:47:50ZTim Piercetwp@curoverse.com
<ul></ul><p>Radhika Chippada wrote:</p>
<blockquote>
<p>Review feedback:</p>
<ul>
<li>Can you call "no_collection_uuids" as "ensure_no_collection_uuids_in_script_params" or something like that?</li>
</ul>
</blockquote>
<p>Sure, that's good. Added at <a class="changeset" title="4269: clean up uuid regex matching Code review feedback: * Improved name for validation "no_col..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/050277d9f11f275fb8c2750e5e267e40da36d76a">050277d</a>.</p>
<blockquote>
<ul>
<li>May be "return true if recursive_hash_search v, pattern" can be "return recursive_hash_search v, pattern" (same comment for Array)</li>
</ul>
</blockquote>
<p>No, the recursive hash search shouldn't return right away just because it failed to find the pattern in a single parameter. If that happens, it needs to search the rest of the parameters it finds. This function only returns false if <em>none</em> of the fields in the hash or array it's searching contain the desired pattern.</p>
<blockquote>
<ul>
<li>Do we want to log these occurrences?</li>
</ul>
</blockquote>
<p>I don't think it's necessary -- these errors should become visible to the user immediately and be traceable. But I'm open to adding logging if we think it would be valuable.</p>
<blockquote>
<ul>
<li>I like "thing". May be "thing1" and "thing2" even better than "thing" and "pattern" :). Just kidding.</li>
</ul>
<ul>
<li>I am a little concerned about looking for uuid pattern matching in script_parameters. Not sure if I can have a plain text or something like that (like a description) in there where I might enter a collection uuid? Just wondering if we just want to look for collection uuid pattern in script_parameter -> input only? Ignore this question, if it is not meaningful concern for this context.</li>
</ul>
</blockquote>
<p>It's a reasonable concern. I don't like it very much either, but it is consistent with the matching that we do elsewhere for collection uuids, e.g. <code>arv-copy</code>, which searches the full text of the entire pipeline instance or template for any field that contains a collection uuid anywhere.</p>
<p>I'll ask Tom to confirm whether or not this is a concern.</p>
<blockquote>
<ul>
<li>Would it make sense to have a fixture with two script parameters (like previous_job_run_with_arvados_sdk_version) and then update the second value (but not changing the uuid) and verify that the update fails? Of course, you said there will be migration coming up soon. So, I am not sure if this will have much value in the long run.</li>
</ul>
</blockquote>
<p>Right -- I don't think there's a lot of value in testing that particular case, since our goal when all is said and done to have a database that's free of inappropriate collection uuids and prevents new ones from being added.</p>
<blockquote>
<ul>
<li>It seems like it would be desirable that the uuid pattern matching is a reusable method? I see two places where uuid pattern matching is implemented in the api server code.</li>
</ul>
</blockquote>
<p>Yes, at the very least it would help to define a uuid regex in one place and use it consistently throughout. Added <code>ArvadosModel.uuid_regex</code> at <a class="changeset" title="4269: clean up uuid regex matching Code review feedback: * Improved name for validation "no_col..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/050277d9f11f275fb8c2750e5e267e40da36d76a">050277d</a> and replaced hardcoded uuid regexes throughout the code base.</p>
<blockquote>
<ul>
<li>It appears that the test "job validation succeeds when no collection uuid in script_parameters" is covered by other tests in this file (those that use the job_attrs)?</li>
</ul>
</blockquote>
<p>Yes, that's implicitly satisfied by the other tests. I didn't think it was necessary to add another test that explicitly addresses this case.</p> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=189092014-12-08T21:58:03ZRadhika Chippadaradhika@curoverse.com
<ul></ul><p>Tim:</p>
<p>Agree with your responses. And, thanks for updating the uuid regex portion. It will be quite helpful in the long run.</p>
<p>Regarding test "job validation succeeds when no collection uuid in script_parameters", I meant this test is covered by other tests in the file and hence is not needed. However, if you prefer to leave it in, that is fine.</p>
<p>LGTM</p> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=189752014-12-09T20:53:57ZTim Piercetwp@curoverse.com
<ul></ul><p>Moved the migration work to <a class="issue tracker-1 status-6 priority-4 priority-default closed" title="Bug: [API] Add migration to change collection uuids to portable_data_hash in old job script_parameters (Rejected)" href="https://dev.arvados.org/issues/4756">#4756</a>.</p> Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashhttps://dev.arvados.org/issues/4269?journal_id=189762014-12-09T20:54:05ZTim Piercetwp@curoverse.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul>