Copilot commented on code in PR #12294: URL: https://github.com/apache/cloudstack/pull/12294#discussion_r2634248845
########## ui/src/views/iam/ForceChangePassword.vue: ########## @@ -0,0 +1,280 @@ +// 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"> + <div class="success-icon">✓</div> Review Comment: The success icon uses a plain text checkmark "✓" instead of an icon component. This could have accessibility issues for screen readers and might not render consistently across different browsers and operating systems. Consider using an icon component from the ant-design library (like CheckCircleOutlined) for better accessibility and consistent rendering. ```suggestion <a-icon type="check-circle" class="success-icon" /> ``` ########## ui/src/views/iam/ChangeUserPassword.vue: ########## @@ -102,6 +107,10 @@ export default { isAdminOrDomainAdmin () { return ['Admin', 'DomainAdmin'].includes(this.$store.getters.userInfo.roletype) }, + isCallerNotSameAsUser () { + const userId = this.$store.getters.userInfo.id + return userId !== this.resource.id Review Comment: The isEmpty check uses loose comparison with !== which compares the result with the resource.id directly. While JavaScript's type coercion makes this work, it would be clearer to use strict comparison. Also consider using the nullish coalescing operator or optional chaining if the resource might be undefined. ```suggestion const resourceId = this.resource?.id ?? null return userId !== resourceId ``` ########## plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryTest.java: ########## @@ -66,12 +68,17 @@ public class ApiDiscoveryTest { @InjectMocks ApiDiscoveryServiceImpl discoveryServiceSpy; + @Mock + UserAccount mockUserAccount; + @Before public void setup() { discoveryServiceSpy.s_apiNameDiscoveryResponseMap = apiNameDiscoveryResponseMapMock; discoveryServiceSpy._apiAccessCheckers = apiAccessCheckersMock; Mockito.when(discoveryServiceSpy._apiAccessCheckers.iterator()).thenReturn(Arrays.asList(apiCheckerMock).iterator()); + Mockito.when(mockUserAccount.getDetails()).thenReturn(null); + Mockito.when(accountServiceMock.getUserAccountById(anyLong())).thenReturn(mockUserAccount); } Review Comment: The test setup mocks getUserAccountById to return null for user details, but there's no test case that verifies the new password change required functionality. The new logic in ApiDiscoveryServiceImpl that restricts APIs when passwordchangerequired is true is not covered by any test. Consider adding test cases for: 1) user with passwordchangerequired=true should only get limited APIs, and 2) user with passwordchangerequired=false should follow normal flow. ########## server/src/main/java/com/cloud/user/AccountManagerImpl.java: ########## @@ -1580,9 +1582,28 @@ public UserAccount updateUser(UpdateUserCmd updateUserCmd) { user.setUser2faEnabled(true); } _userDao.update(user.getId(), user); + updatePasswordChangeRequired(caller, updateUserCmd, user); return _userAccountDao.findById(user.getId()); } + private void updatePasswordChangeRequired(User caller, UpdateUserCmd updateUserCmd, UserVO user) { + if (StringUtils.isNotBlank(updateUserCmd.getPassword())) { + 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()))) { Review Comment: The condition checks both admin roles separately but uses the same logic for both. The redundant parentheses around the role check can be simplified. Consider refactoring to: if (isRootAdmin(caller.getAccountId()) || isDomainAdmin(caller.getAccountId())) ```suggestion if (isRootAdmin(caller.getAccountId()) || isDomainAdmin(caller.getAccountId())) { ``` ########## 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 checkbox is only displayed when both conditions are met, but there's no validation or error handling if the admin attempts to enforce password change for themselves or for other admin/domain admin users. While the server-side logic in updatePasswordChangeRequired handles this, the UI should ideally reflect this restriction or provide feedback to the user about why the option is not available. ```suggestion </a-form-item> <a-form-item v-if="isAdminOrDomainAdmin() && !isCallerNotSameAsUser()"> <a-alert type="info" :show-icon="true" message="You cannot require a password change on next login for administrator or domain administrator accounts, including your own." /> </a-form-item> ``` ########## ui/src/views/iam/ChangeUserPassword.vue: ########## @@ -134,6 +143,10 @@ export default { if (this.isValidValueForKey(values, 'currentpassword') && values.currentpassword.length > 0) { params.currentpassword = values.currentpassword } + + if (this.isAdminOrDomainAdmin() && values.passwordChangeRequired) { Review Comment: The condition checks if passwordChangeRequired is present in values but doesn't validate whether it's actually a boolean. If values.passwordChangeRequired is undefined, null, or any falsy value, this logic would not add it to params. Consider explicitly checking for true value: if (this.isAdminOrDomainAdmin() && values.passwordChangeRequired === true) ```suggestion if (this.isAdminOrDomainAdmin() && values.passwordChangeRequired === true) { ``` ########## ui/src/views/iam/ForceChangePassword.vue: ########## @@ -0,0 +1,280 @@ +// 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"> + <div class="success-icon">✓</div> + <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="handleLogout" + 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="handleLogout">{{ $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') Review Comment: The userid is retrieved from a cookie which could potentially be manipulated by a malicious user. This creates a security vulnerability where a user could change another user's password by modifying the cookie value. The user ID should be retrieved from the authenticated session on the server side rather than from client-side cookies. ########## plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java: ########## @@ -280,12 +283,22 @@ public ListResponse<? extends BaseResponse> listApis(User user, String name) { ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); } - if (role.getRoleType() == RoleType.Admin && role.getId() == RoleType.Admin.getId()) { - logger.info(String.format("Account [%s] is Root Admin, all APIs are allowed.", - ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); + // Limit APIs on first login requiring password change + UserAccount userAccount = accountService.getUserAccountById(user.getId()); + Map<String, String> userAccDetails = userAccount.getDetails(); + if (MapUtils.isNotEmpty(userAccDetails)) { + String needPwdChangeStr = userAccDetails.get(UserDetailVO.PasswordChangeRequired); + if ("true".equalsIgnoreCase(needPwdChangeStr)) { + apisAllowed = Arrays.asList("login", "logout", "updateUser", "listUsers", "listApis"); + } } else { - for (APIChecker apiChecker : _apiAccessCheckers) { - apisAllowed = apiChecker.getApisAllowedToUser(role, user, apisAllowed); + if (role.getRoleType() == RoleType.Admin && role.getId() == RoleType.Admin.getId()) { + logger.info(String.format("Account [%s] is Root Admin, all APIs are allowed.", + ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid"))); + } else { + for (APIChecker apiChecker : _apiAccessCheckers) { + apisAllowed = apiChecker.getApisAllowedToUser(role, user, apisAllowed); + } } } Review Comment: The nested if-else structure has been inverted in a confusing way. The original logic checked for root admin first, but now it's wrapped in an else block after checking for password change requirement. This makes the code flow less intuitive. Consider restructuring to check password change requirement first with an early return, then handle the normal flow. ########## server/src/main/java/com/cloud/user/AccountManagerImpl.java: ########## @@ -1580,9 +1582,28 @@ public UserAccount updateUser(UpdateUserCmd updateUserCmd) { user.setUser2faEnabled(true); } _userDao.update(user.getId(), user); + updatePasswordChangeRequired(caller, updateUserCmd, user); return _userAccountDao.findById(user.getId()); } + private void updatePasswordChangeRequired(User caller, UpdateUserCmd updateUserCmd, UserVO user) { + if (StringUtils.isNotBlank(updateUserCmd.getPassword())) { + 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); + } + } + + // Remove passwordChangeRequired if user updating own pwd or admin has not enforced it + if (isCallerSameAsUser || !isPasswordResetRequired) { + _userDetailsDao.removeDetail(user.getId(), PasswordChangeRequired); + } + } + } Review Comment: The password change required flag is not validated or enforced at the API level during the updateUser operation. A malicious user could potentially bypass the UI restrictions by directly calling the updateUser API with their current password, even if passwordchangerequired is true, without being forced through the proper flow. Consider adding server-side validation to ensure users with passwordchangerequired=true can only update their password and cannot access other updateUser functionality until the password is changed. ########## server/src/main/java/com/cloud/user/AccountManagerImpl.java: ########## @@ -1580,9 +1582,28 @@ public UserAccount updateUser(UpdateUserCmd updateUserCmd) { user.setUser2faEnabled(true); } _userDao.update(user.getId(), user); + updatePasswordChangeRequired(caller, updateUserCmd, user); return _userAccountDao.findById(user.getId()); } + private void updatePasswordChangeRequired(User caller, UpdateUserCmd updateUserCmd, UserVO user) { + if (StringUtils.isNotBlank(updateUserCmd.getPassword())) { + 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); + } + } + + // Remove passwordChangeRequired if user updating own pwd or admin has not enforced it + if (isCallerSameAsUser || !isPasswordResetRequired) { + _userDetailsDao.removeDetail(user.getId(), PasswordChangeRequired); + } + } + } Review Comment: The new updatePasswordChangeRequired method and its logic for handling passwordchangerequired parameter are not covered by any test cases. Consider adding tests to verify: 1) password change required is set when admin changes user password with the flag, 2) flag is removed when user changes own password, 3) flag is removed when admin changes password without the flag, and 4) admins cannot set the flag for themselves or other admins. ########## ui/src/views/iam/ForceChangePassword.vue: ########## @@ -0,0 +1,280 @@ +// 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"> + <div class="success-icon">✓</div> + <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="handleLogout" + 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="handleLogout">{{ $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.$message.success(this.$t('message.please.login.new.password')) + 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) + }) + }, + async handleLogout () { + try { + await this.$store.dispatch('Logout') + } catch (e) { + console.error('Logout failed:', e) + } finally { + Cookies.remove('userid') + Cookies.remove('token') + this.$localStorage.remove(PASSWORD_CHANGE_REQUIRED) + this.$router.replace({ path: '/user/login' }) + } + }, + async isPasswordChangeRequired () { + try { + this.loading = true + const user = await this.getUserInfo() + if (user && !user.passwordchangerequired) { + this.isSubmitted = true Review Comment: The method isPasswordChangeRequired in the created lifecycle hook redirects to login if the flag is false, but it sets isSubmitted to true before the redirect. This creates a brief visual state change that users might see before the redirect happens. Consider removing the isSubmitted assignment since the redirect will navigate away from the page anyway. ```suggestion ``` ########## api/src/main/java/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java: ########## @@ -85,8 +88,11 @@ public class UpdateUserCmd extends BaseCmd { "This parameter is only used to mandate 2FA, not to disable 2FA", since = "4.18.0.0") private Boolean mandate2FA; - @Inject - private RegionService _regionService; + @Parameter(name = ApiConstants.PASSWORD_CHANGE_REQUIRED, + type = CommandType.BOOLEAN, + description = "Provide true to mandate the User to reset password on next login.", Review Comment: The description says "Provide true to mandate the User to reset password on next login" but technically the user resets their password during login (after authentication), not on the next login. Consider clarifying this to "Provide true to require the User to change their password upon their next login" or "Provide true to force password change at next login". ```suggestion description = "Provide true to require the user to change their password upon their next login.", ``` ########## ui/src/views/iam/ForceChangePassword.vue: ########## @@ -0,0 +1,280 @@ +// 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"> + <div class="success-icon">✓</div> + <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="handleLogout" + style="margin-top: 20px;" Review Comment: The inline style attributes are used instead of CSS classes. Consider moving these inline styles to the scoped style section for better maintainability and consistency with the rest of the component's styling approach. ########## ui/src/store/modules/user.js: ########## @@ -298,7 +315,6 @@ const user = { const latestVersion = vueProps.$localStorage.get(LATEST_CS_VERSION, { version: '', fetchedTs: 0 }) commit('SET_LATEST_VERSION', latestVersion) notification.destroy() Review Comment: A blank line was removed after line 317, which disrupts the consistent spacing pattern in this file. The code has blank lines separating logical blocks, and removing this one makes the code less readable. Consider restoring the blank line for consistency. ```suggestion notification.destroy() ``` ########## ui/src/store/modules/user.js: ########## @@ -323,6 +339,23 @@ const user = { commit('SET_DOMAIN_STORE', domainStore) commit('SET_DARK_MODE', darkMode) commit('SET_LATEST_VERSION', latestVersion) + + // This block is to enforce password change for first time login after admin resets password + const isPwdChangeRequired = vueProps.$localStorage.get(PASSWORD_CHANGE_REQUIRED) + commit('SET_PASSWORD_CHANGE_REQUIRED', isPwdChangeRequired) + if (isPwdChangeRequired) { + getAPI('listUsers', { id: Cookies.get('userid') }).then(response => { + const result = response.listusersresponse.user[0] + commit('SET_INFO', result) + commit('SET_NAME', result.firstname + ' ' + result.lastname) + if (result.icon?.base64image) commit('SET_AVATAR', result.icon.base64image) + resolve({}) + }).catch(error => { + reject(error) + }) Review Comment: The API call to listUsers uses id from cookies without validation. If the cookie is tampered with, this could potentially expose information about other users. While the backend should handle authorization, it's better to validate the user ID matches the authenticated session before making the request. ########## ui/src/store/modules/user.js: ########## @@ -323,6 +339,23 @@ const user = { commit('SET_DOMAIN_STORE', domainStore) commit('SET_DARK_MODE', darkMode) commit('SET_LATEST_VERSION', latestVersion) + + // This block is to enforce password change for first time login after admin resets password + const isPwdChangeRequired = vueProps.$localStorage.get(PASSWORD_CHANGE_REQUIRED) + commit('SET_PASSWORD_CHANGE_REQUIRED', isPwdChangeRequired) + if (isPwdChangeRequired) { + getAPI('listUsers', { id: Cookies.get('userid') }).then(response => { + const result = response.listusersresponse.user[0] + commit('SET_INFO', result) + commit('SET_NAME', result.firstname + ' ' + result.lastname) + if (result.icon?.base64image) commit('SET_AVATAR', result.icon.base64image) + resolve({}) + }).catch(error => { + reject(error) + }) + return + } Review Comment: The localStorage operations directly access PASSWORD_CHANGE_REQUIRED without consistent error handling. If localStorage is not available (e.g., in private browsing mode or due to browser settings), these operations could throw exceptions. Consider wrapping localStorage operations in try-catch blocks or using a safer wrapper function. -- 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]
