Skip to content

[CLOUDSTACK-5235] ask users current password when they are executing a password update #2574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rafaelweingartner merged 7 commits into from
May 2, 2018
Merged

[CLOUDSTACK-5235] ask users current password when they are executing a password update #2574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5989267
[CLOUDSTACK-5235] Force users to enter old password when updating pas…
rafaelweingartner Apr 13, 2018
686f886
Formatting for checkstyle
rafaelweingartner Apr 16, 2018
f7f1079
Remove an unused import in AccountManagerImpl
rafaelweingartner Apr 16, 2018
6f0dde6
Apply Nitin's suggestions
rafaelweingartner Apr 17, 2018
0b039bf
Change 'oldPassword' to 'currentPassword'
rafaelweingartner Apr 25, 2018
31c10c7
Second review of Resmo
rafaelweingartner Apr 25, 2018
c86ccb2
Fix typos found by Nitin
rafaelweingartner May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 9 additions & 36 deletions api/src/main/java/com/cloud/user/AccountService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,53 +23,28 @@
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
import org.apache.cloudstack.api.command.admin.user.RegisterCmd;
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;

import com.cloud.domain.Domain;
import com.cloud.exception.PermissionDeniedException;
import com.cloud.offering.DiskOffering;
import com.cloud.offering.ServiceOffering;


public interface AccountService {

/**
* Creates a new user and account, stores the password as is so encrypted passwords are recommended.
*
Copy link
Contributor

@nitin-maharana nitin-maharana Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelweingartner, Thanks for the fix. I think it's better to keep the comments, would be helpful in the documentation. Would be great if you describe a bit about each parameter.

Copy link
Member Author

@rafaelweingartner rafaelweingartner Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I do like Java documentation; however, in this case, the use of @param does not bring benefits. I mean, if each one of the parameters was fully described it would be ok, but that is not what happened here. That is why I removed them.

I am sorry Nitin, but I would rather not document those parameters. Instead, we should get rid of these methods with a lot of parameters. My changes affect only the updateUser method (I already refactored that whole method). Afterwards, I executed a cleanup in the classes that I had modified some code.

* @param userName
* TODO
* @param password
* TODO
* @param firstName
* TODO
* @param lastName
* TODO
* @param email
* TODO
* @param timezone
* TODO
* @param accountName
* TODO
* @param accountType
* TODO
* @param domainId
* TODO
* @param networkDomain
* TODO
*
* @return the user if created successfully, null otherwise
*/
UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName,
short accountType, Long roleId, Long domainId, String networkDomain, Map<String, String> details, String accountUUID, String userUUID);
UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName, short accountType, Long roleId, Long domainId,
String networkDomain, Map<String, String> details, String accountUUID, String userUUID);

UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName, short accountType, Long roleId, Long domainId, String networkDomain,
Map<String, String> details, String accountUUID, String userUUID, User.Source source);
UserAccount createUserAccount(String userName, String password, String firstName, String lastName, String email, String timezone, String accountName, short accountType, Long roleId, Long domainId,
String networkDomain, Map<String, String> details, String accountUUID, String userUUID, User.Source source);

/**
* Locks a user by userId. A locked user cannot access the API, but will still have running VMs/IP addresses
* allocated/etc.
*
* @param userId
* @return UserAccount object
*/
UserAccount lockUser(long userId);

