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.

Added by Nico César about 4 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
11/18/2016
Due date:
% Done:

50%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

Currently, the setup button (in the Admin tab that gives login permissions on shell nodes) is responsible for creating the can_read link permission to all_users. The link is also added when the user is created if the "auto setup new user" feature is enabled.

If auto-setup is disabled, and the admin doesn't want to set up VM access (e.g., external LDAP and other tools provide the users), the admin might activate users by changing the is_active flag to true instead of using the "Setup" button. This nearly works, except that the "all users" link isn't created, and sharing features in Workbench don't work as expected for those users.

We should support this admin workflow by adding the appropriate link (and doing any other needed setup activities) if needed when a user's is_active flag changes to true.

Implementation

Run auto_setup_new_user as an after_update hook when the is_active flag changes to true.


Subtasks

Task #11833: Review 10557-auto-setupResolvedTom Clegg

Task #11873: Review 10557-setup-cleanupResolvedTom Clegg


Related issues

Related to Arvados - Bug #12178: Activating an account (setting is_active from false to true) does not email the new userNew08/25/2017

Associated revisions

Revision 021d36f1
Added by Tom Clegg over 3 years ago

Merge branch '10557-auto-setup'

refs #10557

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

Revision b1f94004
Added by Tom Clegg over 3 years ago

Merge branch '10557-setup-cleanup'

refs #10557

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Nico César about 4 years ago

  • Project changed from Arvados Private to Arvados

#2 Updated by Tom Clegg about 4 years ago

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.

#3 Updated by Tom Clegg about 4 years ago

  • Subject changed from Create can_read permission link to all_users when user is created. to 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.

#4 Updated by Tom Clegg almost 4 years ago

  • Description updated (diff)

#5 Updated by Tom Morris over 3 years ago

  • Target version set to 2017-06-21 sprint

#6 Updated by Tom Clegg over 3 years ago

  • Assigned To set to Tom Clegg

#7 Updated by Tom Clegg over 3 years ago

  • Status changed from New to In Progress

#9 Updated by Peter Amstutz over 3 years ago

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?

#10 Updated by Tom Clegg over 3 years ago

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

#11 Updated by Peter Amstutz over 3 years ago

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

#12 Updated by Tom Clegg over 3 years ago

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?

#14 Updated by Peter Amstutz over 3 years ago

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?

#15 Updated by Tom Clegg over 3 years ago

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.

#16 Updated by Peter Amstutz over 3 years ago

ok, merge it!

#17 Updated by Tom Clegg over 3 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF