https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422016-11-18T13:19:16ZArvadosArvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=454582016-11-18T13:19:16ZNico César
<ul><li><strong>Project</strong> changed from <i>35</i> to <i>Arvados</i></li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=455152016-11-21T16:16:50ZTom Cleggtom@curii.com
<ul></ul><p>The link should be created when the user is activated (not created). I think we can make this (and any other user-activation tasks?) happen automatically whenever is_active changes to true, so it's not as easy for an admin to make a "half-setup" user.</p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=455492016-11-21T20:46:34ZTom Cleggtom@curii.com
<ul><li><strong>Subject</strong> changed from <i>Create can_read permission link to all_users when user is created. </i> to <i>Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.</i></li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=467292017-01-03T17:36:29ZTom Cleggtom@curii.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/46729/diff?detail_id=44976">diff</a>)</li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=524172017-06-07T16:43:03ZTom Morristfmorris@veritasgenetics.com
<ul><li><strong>Target version</strong> set to <i>2017-06-21 sprint</i></li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=524892017-06-07T19:16:45ZTom Cleggtom@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Tom Clegg</i></li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526202017-06-13T17:41:06ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526322017-06-14T01:05:04ZTom Cleggtom@curii.com
<ul></ul><p>10557-auto-setup @ <a class="changeset" title="10557: Always run user setup procedure when is_active becomes true. Arvados-DCO-1.1-Signed-off-b..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/2b62223c9ba420208b9f293825e7f6ae3f50f95b">2b62223c9ba420208b9f293825e7f6ae3f50f95b</a></p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526492017-06-14T15:25:47ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Is the belt-and-suspenders check strictly necessary? It does this:</p>
<pre>
:if => Proc.new { |user|
![system_user_uuid, anonymous_user_uuid].include?(user.uuid)
}
</pre>
<p>and then does this:</p>
<pre>
return if [system_user_uuid, anonymous_user_uuid].include?(self.uuid)
</pre>
<p>Why does the <code>setup</code> method both set <code>@object</code> (initially nil, then set to a User object) and but also set the <code>object_found</code> flag? Seems like the only case that object_found is false is when the object is newly created? Which can be detected with <code>new_record?</code> I think?</p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526532017-06-14T15:37:49ZTom Cleggtom@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Is the belt-and-suspenders check strictly necessary?</p>
</blockquote>
<p>No, I just added one without removing the other. Fixed.</p>
<blockquote>
<p>Why does the <code>setup</code> method both set <code>@object</code> (initially nil, then set to a User object) and but also set the <code>object_found</code> flag? Seems like the only case that object_found is false is when the object is newly created? Which can be detected with <code>new_record?</code> I think?</p>
</blockquote>
<p>AFAICT "object_found" means "user already existed before setup() was called", yes. I don't think <code>new_record?</code> would be true any more after create!(). Anyway, I was mainly trying to reduce the two setup methods to one, so I thought I'd leave well enough alone here.</p>
<p>10557-auto-setup @ <a class="changeset" title="10557: Remove redundant hook condition. Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse...." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/eb0012d203974e54023dfcac6e04fd4c2c40270f">eb0012d203974e54023dfcac6e04fd4c2c40270f</a></p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526542017-06-14T15:45:41ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Tom Clegg wrote:</p>
<blockquote>
<p>AFAICT "object_found" means "user already existed before setup() was called", yes. I don't think <code>new_record?</code> would be true any more after create!(). Anyway, I was mainly trying to reduce the two setup methods to one, so I thought I'd leave well enough alone here.</p>
</blockquote>
<p>Right, you removed one branch based on object_found. There's another branch based on object_found but I don't quite understand why it makes the distinction.</p>
<blockquote>
<p>10557-auto-setup @ <a class="changeset" title="10557: Remove redundant hook condition. Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse...." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/eb0012d203974e54023dfcac6e04fd4c2c40270f">eb0012d203974e54023dfcac6e04fd4c2c40270f</a></p>
</blockquote> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=526582017-06-14T19:31:20ZTom Cleggtom@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Right, you removed one branch based on object_found. There's another branch based on object_found but I don't quite understand why it makes the distinction.</p>
</blockquote>
<p>Looks like in the new user case, "username/" gets prepended to the given repo name even if it already starts with "username/". Kinda sounds like a bug. Seems like we could use more cleanup in this area?</p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=527832017-06-20T14:33:41ZTom Cleggtom@curii.com
<ul></ul><p>10557-setup-cleanup @ <a class="changeset" title="10557: Tidy up users#setup controller. Simplify long conditional, and fix bug where admin asks f..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/74b3ad1f061185ca695e8bbead723b5212bbb06a">74b3ad1f061185ca695e8bbead723b5212bbb06a</a></p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=528002017-06-20T17:51:46ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>Get out your whip and gun, its time for some code archeology.</p>
<p>A few things I found confusing:</p>
<ul>
<li>The setup method can be used to both create a new user and set up an existing user. That should probably be more clearly reflected in a code comment.</li>
</ul>
<ul>
<li><code>full_repo_name</code> can be nil, which means that no repo will be assigned or created, but this doesn't get checked until <code>User.create_user_repo_link</code>. A comment might make it a little more obvious what is going on.</li>
</ul>
<ul>
<li>I could run the setup method with {user => "peter", repo_name => "tomclegg/secrets"} and it'll happily give me manage permission to that repository. I don't think it's a security hole because only admins can do setup, but this behavior intended/desirable? Seems like it mostly provides an opportunity for the wrong repository to be assigned by mistake.</li>
</ul>
<ul>
<li>Could <code>UserNotifier.account_is_setup</code> go in the setup method of the model, instead the controller?</li>
</ul> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=528072017-06-20T19:28:24ZTom Cleggtom@curii.com
<ul></ul><p>Peter Amstutz wrote:</p>
<blockquote>
<p>Get out your whip and gun, its time for some code archeology.</p>
</blockquote>
<p>Is it, though? ;)</p>
<blockquote>
<p>A few things I found confusing:</p>
<ul>
<li>The setup method can be used to both create a new user and set up an existing user. That should probably be more clearly reflected in a code comment.</li>
</ul>
<ul>
<li><code>full_repo_name</code> can be nil, which means that no repo will be assigned or created, but this doesn't get checked until <code>User.create_user_repo_link</code>. A comment might make it a little more obvious what is going on.</li>
</ul>
</blockquote>
<p>OK, I'm now un-volunteering to do further code cleanup in this area as part of this bugfix.</p>
<blockquote>
<ul>
<li>I could run the setup method with {user => "peter", repo_name => "tomclegg/secrets"} and it'll happily give me manage permission to that repository. I don't think it's a security hole because only admins can do setup, but this behavior intended/desirable? Seems like it mostly provides an opportunity for the wrong repository to be assigned by mistake.</li>
</ul>
</blockquote>
<p>To me, the admin's intent seems pretty clear, and handling such a request by adding a "peter/tomclegg/secrets" repo would seem surprising/wrong.</p>
<p>Normally the repo name would be given as just "secrets", of course.</p>
<blockquote>
<ul>
<li>Could <code>UserNotifier.account_is_setup</code> go in the setup method of the model, instead the controller?</li>
</ul>
</blockquote>
<p>It could, but I don't think there's any particular benefit: there's no way for the "update" call (or the UI feature we're fixing here) to set the "want notification" flag.</p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=528092017-06-20T19:47:04ZPeter Amstutzpeter.amstutz@curii.com
<ul></ul><p>ok, merge it!</p> Arvados - Bug #10557: Perform full user setup procedure when (incl. can_read permission link to all_users) when a user is activated by updating the is_active flag.https://dev.arvados.org/issues/10557?journal_id=528112017-06-20T20:32:54ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li></ul>