Expand All @@ -79,8 +54,7 @@ UserAccount createUserAccount(String userName, String password, String firstName

User createUser(String userName, String password, String firstName, String lastName, String email, String timeZone, String accountName, Long domainId, String userUUID);

User createUser(String userName, String password, String firstName, String lastName, String email, String timeZone, String accountName, Long domainId, String userUUID,
User.Source source);
User createUser(String userName, String password, String firstName, String lastName, String email, String timeZone, String accountName, Long domainId, String userUUID, User.Source source);

boolean isAdmin(Long accountId);

Expand All @@ -90,7 +64,7 @@ User createUser(String userName, String password, String firstName, String lastN

UserAccount getActiveUserAccount(String username, Long domainId);

UserAccount updateUser(Long userId, String firstName, String lastName, String email, String userName, String password, String apiKey, String secretKey, String timeZone);
UserAccount updateUser(UpdateUserCmd updateUserCmd);

Account getActiveAccountById(long accountId);

Expand Down Expand Up @@ -128,15 +102,14 @@ User createUser(String userName, String password, String firstName, String lastN

void checkAccess(User user, ControlledEntity entity);

void checkAccess(Account account, AccessType accessType, boolean sameOwner, String apiName,
ControlledEntity... entities) throws PermissionDeniedException;
void checkAccess(Account account, AccessType accessType, boolean sameOwner, String apiName, ControlledEntity... entities) throws PermissionDeniedException;

Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly);

/**
* returns the user account object for a given user id
* @param userId user id
* @return useraccount object if it exists else null
* @return {@link UserAccount} object if it exists else null
*/
UserAccount getUserAccountById(Long userId);

Expand Down
4 changes: 2 additions & 2 deletions api/src/main/java/org/apache/cloudstack/api/ApiConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ public class ApiConstants {
public static final String PARENT_DOMAIN_ID = "parentdomainid";
public static final String PARENT_TEMPLATE_ID = "parenttemplateid";
public static final String PASSWORD = "password";
public static final String CURRENT_PASSWORD = "currentpassword";
public static final String SHOULD_UPDATE_PASSWORD = "update_passwd_on_host";
public static final String NEW_PASSWORD = "new_password";
public static final String PASSWORD_ENABLED = "passwordenabled";
public static final String SSHKEY_ENABLED = "sshkeyenabled";
public static final String PATH = "path";
Expand Down Expand Up @@ -727,4 +727,4 @@ public enum VMDetails {
public enum DomainDetails {
all, resource, min;
}
}
}
30 changes: 26 additions & 4 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import com.cloud.user.UserAccount;

@APICommand(name = "updateUser", description = "Updates a user account", responseObject = UserResponse.class,
requestHasSensitiveInfo = true, responseHasSensitiveInfo = true)
requestHasSensitiveInfo = true, responseHasSensitiveInfo = true)
public class UpdateUserCmd extends BaseCmd {
public static final Logger s_logger = Logger.getLogger(UpdateUserCmd.class.getName());

Expand Down Expand Up @@ -65,20 +65,22 @@ public class UpdateUserCmd extends BaseCmd {
acceptedOnAdminPort = false)
private String password;

@Parameter(name = ApiConstants.CURRENT_PASSWORD, type = CommandType.STRING, description = "Current password that was being used by the user. You must inform the current password when updating the password.", acceptedOnAdminPort = false)
private String currentPassword;

@Parameter(name = ApiConstants.SECRET_KEY, type = CommandType.STRING, description = "The secret key for the user. Must be specified with userSecretKey")
private String secretKey;

@Parameter(name = ApiConstants.TIMEZONE,
type = CommandType.STRING,
description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
type = CommandType.STRING,
description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
private String timezone;

@Parameter(name = ApiConstants.USERNAME, type = CommandType.STRING, description = "Unique username")
private String username;

@Inject
RegionService _regionService;
private RegionService _regionService;

/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
Expand Down Expand Up @@ -108,6 +110,10 @@ public String getPassword() {
return password;
}

public String getCurrentPassword() {
return currentPassword;
}

public String getSecretKey() {
return secretKey;
}
Expand Down Expand Up @@ -152,4 +158,20 @@ public void execute() {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update user");
}
}

public void setId(Long id) {
this.id = id;
}

public void setFirstname(String firstname) {
this.firstname = firstname;
}

public void setLastname(String lastname) {
this.lastname = lastname;
}

public void setEmail(String email) {
this.email = email;
}
}
7 changes: 0 additions & 7 deletions ...l/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,6 @@ public UserAccount getActiveUserAccount(String username, Long domainId) {
return null;
}

@Override
public UserAccount updateUser(Long userId, String firstName, String lastName, String email, String userName, String password, String apiKey, String secretKey,
String timeZone) {
// TODO Auto-generated method stub
return null;
}

@Override
public User getActiveUser(long arg0) {
return _systemUser;
Expand Down
52 changes: 26 additions & 26 deletions ...thenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,14 @@

import javax.inject.Inject;

import com.cloud.user.Account;
import com.cloud.user.User;
import com.cloud.user.UserAccount;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.BaseListCmd;
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.api.response.LdapUserResponse;
import org.apache.cloudstack.api.response.ListResponse;
Expand All @@ -54,25 +52,23 @@
import com.cloud.exception.NetworkRuleConflictException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.DomainService;
import com.cloud.user.User;
import com.cloud.user.UserAccount;

@APICommand(name = "importLdapUsers", description = "Import LDAP users", responseObject = LdapUserResponse.class, since = "4.3.0",
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
@APICommand(name = "importLdapUsers", description = "Import LDAP users", responseObject = LdapUserResponse.class, since = "4.3.0", requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
public class LdapImportUsersCmd extends BaseListCmd {

public static final Logger s_logger = Logger.getLogger(LdapImportUsersCmd.class.getName());

private static final String s_name = "ldapuserresponse";

@Parameter(name = ApiConstants.TIMEZONE,
type = CommandType.STRING,
description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
@Parameter(name = ApiConstants.TIMEZONE, type = CommandType.STRING, description = "Specifies a timezone for this command. For more information on the timezone parameter, see Time Zone Format.")
private String timezone;

@Parameter(name = ApiConstants.ACCOUNT_TYPE,
type = CommandType.SHORT,
description = "Type of the account. Specify 0 for user, 1 for root admin, and 2 for domain admin")
@Parameter(name = ApiConstants.ACCOUNT_TYPE, type = CommandType.SHORT, description = "Type of the account. Specify 0 for user, 1 for root admin, and 2 for domain admin")
private Short accountType;

@Parameter(name = ApiConstants.ROLE_ID, type = CommandType.UUID, entityType = RoleResponse.class, description = "Creates the account under the specified role.")
Expand All @@ -81,16 +77,13 @@ public class LdapImportUsersCmd extends BaseListCmd {
@Parameter(name = ApiConstants.ACCOUNT_DETAILS, type = CommandType.MAP, description = "details for account used to store specific parameters")
private Map<String, String> details;

@Parameter(name = ApiConstants.DOMAIN_ID,
type = CommandType.UUID,
entityType = DomainResponse.class,
description = "Specifies the domain to which the ldap users are to be "
+ "imported. If no domain is specified, a domain will created using group parameter. If the group is also not specified, a domain name based on the OU information will be "
+ "created. If no OU hierarchy exists, will be defaulted to ROOT domain")
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class, description = "Specifies the domain to which the ldap users are to be "
+ "imported. If no domain is specified, a domain will created using group parameter. If the group is also not specified, a domain name based on the OU information will be "
+ "created. If no OU hierarchy exists, will be defaulted to ROOT domain")
private Long domainId;

@Parameter(name = ApiConstants.GROUP, type = CommandType.STRING, description = "Specifies the group name from which the ldap users are to be imported. "
+ "If no group is specified, all the users will be imported.")
+ "If no group is specified, all the users will be imported.")
private String groupName;

private Domain _domain;
Expand Down Expand Up @@ -121,20 +114,27 @@ private void createCloudstackUserAccount(LdapUser user, String accountName, Doma
} else {
// check if the user exists. if yes, call update
UserAccount csuser = _accountService.getActiveUserAccount(user.getUsername(), domain.getId());
if(csuser == null) {
if (csuser == null) {
s_logger.debug("No user exists with name: " + user.getUsername() + " creating a user in the account: " + accountName);
_accountService.createUser(user.getUsername(), generatePassword(), user.getFirstname(), user.getLastname(), user.getEmail(), timezone, accountName, domain.getId(),
UUID.randomUUID().toString(), User.Source.LDAP);
UUID.randomUUID().toString(), User.Source.LDAP);
} else {
s_logger.debug("account with name: " + accountName + " exist and user with name: " + user.getUsername() + " exists in the account. Updating the account.");
_accountService.updateUser(csuser.getId(), user.getFirstname(), user.getLastname(), user.getEmail(), null, null, null, null, null);
s_logger.debug("Account [name=%s] and user [name=%s] already exist in CloudStack. Executing the user update.");

UpdateUserCmd updateUserCmd = new UpdateUserCmd();
updateUserCmd.setId(csuser.getId());
updateUserCmd.setFirstname(user.getFirstname());
updateUserCmd.setLastname(user.getLastname());
updateUserCmd.setEmail(user.getEmail());

_accountService.updateUser(updateUserCmd);
}
}
}

@Override
public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
ResourceAllocationException, NetworkRuleConflictException {
public void execute()
throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException, NetworkRuleConflictException {
if (getAccountType() == null && getRoleId() == null) {
throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Both account type and role ID are not provided");
}
Expand Down Expand Up @@ -177,7 +177,7 @@ public Long getRoleId() {

private String getAccountName(LdapUser user) {
String finalAccountName = accountName;
if(finalAccountName == null ) {
if (finalAccountName == null) {
finalAccountName = user.getUsername();
}
return finalAccountName;
Expand Down Expand Up @@ -244,7 +244,7 @@ private String generatePassword() throws ServerApiException {
final byte bytes[] = new byte[20];
randomGen.nextBytes(bytes);
return new String(Base64.encode(bytes), "UTF-8");
} catch ( NoSuchAlgorithmException | UnsupportedEncodingException e) {
} catch (NoSuchAlgorithmException | UnsupportedEncodingException e) {
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to generate random password");
}
}
Expand Down
Loading