Copilot commented on code in PR #13955:
URL: https://github.com/apache/cloudstack/pull/13955#discussion_r4524012719
##########
ui/src/views/iam/AddAccount.vue:
##########
@@ -250,6 +256,14 @@ export default {
}
},
immediate: false
+ },
+ 'form.samlenable' (samlEnabled) {
+ // a SAML-authenticated account never logs in with a native password
+ this.rules.password = samlEnabled ? [] : [{ required: true, message:
this.$t('message.error.required.input') }]
+ this.rules.confirmpassword = samlEnabled ? [] : [
+ { required: true, message: this.$t('message.error.required.input') },
+ { validator: this.validateConfirmPassword }
+ ]
Review Comment:
This watcher only runs when `form.samlenable` changes; if the form loads
with `samlenable === true` initially, the password fields are hidden but the
original required validation rules may still be active, potentially blocking
submission. Make the watcher `immediate: true` or initialize
`rules.password`/`rules.confirmpassword` during component initialization based
on the initial `form.samlenable` value.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java:
##########
@@ -191,10 +193,16 @@ public void execute() {
/**
* TODO: this should be done through a validator. for now replicating the
validation logic in create account and user
+ *
+ * <p>An omitted password generates a random one instead of failing, since
an account that will
+ * only ever authenticate externally (SAML/LDAP) has no need for the admin
to set one. A password
+ * that is passed but blank is still rejected, as that is more likely a
client error.
*/
private void validateParams() {
- if(StringUtils.isEmpty(getPassword())) {
- throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
passwords are not allowed");
+ if (getPassword() == null) {
+ password =
PasswordGenerator.generateRandomPassword(GENERATED_PASSWORD_LENGTH);
Review Comment:
The generated password length is a hard-coded magic number (`12`). To keep
behavior consistent with any system-wide password policy and avoid future
drift, consider sourcing this from an existing password policy/configuration
(or a shared constant) rather than embedding a fixed length in the command.
##########
ui/src/views/iam/AddAccount.vue:
##########
@@ -408,12 +422,16 @@ export default {
const params = {
roleid: values.roleid,
username: values.username,
- password: values.password,
email: values.email,
firstname: values.firstname,
lastname: values.lastname,
domainid: values.domainid
}
+ if (!values.samlenable) {
+ // SAML-authenticated accounts never log in with a native password;
let the API
+ // generate one rather than asking the admin to set one that will
never be used
Review Comment:
The decision to omit `params.password` depends on `values.samlenable`, but
the UI visibility/validation logic is driven by `form.samlenable`. If
`samlenable` is not part of the submitted `values` (or is undefined), this
condition will default to true and may send an undefined/empty `password`. Use
the same source of truth (e.g., `this.form.samlenable`) for building params, or
ensure `samlenable` is always included in `values`.
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmdTest.java:
##########
@@ -81,28 +87,33 @@ public void testExecuteWithNotBlankPassword() {
}
@Test
- public void testExecuteWithNullPassword() {
+ public void testExecuteWithNullPasswordGeneratesOne() {
ReflectionTestUtils.setField(createAccountCmd, "password", null);
- try {
- createAccountCmd.execute();
- Assert.fail("should throw exception for a null password");
- } catch (ServerApiException e) {
- Assert.assertEquals(ApiErrorCode.PARAM_ERROR, e.getErrorCode());
- Assert.assertEquals("Empty passwords are not allowed",
e.getMessage());
- }
- Mockito.verify(accountService,
Mockito.never()).createUserAccount(createAccountCmd);
+ UserAccount userAccount = Mockito.mock(UserAccount.class);
+
Mockito.when(accountService.createUserAccount(createAccountCmd)).thenReturn(userAccount);
+
Mockito.when(responseGenerator.createUserAccountResponse(ResponseView.Full,
userAccount)).thenReturn(Mockito.mock(AccountResponse.class));
+
+ createAccountCmd.execute();
+
+ Assert.assertEquals("a password should be generated for accounts that
authenticate externally",
+ 12, createAccountCmd.getPassword().length());
Review Comment:
The test duplicates the generated password length (`12`), which can become
brittle if the production constant changes. Prefer asserting against a shared
value (e.g., expose the length via a non-private constant/getter for tests, or
centralize the expected length in one place) to avoid accidental divergence.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java:
##########
@@ -37,12 +37,14 @@
import com.cloud.user.Account;
import com.cloud.user.UserAccount;
+import com.cloud.utils.PasswordGenerator;
@APICommand(name = "createAccount", description = "Creates an account",
responseObject = AccountResponse.class, entityType = {Account.class},
requestHasSensitiveInfo = true, responseHasSensitiveInfo = true)
public class CreateAccountCmd extends BaseCmd {
+ private static final int GENERATED_PASSWORD_LENGTH = 12;
Review Comment:
The generated password length is a hard-coded magic number (`12`). To keep
behavior consistent with any system-wide password policy and avoid future
drift, consider sourcing this from an existing password policy/configuration
(or a shared constant) rather than embedding a fixed length in the command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
