user creation api extension: Added temp password feature from admin ui into users api call
Created by: duk3luk3
This pull request
- fixes outdated comments in the POST /users api call
- adds the parameter expired_password to the call which if set will cause the password to be set expired
- adds the parameter force_random_password to the call which if set will cause the password to be randomly generated
To properly handle the more complex logic needed to make this change backwards-compatible and robust, some code had to be added. Comments were inserted where expedient.
I believe it is self-evident that adding the "generate temp password" feature that is already in the admin ui to the api call is useful. It enables for example batch-adding users with random passwords.
Merge request reports
Activity
Created by: dosire
Can you please include tests for the new functionality? If you can transform if/else statements to object calls that would be great too.
By Administrator on 2013-09-11T07:19:42 (imported from GitLab project)
By Administrator on 2013-09-11T07:19:42 (imported from GitLab)
Created by: duk3luk3
I've updated my PR with a test and some bugfixes.
I've also changed the other tests for the POST /users API to work as intended; this leads to one of them failing.
I don't know which of the if-statements I should change. Do you mean using validators in the user model instead of testing for correct parameters in the API call handler?
By Administrator on 2013-09-16T17:17:13 (imported from GitLab project)
By Administrator on 2013-09-16T17:17:13 (imported from GitLab)
39 # projects_limit - Number of projects user can create 40 # extern_uid - External authentication provider UID 41 # provider - External provider 42 # bio - Bio 33 # email (required) - Email 34 # password (required unless force_random_password is set) - Password 35 # name (required) - Name 36 # username (required) - username 37 # skype - Skype ID 38 # linkedin - Linkedin 39 # twitter - Twitter account 40 # projects_limit - Number of projects user can create 41 # extern_uid - External authentication provider UID 42 # provider - External provider 43 # bio - Bio 44 # expired_password (true/false) - password is set expired 40 # extern_uid - External authentication provider UID 41 # provider - External provider 42 # bio - Bio 33 # email (required) - Email 34 # password (required unless force_random_password is set) - Password 35 # name (required) - Name 36 # username (required) - username 37 # skype - Skype ID 38 # linkedin - Linkedin 39 # twitter - Twitter account 40 # projects_limit - Number of projects user can create 41 # extern_uid - External authentication provider UID 42 # provider - External provider 43 # bio - Bio 44 # expired_password (true/false) - password is set expired 45 # force_random_password (true/false; required unless password is set) - generate random password for user 44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] Created by: dosire
Why not integrate this as the else clause of an adapted clause stating at line 65
if force_random
elsif params[:password].present? ... else render_api_error end
By Administrator on 2013-10-23T15:10:57 (imported from GitLab project)
By Administrator on 2013-10-23T15:10:57 (imported from GitLab)
44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] Created by: duk3luk3
I think it's better to throw an API error if both options are set at the same time. Introducing an implicit preference violates the principle of least surprise.
By Administrator on 2013-10-23T15:10:57 (imported from GitLab project)
By Administrator on 2013-10-23T15:10:57 (imported from GitLab)
44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] Created by: dosire
I think both if statements are related so it is better to bundle them. In the second statement you already expect a params[:password] to be set if there is no force_random. It seems better to put the exception handling there as well. This will also reduce the paths through the code (on first sight) from 4 to 3 (from two if/else statements to one if/elsif/else statement).
By Administrator on 2013-10-23T15:10:57 (imported from GitLab project)
By Administrator on 2013-10-23T15:10:57 (imported from GitLab)
44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] Created by: duk3luk3
Would you prefer it like this?
https://gist.github.com/duk3luk3/6593934
By Administrator on 2013-10-23T15:10:57 (imported from GitLab project)
By Administrator on 2013-10-23T15:10:57 (imported from GitLab)
44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] 44 47 # POST /users 45 48 post do 46 49 authenticated_as_admin! 47 required_attributes! [:email, :password, :name, :username] 48 attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 49 user = User.build_user(attrs, as: :admin) 50 required_attributes! [:email, :name, :username] 51 52 attrs = attributes_for_keys [:email, :name, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio] 53 54 #parse password strategy params 55 expired = params[:expired_password] && (params[:expired_password].to_i > 0) 56 force_random = params[:force_random_password] && (params[:force_random_password].to_i > 0) 57 58 if params[:password] && !force_random 59 attrs[:password] = params[:password] Created by: dosire
@duk3luk3 You're welcome, thank you for your efforts. I've asked Dmitriy to take a look at this.
By Administrator on 2013-09-23T07:33:18 (imported from GitLab project)
By Administrator on 2013-09-23T07:33:18 (imported from GitLab)
Created by: duk3luk3
Pulled in 6-1-stable and added a fix for a syntax error I had introduced. Will pull in 6-2-stable and then master as soon as I get to it.
By Administrator on 2013-10-23T15:50:46 (imported from GitLab project)
By Administrator on 2013-10-23T15:50:46 (imported from GitLab)
Created by: dosire
@duk3luk3 We want to start sending users an activation link instead of a temporary password when they create an account. Therefore it doesn't seem wise to add the temporary password to the API anymore. Also something I should have mentioned earlier, please try to keep PR's as small as possible. If you want to update the documentation do that in one PR.
By Administrator on 2013-12-09T10:43:41 (imported from GitLab project)
By Administrator on 2013-12-09T10:43:41 (imported from GitLab)
