Copilot commented on code in PR #13955:
URL: https://github.com/apache/cloudstack/pull/13955#discussion_r4512722975
##########
ui/src/views/iam/AddAccount.vue:
##########
@@ -53,7 +53,7 @@
v-model:value="form.username"
:placeholder="apiParams.username.description" />
</a-form-item>
- <a-row :gutter="12">
+ <a-row :gutter="12" v-if="!form.samlenable">
Review Comment:
The password fields are hidden when `form.samlenable` is initially true, but
this watcher won’t run on initial render, so the form can still retain required
password validation and prevent submission. Make the watcher run for the
initial value (e.g., `immediate: true`) or initialize
`rules.password/confirmpassword` from the initial `form.samlenable` state
during component setup.
##########
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:
The password fields are hidden when `form.samlenable` is initially true, but
this watcher won’t run on initial render, so the form can still retain required
password validation and prevent submission. Make the watcher run for the
initial value (e.g., `immediate: true`) or initialize
`rules.password/confirmpassword` from the initial `form.samlenable` state
during component setup.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java:
##########
@@ -75,8 +77,8 @@ public class CreateAccountCmd extends BaseCmd {
@Parameter(name = ApiConstants.PASSWORD,
type = CommandType.STRING,
- required = true,
- description = "Clear text password (Default hashed to
SHA256SALT). If you wish to use any other hashing algorithm, you would need to
write a custom authentication adapter See Docs section.")
+ description = "Clear text password (Default hashed to
SHA256SALT). If you wish to use any other hashing algorithm, you will need to
write a custom authentication adapter See Docs section. "
+ + "If omitted, a random password is generated, e.g. for
an account that will only ever authenticate externally via SAML/LDAP.")
Review Comment:
Generating a random password whenever the client omits `password` changes
the API semantics for all callers (including accidental omissions), potentially
creating accounts whose password is unknown to the admin. Consider gating
auto-generation behind an explicit signal (e.g., a `generatepassword` boolean)
or a known external-auth path, so client bugs still fail fast. Also, the
parameter description contains a trailing space after the period (\"... Docs
section. \"), which should be removed to avoid propagating formatting issues
into generated API docs.
##########
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:
Generating a random password whenever the client omits `password` changes
the API semantics for all callers (including accidental omissions), potentially
creating accounts whose password is unknown to the admin. Consider gating
auto-generation behind an explicit signal (e.g., a `generatepassword` boolean)
or a known external-auth path, so client bugs still fail fast. Also, the
parameter description contains a trailing space after the period (\"... Docs
section. \"), which should be removed to avoid propagating formatting issues
into generated API docs.
##########
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);
+ } else if (StringUtils.isBlank(getPassword())) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Blank
passwords are not allowed");
}
Review Comment:
Generating a random password whenever the client omits `password` changes
the API semantics for all callers (including accidental omissions), potentially
creating accounts whose password is unknown to the admin. Consider gating
auto-generation behind an explicit signal (e.g., a `generatepassword` boolean)
or a known external-auth path, so client bugs still fail fast. Also, the
parameter description contains a trailing space after the period (\"... Docs
section. \"), which should be removed to avoid propagating formatting issues
into generated API docs.
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmdTest.java:
##########
@@ -81,28 +81,30 @@ 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);
+ // the mock accountService createUserAccount returns null, so execute
fails after validation
+ ServerApiException e = Assert.assertThrows(ServerApiException.class,
() -> createAccountCmd.execute());
+ Assert.assertEquals(ApiErrorCode.INTERNAL_ERROR, e.getErrorCode());
+ Assert.assertEquals("a password should be generated for accounts that
authenticate externally",
+ 12, createAccountCmd.getPassword().length());
+ Mockito.verify(accountService,
Mockito.times(1)).createUserAccount(createAccountCmd);
}
Review Comment:
This test validates password generation, but it asserts an `INTERNAL_ERROR`
that occurs only because the mocked `accountService.createUserAccount` returns
null. That couples the test to an unrelated failure mode and makes it brittle.
Prefer stubbing `createUserAccount` to return a non-null `UserAccount` so
`execute()` can complete normally, then assert the generated password
properties and the service interaction without depending on an internal-error
path.
--
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]
