Arvados

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.

10557-auto-setup @ 2b62223c9ba420208b9f293825e7f6ae3f50f95b

Is the belt-and-suspenders check strictly necessary? It does this:

:if => Proc.new { |user|
    ![system_user_uuid, anonymous_user_uuid].include?(user.uuid)
  }

and then does this:

return if [system_user_uuid, anonymous_user_uuid].include?(self.uuid)

Why does the setup method both set @object (initially nil, then set to a User object) and but also set the object_found flag? Seems like the only case that object_found is false is when the object is newly created? Which can be detected with new_record? I think?

Peter Amstutz wrote:

Is the belt-and-suspenders check strictly necessary?

No, I just added one without removing the other. Fixed.

Why does the setup method both set @object (initially nil, then set to a User object) and but also set the object_found flag? Seems like the only case that object_found is false is when the object is newly created? Which can be detected with new_record? I think?

AFAICT "object_found" means "user already existed before setup() was called", yes. I don't think new_record? 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.

10557-auto-setup @ eb0012d203974e54023dfcac6e04fd4c2c40270f

Tom Clegg wrote:

AFAICT "object_found" means "user already existed before setup() was called", yes. I don't think new_record? 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.

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.

10557-auto-setup @ eb0012d203974e54023dfcac6e04fd4c2c40270f

Peter Amstutz wrote:

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.

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?

10557-setup-cleanup @ 74b3ad1f061185ca695e8bbead723b5212bbb06a

Get out your whip and gun, its time for some code archeology.

A few things I found confusing:

• 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.

• full_repo_name can be nil, which means that no repo will be assigned or created, but this doesn't get checked until User.create_user_repo_link. A comment might make it a little more obvious what is going on.

• 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.

• Could UserNotifier.account_is_setup go in the setup method of the model, instead the controller?

Peter Amstutz wrote:

Get out your whip and gun, its time for some code archeology.

Is it, though? ;)

A few things I found confusing:

• 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.

• full_repo_name can be nil, which means that no repo will be assigned or created, but this doesn't get checked until User.create_user_repo_link. A comment might make it a little more obvious what is going on.

OK, I'm now un-volunteering to do further code cleanup in this area as part of this bugfix.

• 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.

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.

Normally the repo name would be given as just "secrets", of course.

• Could UserNotifier.account_is_setup go in the setup method of the model, instead the controller?

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.

ok, merge it!