Merge pull request #1350 from shapeblue/master-quotalock

Quota: consolidated lockable account check to a method. Added unit tests to check lockablity of various accounts.

Currently normal user and domain admin accounts are eligible for locking.

* pr/1350:
  Quota: consolidated lockable account check to a method. Added unit tests to check lockablity of various accounts

Signed-off-by: Will Stevens <williamstevens@gmail.com>

framework/quota/src/org/apache/cloudstack/quota/QuotaAlertManagerImpl.java

@@ -27,7 +27,6 @@ import com.cloud.user.dao.UserDao;
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.component.ManagerBase;
 import com.cloud.utils.db.TransactionLegacy;
-import com.cloud.utils.exception.CloudRuntimeException;
 import com.google.common.base.Strings;
 import com.sun.mail.smtp.SMTPMessage;
 import com.sun.mail.smtp.SMTPSSLTransport;
@@ -83,6 +82,8 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
     private ConfigurationDao _configDao;
     @Inject
     private QuotaUsageDao _quotaUsage;
+    @Inject
+    private QuotaManager _quotaManager;
 
     private EmailQuotaAlert _emailQuotaAlert;
     private boolean _lockAccountEnforcement = false;
@@ -159,7 +160,7 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
                 }
                 if (accountBalance.compareTo(zeroBalance) < 0) {
                     if (_lockAccountEnforcement && (lockable == 1)) {
-                        if (account.getType() == Account.ACCOUNT_TYPE_NORMAL) {
+                        if (_quotaManager.isLockable(account)) {
                             s_logger.info("Locking account " + account.getAccountName() + " due to quota < 0.");
                             lockAccount(account.getId());
                         }
@@ -383,7 +384,8 @@ public class QuotaAlertManagerImpl extends ManagerBase implements QuotaAlertMana
 
         public void sendQuotaAlert(List<String> emails, String subject, String body) throws MessagingException, UnsupportedEncodingException {
             if (_smtpSession == null) {
-                throw new CloudRuntimeException("Unable to create smtp session.");
+                s_logger.error("Unable to create smtp session.");
+                return;
             }
             SMTPMessage msg = new SMTPMessage(_smtpSession);
             msg.setSender(new InternetAddress(_emailSender, _emailSender));

framework/quota/src/org/apache/cloudstack/quota/QuotaManager.java

@@ -16,10 +16,13 @@
 //under the License.
 package org.apache.cloudstack.quota;
 
+import com.cloud.user.AccountVO;
 import com.cloud.utils.component.Manager;
 
 public interface QuotaManager extends Manager {
 
     boolean calculateQuotaUsage();
 
+    boolean isLockable(AccountVO account);
+
 }

framework/quota/src/org/apache/cloudstack/quota/QuotaManagerImpl.java

@@ -464,4 +464,9 @@ public class QuotaManagerImpl extends ManagerBase implements QuotaManager {
         return quota_usage;
     }
 
+    @Override
+    public boolean isLockable(AccountVO account) {
+        return (account.getType() == AccountVO.ACCOUNT_TYPE_NORMAL || account.getType() == AccountVO.ACCOUNT_TYPE_DOMAIN_ADMIN);
+    }
+
 }

framework/quota/src/org/apache/cloudstack/quota/dao/QuotaBalanceDaoImpl.java

@@ -156,7 +156,7 @@ public class QuotaBalanceDaoImpl extends GenericDaoBase<QuotaBalanceVO, Long> im
                 // get records before startDate to find start balance
                 for (QuotaBalanceVO entry : quotaUsageRecords) {
                     if (s_logger.isDebugEnabled()) {
-                        s_logger.debug("FindQuotaBalIance Entry=" + entry);
+                        s_logger.debug("FindQuotaBalance Entry=" + entry);
                     }
                     if (entry.getCreditsId() > 0) {
                         trimmedRecords.add(entry);

framework/quota/test/org/apache/cloudstack/quota/QuotaManagerImplTest.java

@@ -197,4 +197,42 @@ public class QuotaManagerImplTest extends TestCase {
         Mockito.verify(quotaAcc, Mockito.times(1)).persistQuotaAccount(Mockito.any(QuotaAccountVO.class));
     }
 
+    private AccountVO accountVO = new AccountVO();
+
+    @Test
+    public void testAdminLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_ADMIN);
+        assertFalse(quotaManager.isLockable(accountVO));
+    }
+
+    @Test
+    public void testNormalLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_NORMAL);
+        assertTrue(quotaManager.isLockable(accountVO));
+    }
+
+    @Test
+    public void tesDomainAdmingLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_DOMAIN_ADMIN);
+        assertTrue(quotaManager.isLockable(accountVO));
+    }
+
+    @Test
+    public void testReadOnlyAdminLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_READ_ONLY_ADMIN);
+        assertFalse(quotaManager.isLockable(accountVO));
+    }
+
+    @Test
+    public void testResourceDomainAdminLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN);
+        assertFalse(quotaManager.isLockable(accountVO));
+    }
+
+    @Test
+    public void testProjectLockableAccount() {
+        accountVO.setType(Account.ACCOUNT_TYPE_PROJECT);
+        assertFalse(quotaManager.isLockable(accountVO));
+    }
+
 }

plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaCreditsCmd.java

@@ -123,7 +123,7 @@ public class QuotaCreditsCmd extends BaseCmd {
         if (getValue() == null) {
             throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Please send a valid non-empty quota value");
         }
-        if (getQuotaEnforce() != null && getQuotaEnforce()) {
+        if (getQuotaEnforce() != null) {
             _quotaService.setLockAccount(accountId, getQuotaEnforce());
         }
         if (getMinBalance() != null) {
@@ -133,7 +133,7 @@ public class QuotaCreditsCmd extends BaseCmd {
             throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Please set a value for min balance");
         }
 
-        final QuotaCreditsResponse response = _responseBuilder.addQuotaCredits(accountId, getDomainId(), getValue(), CallContext.current().getCallingUserId());
+        final QuotaCreditsResponse response = _responseBuilder.addQuotaCredits(accountId, getDomainId(), getValue(), CallContext.current().getCallingUserId(), getQuotaEnforce());
         response.setResponseName(getCommandName());
         response.setObjectName("quotacredits");
         setResponseObject(response);

plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilder.java

@@ -51,7 +51,7 @@ public interface QuotaResponseBuilder {
 
     List<QuotaBalanceVO> getQuotaBalance(QuotaBalanceCmd cmd);
 
-    QuotaCreditsResponse addQuotaCredits(Long accountId, Long domainId, Double amount, Long updatedBy);
+    QuotaCreditsResponse addQuotaCredits(Long accountId, Long domainId, Double amount, Long updatedBy, Boolean enforce);
 
     List<QuotaEmailTemplateResponse> listQuotaEmailTemplates(QuotaEmailTemplateListCmd cmd);
 

plugins/database/quota/src/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java

@@ -32,6 +32,7 @@ import org.apache.cloudstack.api.command.QuotaEmailTemplateUpdateCmd;
 import org.apache.cloudstack.api.command.QuotaStatementCmd;
 import org.apache.cloudstack.api.command.QuotaTariffListCmd;
 import org.apache.cloudstack.api.command.QuotaTariffUpdateCmd;
+import org.apache.cloudstack.quota.QuotaManager;
 import org.apache.cloudstack.quota.QuotaService;
 import org.apache.cloudstack.quota.QuotaStatement;
 import org.apache.cloudstack.quota.constant.QuotaConfig;
@@ -98,6 +99,8 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder {
     private AccountManager _accountMgr;
     @Inject
     private QuotaStatement _statement;
+    @Inject
+    private QuotaManager _quotaManager;
 
     @Override
     public QuotaTariffResponse createQuotaTariffResponse(QuotaTariffVO tariff) {
@@ -385,11 +388,10 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder {
     }
 
     @Override
-    public QuotaCreditsResponse addQuotaCredits(Long accountId, Long domainId, Double amount, Long updatedBy) {
+    public QuotaCreditsResponse addQuotaCredits(Long accountId, Long domainId, Double amount, Long updatedBy, Boolean enforce) {
         Date despositedOn = _quotaService.computeAdjustedTime(new Date());
         QuotaBalanceVO qb = _quotaBalanceDao.findLaterBalanceEntry(accountId, domainId, despositedOn);
 
-
         if (qb != null) {
             throw new InvalidParameterValueException("Incorrect deposit date: " + despositedOn + " there are balance entries after this date");
         }
@@ -411,16 +413,12 @@ public class QuotaResponseBuilderImpl implements QuotaResponseBuilder {
         _quotaService.saveQuotaAccount(account, currentAccountBalance, despositedOn);
         if (lockAccountEnforcement) {
             if (currentAccountBalance.compareTo(new BigDecimal(0)) >= 0) {
-                if (account.getType() == Account.ACCOUNT_TYPE_NORMAL) {
+                if (account.getState() == Account.State.locked) {
-                    if (account.getState() == Account.State.locked) {
+                    s_logger.info("UnLocking account " + account.getAccountName() + " , due to positive balance " + currentAccountBalance);
-                        s_logger.info("UnLocking account " + account.getAccountName() + " , due to positive balance " + currentAccountBalance);
+                    _accountMgr.enableAccount(account.getAccountName(), domainId, accountId);
-                        _accountMgr.enableAccount(account.getAccountName(), domainId, accountId);
-                    }
-                } else {
-                    s_logger.warn("Only normal accounts will get locked " + account.getAccountName() + " even if they have run out of quota " + currentAccountBalance);
                 }
             } else { // currentAccountBalance < 0 then lock the account
-                if (account.getState() == Account.State.enabled) {
+                if (_quotaManager.isLockable(account) && account.getState() == Account.State.enabled && enforce) {
                     s_logger.info("Locking account " + account.getAccountName() + " , due to negative balance " + currentAccountBalance);
                     _accountMgr.lockAccount(account.getAccountName(), domainId, accountId);
                 }

plugins/database/quota/src/org/apache/cloudstack/quota/QuotaServiceImpl.java

@@ -274,14 +274,14 @@ public class QuotaServiceImpl extends ManagerBase implements QuotaService, Confi
     }
 
     @Override
-    public void setLockAccount(Long accountId, Boolean state) {
+    public void setLockAccount(Long accountId, Boolean enforce) {
         QuotaAccountVO acc = _quotaAcc.findByIdQuotaAccount(accountId);
         if (acc == null) {
             acc = new QuotaAccountVO(accountId);
-            acc.setQuotaEnforce(state ? 1 : 0);
+            acc.setQuotaEnforce(enforce ? 1 : 0);
             _quotaAcc.persistQuotaAccount(acc);
         } else {
-            acc.setQuotaEnforce(state ? 1 : 0);
+            acc.setQuotaEnforce(enforce ? 1 : 0);
             _quotaAcc.updateQuotaAccount(accountId, acc);
         }
     }

plugins/database/quota/test/org/apache/cloudstack/api/command/QuotaCreditsCmdTest.java

@@ -65,7 +65,7 @@ public class QuotaCreditsCmdTest extends TestCase {
         AccountVO acc = new AccountVO();
         acc.setId(2L);
         Mockito.when(accountService.getActiveAccountByName(Mockito.anyString(), Mockito.anyLong())).thenReturn(acc);
-        Mockito.when(responseBuilder.addQuotaCredits(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyDouble(), Mockito.anyLong())).thenReturn(new QuotaCreditsResponse());
+        Mockito.when(responseBuilder.addQuotaCredits(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyDouble(), Mockito.anyLong(), Mockito.anyBoolean())).thenReturn(new QuotaCreditsResponse());
 
         // No value provided test
         try {
@@ -79,7 +79,7 @@ public class QuotaCreditsCmdTest extends TestCase {
         cmd.execute();
         Mockito.verify(quotaService, Mockito.times(0)).setLockAccount(Mockito.anyLong(), Mockito.anyBoolean());
         Mockito.verify(quotaService, Mockito.times(1)).setMinBalance(Mockito.anyLong(), Mockito.anyDouble());
-        Mockito.verify(responseBuilder, Mockito.times(1)).addQuotaCredits(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyDouble(), Mockito.anyLong());
+        Mockito.verify(responseBuilder, Mockito.times(1)).addQuotaCredits(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyDouble(), Mockito.anyLong(), Mockito.anyBoolean());
 
 
     }

plugins/database/quota/test/org/apache/cloudstack/api/response/QuotaResponseBuilderImplTest.java

@@ -150,7 +150,7 @@ public class QuotaResponseBuilderImplTest extends TestCase {
         account.setState(Account.State.locked);
         Mockito.when(accountDao.findById(Mockito.anyLong())).thenReturn(account);
 
-        QuotaCreditsResponse resp = quotaResponseBuilder.addQuotaCredits(accountId, domainId, amount, updatedBy);
+        QuotaCreditsResponse resp = quotaResponseBuilder.addQuotaCredits(accountId, domainId, amount, updatedBy, true);
         assertTrue(resp.getCredits().compareTo(credit.getCredit()) == 0);
     }