Copilot commented on code in PR #12294:
URL: https://github.com/apache/cloudstack/pull/12294#discussion_r2667289180
##########
api/src/test/java/org/apache/cloudstack/api/command/admin/user/CreateUserCmdTest.java:
##########
@@ -95,6 +95,6 @@ public void testExecuteWithEmptyPassword() {
Assert.assertEquals(ApiErrorCode.PARAM_ERROR,e.getErrorCode());
Assert.assertEquals("Empty passwords are not allowed",
e.getMessage());
}
- Mockito.verify(accountService, Mockito.never()).createUser(null, null,
null, null, null, null, null, null, null);
+ Mockito.verify(accountService, Mockito.never()).createUser(null, null,
null, null, null, null, null, null, null, true);
Review Comment:
The test expects 'false' for the password change required parameter on line
85, but line 98 expects 'true'. This inconsistency appears incorrect - when
testing empty password validation, the passwordChangeRequired parameter value
should not matter, and should likely be 'false' for consistency with the other
tests.
```suggestion
Mockito.verify(accountService, Mockito.never()).createUser(null,
null, null, null, null, null, null, null, null, false);
```
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1544,14 +1558,21 @@ public UserVO createUser(String userName, String
password, String firstName, Str
verifyCallerPrivilegeForUserOrAccountOperations(account);
UserVO user;
user = createUser(account.getId(), userName, password, firstName,
lastName, email, timeZone, userUUID, source);
+ if (isPasswordChangeRequired) {
+ long callerAccountId = CallContext.current().getCallingAccountId();
+ if ((isRootAdmin(callerAccountId) ||
isDomainAdmin(callerAccountId))) {
Review Comment:
According to the PR description, password change enforcement should only be
applicable to normal user roles, excluding admin and domain admin. However,
this code doesn't validate the target account's role type before allowing the
password change flag to be set. Consider adding a check to prevent enforcing
password changes on Admin or DomainAdmin accounts.
```suggestion
if (account.getType() == Account.Type.ADMIN || account.getType()
== Account.Type.DOMAIN_ADMIN) {
logger.warn("Enforcing password change is not permitted for
account type [{}].", account.getType());
throw new InvalidParameterValueException("CloudStack does
not support enforcing password change for Admin or DomainAdmin users.");
}
long callerAccountId =
CallContext.current().getCallingAccountId();
if (isRootAdmin(callerAccountId) ||
isDomainAdmin(callerAccountId)) {
```
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -1585,10 +1606,36 @@ public UserAccount updateUser(UpdateUserCmd
updateUserCmd) {
if (mandate2FA != null && mandate2FA) {
user.setUser2faEnabled(true);
}
+ updatePasswordChangeRequired(caller, updateUserCmd, user);
_userDao.update(user.getId(), user);
return _userAccountDao.findById(user.getId());
}
+ private void updatePasswordChangeRequired(User caller, UpdateUserCmd
updateUserCmd, UserVO user) {
+ User.Source userSource = user.getSource();
+ if ((userSource == User.Source.SAML2 || userSource ==
User.Source.SAML2DISABLED || userSource == User.Source.LDAP)
+ && updateUserCmd.isPasswordChangeRequired()) {
+ logger.warn("Enforcing password change is not permitted for source
[{}].", user.getSource());
+ throw new InvalidParameterValueException("CloudStack does not
support enforcing password change for SAML or LDAP users.");
+ }
+
+ boolean isCallerSameAsUser = user.getId() == caller.getId();
+ boolean isPasswordResetRequired =
updateUserCmd.isPasswordChangeRequired() && !isCallerSameAsUser;
+ // Admins only can enforce passwordChangeRequired for user
+ if (isRootAdmin(caller.getAccountId()) ||
isDomainAdmin(caller.getAccountId())) {
+ if (isPasswordResetRequired) {
+ _userDetailsDao.addDetail(user.getId(),
PasswordChangeRequired, "true", false);
+ }
+ }
Review Comment:
According to the PR description, password change enforcement should only be
applicable to normal user roles, excluding admin and domain admin. However,
this method doesn't validate the target user's role type before allowing the
password change flag to be set. Consider adding a check to prevent enforcing
password changes on Admin or DomainAdmin users, similar to the SAML/LDAP
validation above.
##########
plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java:
##########
@@ -66,6 +69,7 @@ public class ApiDiscoveryServiceImpl extends
ComponentLifecycleBase implements A
List<APIChecker> _apiAccessCheckers = null;
List<PluggableService> _services = null;
protected static Map<String, ApiDiscoveryResponse>
s_apiNameDiscoveryResponseMap = null;
+ public static final List<String> APIS_ALLOWED_FOR_PASSWORD_CHANGE =
Arrays.asList("login", "logout", "updateUser", "listApis");
Review Comment:
The PR description states that the ListApis response should contain 5 APIs
including "listUsers": ["login", "logout", "updateUser", "listUsers",
"listApis"]. However, this constant only includes 4 APIs and is missing
"listUsers". Either the constant needs to be updated to include "listUsers", or
the PR description needs to be corrected.
```suggestion
public static final List<String> APIS_ALLOWED_FOR_PASSWORD_CHANGE =
Arrays.asList("login", "logout", "updateUser", "listUsers", "listApis");
```
##########
ui/src/views/iam/ChangeUserPassword.vue:
##########
@@ -49,6 +49,11 @@
v-model:value="form.confirmpassword"
:placeholder="$t('label.confirmpassword.description')"/>
</a-form-item>
+ <a-form-item v-if="isAdminOrDomainAdmin() && isCallerNotSameAsUser()"
name="passwordChangeRequired" ref="passwordChangeRequired">
+ <a-checkbox v-model:checked="form.passwordChangeRequired">
+ {{ $t('label.change.password.onlogin') }}
+ </a-checkbox>
+ </a-form-item>
Review Comment:
The condition should also verify that the target user has a normal user role
(resource.accounttype === 0) before displaying this checkbox. According to the
PR description, password change enforcement is only applicable to normal users,
excluding admin and domain admin.
##########
ui/src/config/section/user.js:
##########
@@ -82,6 +82,23 @@ export default {
popup: true,
component: shallowRef(defineAsyncComponent(() =>
import('@/views/iam/EditUser.vue')))
},
+ {
+ api: 'updateUser',
+ icon: 'redo-outlined',
+ label: 'label.change.password.enforce',
+ dataView: true,
+ args: ['passwordchangerequired'],
+ mapping: {
+ passwordchangerequired: {
+ value: (record) => { return true }
+ }
+ },
+ popup: true,
+ show: (record, store) => {
+ return ['Admin', 'DomainAdmin'].includes(store.userInfo.roletype) &&
+ !record.isdefault && (store.userInfo.id !== record.id)
Review Comment:
The show condition should also verify that the target user has a normal user
role (accounttype === 0) before displaying this action. According to the PR
description, password change enforcement is only applicable to normal users,
excluding admin and domain admin. Consider adding a check like:
record.accounttype === 0
```suggestion
!record.isdefault &&
(store.userInfo.id !== record.id) &&
record.accounttype === 0
```
##########
ui/src/views/iam/ForceChangePassword.vue:
##########
@@ -0,0 +1,269 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+<template>
+ <div class="user-layout-wrapper">
+ <div class="container">
+ <div class="user-layout-content">
+ <a-card :bordered="false" class="force-password-card">
+ <template #title>
+ <div style="text-align: center; font-size: 18px; font-weight:
bold;">
+ {{ $t('label.action.change.password') }}
+ </div>
+ <div v-if="!isSubmitted" style="text-align: center; font-size:
14px; color: #666; margin-top: 5px;">
+ {{ $t('message.change.password.required') }}
+ </div>
+ </template>
+ <a-spin :spinning="loading">
+ <div v-if="isSubmitted" class="success-state">
+ <check-outlined class="success-icon" />
+ <div class="success-text">
+ {{ $t('message.success.change.password') }}
+ </div>
+ <div class="success-subtext">
+ {{ $t('message.please.login.new.password') }}
+ </div>
+ <a-button
+ type="primary"
+ size="large"
+ block
+ @click="redirectToLogin()"
+ style="margin-top: 20px;"
+ >
+ {{ $t('label.login') }}
+ </a-button>
+ </div>
+
+ <a-form
+ v-else
+ :ref="formRef"
+ :model="form"
+ :rules="rules"
+ layout="vertical"
+ @finish="handleSubmit"
+ v-ctrl-enter="handleSubmit"
+ >
+ <a-form-item name="currentpassword"
:label="$t('label.currentpassword')">
+ <a-input-password
+ v-model:value="form.currentpassword"
+ :placeholder="$t('label.currentpassword')"
+ size="large"
+ v-focus="true"
+ />
+ </a-form-item>
+
+ <a-form-item name="password" :label="$t('label.new.password')">
+ <a-input-password
+ v-model:value="form.password"
+ :placeholder="$t('label.new.password')"
+ size="large"
+ />
+ </a-form-item>
+
+ <a-form-item name="confirmpassword"
:label="$t('label.confirmpassword')">
+ <a-input-password
+ v-model:value="form.confirmpassword"
+ :placeholder="$t('label.confirmpassword')"
+ size="large"
+ />
+ </a-form-item>
+
+ <a-form-item>
+ <a-button
+ type="primary"
+ size="large"
+ block
+ :loading="loading"
+ @click="handleSubmit"
+ >
+ {{ $t('label.ok') }}
+ </a-button>
+ </a-form-item>
+
+ <div class="actions">
+ <a @click="logoutAndRedirectToLogin()">{{ $t('label.logout')
}}</a>
+ </div>
+ </a-form>
+ </a-spin>
+ </a-card>
+ </div>
+ </div>
+ </div>
+</template>
+
+<script>
+import { ref, reactive, toRaw } from 'vue'
+import { postAPI } from '@/api'
+import Cookies from 'js-cookie'
+import { PASSWORD_CHANGE_REQUIRED } from '@/store/mutation-types'
+
+export default {
+ name: 'ForceChangePassword',
+ data () {
+ return {
+ loading: false,
+ isSubmitted: false
+ }
+ },
+ created () {
+ this.formRef = ref()
+ this.form = reactive({})
+ this.isPasswordChangeRequired()
+ },
+ computed: {
+ rules () {
+ return {
+ currentpassword: [{ required: true, message:
this.$t('message.error.current.password') }],
+ password: [{ required: true, message:
this.$t('message.error.new.password') }],
+ confirmpassword: [
+ { required: true, message: this.$t('message.error.confirm.password')
},
+ { validator: this.validateTwoPassword, trigger: 'change' }
+ ]
+ }
+ }
+ },
+ methods: {
+ async validateTwoPassword (rule, value) {
+ if (!value || value.length === 0) {
+ return Promise.resolve()
+ } else if (rule.field === 'confirmpassword') {
+ const form = this.form
+ const messageConfirm = this.$t('message.validate.equalto')
+ const passwordVal = form.password
+ if (passwordVal && passwordVal !== value) {
+ return Promise.reject(messageConfirm)
+ } else {
+ return Promise.resolve()
+ }
+ } else {
+ return Promise.resolve()
+ }
+ },
+ handleSubmit (e) {
+ e.preventDefault()
+ if (this.loading) return
+ this.formRef.value.validate().then(() => {
+ this.loading = true
+ const values = toRaw(this.form)
+ const userId = Cookies.get('userid')
+
+ const params = {
+ id: userId,
+ password: values.password,
+ currentpassword: values.currentpassword
+ }
+ postAPI('updateUser', params).then(() => {
+ this.$localStorage.remove(PASSWORD_CHANGE_REQUIRED)
+ this.handleLogout()
+ this.isSubmitted = true
+ }).catch(error => {
+ console.error(error)
+ this.$message.error(this.$t('message.error.change.password'))
+ }).finally(() => {
+ this.loading = false
+ })
+ }).catch(error => {
+ console.log('Validation failed:', error)
Review Comment:
Remove these console logging statements before merging to production. Debug
logging should not be left in production code.
```suggestion
}).catch(() => {
// Validation failed; errors are displayed via form validation rules.
```
##########
ui/src/views/iam/ForceChangePassword.vue:
##########
@@ -0,0 +1,269 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+<template>
+ <div class="user-layout-wrapper">
+ <div class="container">
+ <div class="user-layout-content">
+ <a-card :bordered="false" class="force-password-card">
+ <template #title>
+ <div style="text-align: center; font-size: 18px; font-weight:
bold;">
+ {{ $t('label.action.change.password') }}
+ </div>
+ <div v-if="!isSubmitted" style="text-align: center; font-size:
14px; color: #666; margin-top: 5px;">
+ {{ $t('message.change.password.required') }}
+ </div>
+ </template>
+ <a-spin :spinning="loading">
+ <div v-if="isSubmitted" class="success-state">
+ <check-outlined class="success-icon" />
+ <div class="success-text">
+ {{ $t('message.success.change.password') }}
+ </div>
+ <div class="success-subtext">
+ {{ $t('message.please.login.new.password') }}
+ </div>
+ <a-button
+ type="primary"
+ size="large"
+ block
+ @click="redirectToLogin()"
+ style="margin-top: 20px;"
+ >
+ {{ $t('label.login') }}
+ </a-button>
+ </div>
+
+ <a-form
+ v-else
+ :ref="formRef"
+ :model="form"
+ :rules="rules"
+ layout="vertical"
+ @finish="handleSubmit"
+ v-ctrl-enter="handleSubmit"
+ >
+ <a-form-item name="currentpassword"
:label="$t('label.currentpassword')">
+ <a-input-password
+ v-model:value="form.currentpassword"
+ :placeholder="$t('label.currentpassword')"
+ size="large"
+ v-focus="true"
+ />
+ </a-form-item>
+
+ <a-form-item name="password" :label="$t('label.new.password')">
+ <a-input-password
+ v-model:value="form.password"
+ :placeholder="$t('label.new.password')"
+ size="large"
+ />
+ </a-form-item>
+
+ <a-form-item name="confirmpassword"
:label="$t('label.confirmpassword')">
+ <a-input-password
+ v-model:value="form.confirmpassword"
+ :placeholder="$t('label.confirmpassword')"
+ size="large"
+ />
+ </a-form-item>
+
+ <a-form-item>
+ <a-button
+ type="primary"
+ size="large"
+ block
+ :loading="loading"
+ @click="handleSubmit"
+ >
+ {{ $t('label.ok') }}
+ </a-button>
+ </a-form-item>
+
+ <div class="actions">
+ <a @click="logoutAndRedirectToLogin()">{{ $t('label.logout')
}}</a>
+ </div>
+ </a-form>
+ </a-spin>
+ </a-card>
+ </div>
+ </div>
+ </div>
+</template>
+
+<script>
+import { ref, reactive, toRaw } from 'vue'
+import { postAPI } from '@/api'
+import Cookies from 'js-cookie'
+import { PASSWORD_CHANGE_REQUIRED } from '@/store/mutation-types'
+
+export default {
+ name: 'ForceChangePassword',
+ data () {
+ return {
+ loading: false,
+ isSubmitted: false
+ }
+ },
+ created () {
+ this.formRef = ref()
+ this.form = reactive({})
+ this.isPasswordChangeRequired()
+ },
+ computed: {
+ rules () {
+ return {
+ currentpassword: [{ required: true, message:
this.$t('message.error.current.password') }],
+ password: [{ required: true, message:
this.$t('message.error.new.password') }],
+ confirmpassword: [
+ { required: true, message: this.$t('message.error.confirm.password')
},
+ { validator: this.validateTwoPassword, trigger: 'change' }
+ ]
+ }
+ }
+ },
+ methods: {
+ async validateTwoPassword (rule, value) {
+ if (!value || value.length === 0) {
+ return Promise.resolve()
+ } else if (rule.field === 'confirmpassword') {
+ const form = this.form
+ const messageConfirm = this.$t('message.validate.equalto')
+ const passwordVal = form.password
+ if (passwordVal && passwordVal !== value) {
+ return Promise.reject(messageConfirm)
+ } else {
+ return Promise.resolve()
+ }
+ } else {
+ return Promise.resolve()
+ }
+ },
+ handleSubmit (e) {
+ e.preventDefault()
+ if (this.loading) return
+ this.formRef.value.validate().then(() => {
+ this.loading = true
+ const values = toRaw(this.form)
+ const userId = Cookies.get('userid')
+
+ const params = {
+ id: userId,
+ password: values.password,
+ currentpassword: values.currentpassword
+ }
+ postAPI('updateUser', params).then(() => {
+ this.$localStorage.remove(PASSWORD_CHANGE_REQUIRED)
+ this.handleLogout()
Review Comment:
The handleLogout call should be awaited before setting isSubmitted to true.
Without await, isSubmitted may be set before the logout completes, potentially
causing the success state to display while the user is still logged in.
```suggestion
postAPI('updateUser', params).then(async () => {
this.$localStorage.remove(PASSWORD_CHANGE_REQUIRED)
await this.handleLogout()
```
##########
ui/src/views/iam/ForceChangePassword.vue:
##########
@@ -0,0 +1,269 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+<template>
+ <div class="user-layout-wrapper">
+ <div class="container">
+ <div class="user-layout-content">
+ <a-card :bordered="false" class="force-password-card">
+ <template #title>
+ <div style="text-align: center; font-size: 18px; font-weight:
bold;">
+ {{ $t('label.action.change.password') }}
+ </div>
+ <div v-if="!isSubmitted" style="text-align: center; font-size:
14px; color: #666; margin-top: 5px;">
+ {{ $t('message.change.password.required') }}
+ </div>
+ </template>
+ <a-spin :spinning="loading">
+ <div v-if="isSubmitted" class="success-state">
+ <check-outlined class="success-icon" />
Review Comment:
The component 'check-outlined' is used but not imported. You need to import
'CheckOutlined' from '@ant-design/icons-vue' in the script section for this
component to work properly.
##########
ui/src/views/iam/AddUser.vue:
##########
@@ -384,6 +389,10 @@ export default {
if (this.isValidValueForKey(rawParams, 'timezone') &&
rawParams.timezone.length > 0) {
params.timezone = rawParams.timezone
}
+ console.log('rawParams.passwordChangeRequired',
rawParams.passwordChangeRequired)
Review Comment:
Remove this console.log statement before merging to production. Debug
logging should not be left in production code.
```suggestion
```
--
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]
