Copilot commented on code in PR #13955:
URL: https://github.com/apache/cloudstack/pull/13955#discussion_r3971208237
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java:
##########
@@ -191,10 +192,13 @@ public void execute() {
/**
* TODO: this should be done through a validator. for now replicating the
validation logic in create account and user
+ *
+ * <p>A blank 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.
*/
private void validateParams() {
- if(StringUtils.isEmpty(getPassword())) {
- throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
passwords are not allowed");
+ if (StringUtils.isBlank(getPassword())) {
+ password = PasswordGenerator.generateRandomPassword(12);
}
Review Comment:
This changes behavior for callers that explicitly pass an empty/whitespace
password: instead of failing, it silently generates an unknown password, which
can create an account the caller cannot log into (and may mask client bugs).
Consider generating a random password only when the parameter is truly omitted
(null), and keep rejecting empty/blank strings with a validation error.
##########
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 won’t run on initial render, so if `form.samlenable` is
initially true (e.g., defaulted from existing data), password rules may still
be required while the password UI is hidden, potentially blocking form
submission. Make the watcher run on initialization (e.g., set it to `immediate:
true`) or initialize `rules.password` / `rules.confirmpassword` in `created()`
based on the current `form.samlenable` value.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java:
##########
@@ -191,10 +192,13 @@ public void execute() {
/**
* TODO: this should be done through a validator. for now replicating the
validation logic in create account and user
+ *
+ * <p>A blank 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.
*/
private void validateParams() {
- if(StringUtils.isEmpty(getPassword())) {
- throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
passwords are not allowed");
+ if (StringUtils.isBlank(getPassword())) {
+ password = PasswordGenerator.generateRandomPassword(12);
Review Comment:
The password length `12` is a magic number here. Prefer referencing an
existing configured minimum/standard password length (if the project has one)
or extracting this to a named constant so it stays consistent with any password
policy changes.
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/account/CreateAccountCmdTest.java:
##########
@@ -81,28 +80,26 @@ 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());
+ Assert.assertTrue("Received exception as the mock accountService
createUserAccount returns null user", true);
}
- Mockito.verify(accountService,
Mockito.never()).createUserAccount(createAccountCmd);
+ Assert.assertNotNull("a password should be generated for accounts that
authenticate externally", createAccountCmd.getPassword());
+ Mockito.verify(accountService,
Mockito.times(1)).createUserAccount(createAccountCmd);
Review Comment:
The assertion in the catch block is a no-op (`assertTrue(..., true)` always
passes), and the test name implies success-path behavior. If `execute()` is
expected to succeed now, remove the try/catch and set up the mock so
`createUserAccount` does not trigger an exception; then assert the generated
password and verify the service call. If an exception is still expected, the
test should assert on the error code/message and keep an explicit failure when
no exception is thrown.
--
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]
