[NAE-2173] Password change #345
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Service as UserServiceImpl
participant User as AbstractUser
participant Encoder as PasswordEncoder
participant Repo as UserRepository
Caller->>Service: changePassword(user, newPassword, oldPassword)
activate Service
Service->>Service: canUpdatePassword(user, newPassword)
Service->>Service: verifyPasswords(user, oldPassword)
alt verification succeeds
Service->>Encoder: encode(newPassword)
Encoder-->>Service: encodedNew
Service->>User: setPassword(encodedNew)
Service->>Repo: saveUser(user)
Repo-->>Service: persistedUser
Service-->>Caller: persistedUser
else verification fails
Service-->>Caller: throw IllegalArgumentException / RuntimeException
end
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
84-85: Redundant reinitialization of credentials in constructors.The field is already initialized at declaration; reassigning is unnecessary.
- this.credentials = new HashMap<>(); + // credentials already initialized at field declarationAlso applies to: 107-108
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
652-660: Tighten validation and exceptions; avoid vague message.Use IllegalStateException, check blank, and improve wording. Consider extracting "password" credential type into a constant.
- if (!user.isCredentialEnabled("password")) { - throw new RuntimeException("Password does not exists or authorization is not enabled"); - } - - if (password == null) { - throw new IllegalArgumentException("Password is not set"); - } + if (!user.isCredentialEnabled("password")) { + throw new IllegalStateException("Password credential does not exist or is not enabled."); + } + if (password == null || password.isBlank()) { + throw new IllegalArgumentException("New password must be provided."); + }Optional: introduce
private static final String PASSWORD_CREDENTIAL = "password";and reuse it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java(0 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(2 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(3 hunks)
💤 Files with no reviewable changes (1)
- nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java
🧰 Additional context used
🧬 Code graph analysis (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
Data(19-229)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (1)
Data(24-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java (1)
402-410: AlignchangePasswordsignature and docs
- Swapped parameters to the conventional order
(user, oldPassword, newPassword)- Expanded Javadoc to clarify that this verifies the current password and throws on invalid input
- Updated in
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java:402–410nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java:264- No direct invocations were found in the checked modules; please search across the repo (e.g. in
AuthenticationController) forchangePassword(and update all call sites to the new parameter order--- a/nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java @@ -402,9 +402,17 @@ public interface UserService { - /** - * Resets password for user. - * - * @param user user - * @param newPassword new password - * @param oldPassword old password - * @return the updated user - */ - AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword); + /** + * Changes the user's password after verifying the current (old) password. + * + * @param user user + * @param oldPassword current password (required) + * @param newPassword new password (required) + * @return the updated user + * @throws IllegalArgumentException if oldPassword does not match or any required argument is null/blank + * @throws IllegalStateException if password credential is missing or disabled + */ + AbstractUser changePassword(AbstractUser user, String oldPassword, String newPassword);--- a/nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java @@ -262,7 +262,7 @@ public class UserServiceImpl implements UserService { - @Override - public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) { + @Override + public AbstractUser changePassword(AbstractUser user, String oldPassword, String newPassword) { canUpdatePassword(user, newPassword); // ... }
...t-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java
Outdated
Show resolved
Hide resolved
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Show resolved
Hide resolved
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Show resolved
Hide resolved
…'t match, null checks, password verification
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (1)
57-60: Nice defensive copy; use a constant for the key to avoid typos and enable reuse.The copy avoids mutating the domain map—good resolution of the earlier concern. Replace the literal with a constant.
- attributes.put("enabledCredentials", enabledCredentialsAttribute); + attributes.put(ATTR_ENABLED_CREDENTIALS, enabledCredentialsAttribute);Add once (outside this hunk):
public static final String ATTR_ENABLED_CREDENTIALS = "enabledCredentials";
🧹 Nitpick comments (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (1)
39-48: Make enabledCredentials computed, non-required (and optionally read-only); consider deterministic order.
- Setting
required=trueon a server-computed attribute can force clients/forms to send it back and may break updates. Prefer not required (and, if supported byAttribute, mark it read-only).- Optional: collect to a deterministic set (e.g., TreeSet) for stable JSON output and easier testing.
Apply:
- enabledCredentialsAttribute.setRequired(true); + // Computed on server; do not require it on client payloads + enabledCredentialsAttribute.setRequired(false)Optional deterministic ordering:
- .collect(Collectors.toSet())); + .collect(java.util.stream.Collectors.toCollection(java.util.TreeSet::new)));Also, please confirm exposing the list of enabled credential types in this DTO is acceptable for all endpoints that serialize it (particularly non-admin contexts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(2 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (1)
7-7: LGTM: necessary imports added.
CredentialandCollectorsare both used below; import scope is minimal and correct.Also applies to: 14-14
- Removed credentials from `AbstractUser` and added it to `User` for better specificity. - Enhanced JavaDoc comments in `User` and `AbstractUser` for clearer documentation. - Fixed potential null pointer scenarios in credential handling within `User`.
- Change credentials visibility in `User` to private
tuplle
previously approved these changes
Sep 5, 2025
...t-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (12)
81-84: Null-guard credentials in getToken().- Credential<?> tokenCredential = this.credentials.get("token"); + Credential<?> tokenCredential = ensureCredentials().get("token");
91-94: Null-guard credentials in setToken().- this.credentials.put(tokenCredential.getType(), tokenCredential); + ensureCredentials().put(tokenCredential.getType(), tokenCredential);
102-108: Null-guard credentials in getPassword().- Credential<?> passCred = this.credentials.get("password"); + Credential<?> passCred = ensureCredentials().get("password");
116-119: Null-guard credentials in setPassword().- this.credentials.put("password", passwordCredential); + ensureCredentials().put("password", passwordCredential);
126-135: Null-guard credentials in setExpirationDate().- Credential<?> tokenCredential = this.credentials.get("token"); + Credential<?> tokenCredential = ensureCredentials().get("token"); @@ - this.credentials.put("token", newTokenCredential); + ensureCredentials().put("token", newTokenCredential);
142-152: Handle null/unknown types in getExpirationDate().Avoid ClassCastException/NPE when property is absent or not Date/LocalDateTime.
- Credential<?> tokenCredential = this.credentials.get("token"); + Credential<?> tokenCredential = ensureCredentials().get("token"); if (tokenCredential != null) { Object obj = tokenCredential.getProperty("expirationDate"); - if (obj instanceof LocalDateTime) { + if (obj instanceof LocalDateTime) { return (LocalDateTime) obj; } - return DateUtils.dateToLocalDateTime((Date) obj); + if (obj instanceof Date) { + return DateUtils.dateToLocalDateTime((Date) obj); + } + return null; } return null;
171-182: Bug: MFA filter checks credential.type, but you store MFA under key "MFA-".Current predicate likely returns empty set (e.g., type "EMAIL" doesn’t start with "MFA"). Filter by entry key instead.
- return this.credentials.entrySet().stream() + return ensureCredentials().entrySet().stream() .filter(entry -> { Credential<?> credential = entry.getValue(); - return credential != null - && credential.getType() != null - && credential.getType().toUpperCase().startsWith("MFA") - && credential.isEnabled(); + String key = entry.getKey(); + return credential != null + && key != null + && key.toUpperCase().startsWith("MFA") + && credential.isEnabled(); }) .map(Map.Entry::getKey) .collect(Collectors.toSet());
190-194: Null-guard credentials in disableCredential().- if (this.credentials.containsKey(type)) { - this.credentials.get(type).setEnabled(false); + if (ensureCredentials().containsKey(type)) { + ensureCredentials().get(type).setEnabled(false); }
235-247: Null-guard and reduce log noise in setCredentialProperty().- Credential<?> credential = this.credentials.get(type); + Credential<?> credential = ensureCredentials().get(type);Optional: early-return if value not Serializable without warning spam in hot paths; consider debug level.
257-260: Null-guard credentials in getCredentialProperty().- Credential<?> credential = this.credentials.get(type); + Credential<?> credential = ensureCredentials().get(type);
269-270: Null-guard credentials in hasCredential().- return this.credentials.containsKey(type); + return ensureCredentials().containsKey(type);
279-281: Null-guard credentials in getCredential().- return this.credentials.get(type); + return ensureCredentials().get(type);
♻️ Duplicate comments (1)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
159-161: Avoid silent no-ops/nulls in credential helpers; make abstract or implement sensible defaults.These methods currently return false/null or do nothing, which can mask errors and break password updates for subclasses that don’t override them. Either:
- Make them abstract to force implementations in all subclasses, or
- Provide a default backed by the concrete credential store.
This was previously flagged; still applicable.
Example (if keeping concrete defaults in AbstractUser, adapt to actual storage):
- public boolean isCredentialEnabled(String type) { - return false; - } + public boolean isCredentialEnabled(String type) { + Credential<?> c = getCredential(type); + return c != null && c.isEnabled(); + } - public Credential<?> getCredential(String type) { - return null; - } + public Credential<?> getCredential(String type) { + // delegate to subclass or shared store; return null by default if unavailable + return null; + } - public void disableCredential(String type) { - } + public void disableCredential(String type) { + Credential<?> c = getCredential(type); + if (c != null) c.setEnabled(false); + } - public void setCredentialProperty(String type, String key, Object value) { - } + public void setCredentialProperty(String type, String key, Object value) { /* delegate or no-op */ } - public Object getCredentialProperty(String type, String key) { - return null; - } + public Object getCredentialProperty(String type, String key) { return null; }Also applies to: 169-171, 178-179, 188-189, 198-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java(4 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java(24 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (4)
52-55: Javadoc improvement looks good.Clearer description for avatar. No action needed.
144-151: Javadoc clarity LGTM.Overload docs read well.
203-208: Name computation LGTM.Trim handles spacing correctly given @NotNull first/last names.
211-218: Full name computation LGTM.Middle name handling is safe.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Outdated
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Show resolved
Hide resolved
- Changed log level in `UserServiceImpl` from trace to debug for better verbosity. - Improved null safety in `User` initialization and credential handling. - Set default value for `credentials` in `User` to avoid null-pointer issues. - Replaced `.equals()` with `==` for better performance checking `UserState`.
tuplle
previously approved these changes
Sep 5, 2025
coderabbitai
bot
added
improvement
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (4)
91-94: Use a stable 'token' key; getters use the literal "token".
setToken()inserts bytokenCredential.getType()while readers use"token". If the type ever differs (case/override), reads will fail. Insert under the literal key for consistency.public void setToken(String token) { TokenCredential tokenCredential = new TokenCredential(token, 1, true); - this.credentials.put(tokenCredential.getType(), tokenCredential); + this.credentials.put("token", tokenCredential); }
146-151: Harden type handling in getExpirationDate to avoid ClassCastException.Guard the Date cast; log on unexpected type.
- if (obj instanceof LocalDateTime) { - return (LocalDateTime) obj; - } - return DateUtils.dateToLocalDateTime((Date) obj); + if (obj instanceof LocalDateTime ldt) { + return ldt; + } + if (obj instanceof Date d) { + return DateUtils.dateToLocalDateTime(d); + } + log.warn("Unexpected token expirationDate property type: {}", obj.getClass()); + return null;
339-345: removeCredential: avoid reinitializing map on null.No-op if
credentialsis null; don't allocate a new map.- if (this.credentials == null) { - this.credentials = new HashMap<>(); - } else { - this.credentials.remove(key); - } + if (this.credentials != null) { + this.credentials.remove(key); + }
366-371: Return a defensive copy from getCredentialsKeys.Expose an unmodifiable copy to protect internal state.
- if (this.credentials == null) { - return Set.of(); - } - return this.credentials.keySet(); + if (this.credentials == null) { + return Set.of(); + } + return java.util.Collections.unmodifiableSet(new java.util.HashSet<>(this.credentials.keySet()));
♻️ Duplicate comments (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (1)
51-55: Ensure credentials map is never null across all code paths.@AllArgsConstructor still allows null assignment; several methods assume non-null (e.g., getEnabledMFAMethods). Centralize a null-guard and use it where dereferencing occurs. This echoes prior feedback.
@@ - private Map<String, Credential<?>> credentials = new HashMap<>(); + private Map<String, Credential<?>> credentials = new HashMap<>(); + + private Map<String, Credential<?>> ensureCredentials() { + if (this.credentials == null) this.credentials = new HashMap<>(); + return this.credentials; + }Apply in stream-based methods (example shown):
- public Set<String> getEnabledMFAMethods() { - return this.credentials.entrySet().stream() + public Set<String> getEnabledMFAMethods() { + return ensureCredentials().entrySet().stream() .filter(entry -> { Credential<?> credential = entry.getValue(); return credential != null && credential.getType() != null && credential.getType().toUpperCase().startsWith("MFA") && credential.isEnabled(); }) .map(Map.Entry::getKey) .collect(Collectors.toSet()); }nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
271-273: Reuse setPassword(...) helper and fix log wording.Avoid duplicating encoding logic; keep consistent logging.
- log.debug("Setting password for user [{}]", user.getUsername()); - user.setPassword(passwordEncoder.encode(newPassword)); - return saveUser(user); + log.debug("Changing password for user [{}]", user.getUsername()); + setPassword(user, newPassword); + return saveUser(user);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java(24 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(2 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
🧬 Code graph analysis (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (1)
Data(24-381)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
Data(16-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (1)
171-182: Confirm semantics: keys vs types in getEnabledMFAMethods.Method returns map keys (e.g., "MFA-TOTP"), not
credential.getType()(e.g., "TOTP"). Is the API expecting prefixed keys or raw types?nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (1)
61-64: LGTM: defensive attributes copy and fullName usage.Copy-on-write of attributes and
getFullName()adoption look good.nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
263-274: Admin flow: add an overload or confirm bypass of old-password check
No invocations ofUserServiceImpl.changePassword(AbstractUser, String, String)were found—old-password verification is always enforced. Confirm how admins should bypass this or introduce a dedicated method/endpoint for admin-initiated password resets.
Comment on lines
+644
to
+649
| throw new IllegalArgumentException("confirmation password is not set"); | ||
| } | ||
|
|
||
| log.trace("Verifying password for user [{}]", user.getUsername()); | ||
| return passwordEncoder.matches(password, user.getPassword()); | ||
| } |
There was a problem hiding this comment.
Null-safe verification and clearer message.
Handle null stored password and align message with param meaning.
- if (password == null) {
- throw new IllegalArgumentException("confirmation password is not set");
- }
-
- log.trace("Verifying password for user [{}]", user.getUsername());
- return passwordEncoder.matches(password, user.getPassword());
+ if (password == null) {
+ throw new IllegalArgumentException("Old password is not set");
+ }
+ String stored = user.getPassword();
+ if (stored == null) {
+ log.warn("User [{}] has no stored password to verify against", user.getUsername());
+ return false;
+ }
+ log.trace("Verifying old password for user [{}]", user.getUsername());
+ return passwordEncoder.matches(password, stored);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| throw new IllegalArgumentException("confirmation password is not set"); | |
| } | |
| log.trace("Verifying password for user [{}]", user.getUsername()); | |
| return passwordEncoder.matches(password, user.getPassword()); | |
| } | |
| if (password == null) { | |
| throw new IllegalArgumentException("Old password is not set"); | |
| } | |
| String stored = user.getPassword(); | |
| if (stored == null) { | |
| log.warn("User [{}] has no stored password to verify against", user.getUsername()); | |
| return false; | |
| } | |
| log.trace("Verifying old password for user [{}]", user.getUsername()); | |
| return passwordEncoder.matches(password, stored); |
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 644-649, the code currently throws "confirmation password is not
set" and then calls passwordEncoder.matches without guarding against a null
stored password; change this to check user.getPassword() for null and throw a
clearer exception like "stored password is not set for user [username]" (or
return false) before calling passwordEncoder.matches, so verification is
null-safe and the message reflects the stored password rather than the
input/confirmation parameter.
Comment on lines
+651
to
+654
| protected void canUpdatePassword(AbstractUser user, String password) { | ||
| if (!user.isCredentialEnabled("password")) { | ||
| throw new RuntimeException("Password does not exists or authorization is not enabled"); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify exception text; avoid generic RuntimeException.
Use precise wording and a more specific exception type.
- if (!user.isCredentialEnabled("password")) {
- throw new RuntimeException("Password does not exists or authorization is not enabled");
- }
+ if (!user.isCredentialEnabled("password")) {
+ throw new IllegalStateException("Password credential does not exist or is disabled");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected void canUpdatePassword(AbstractUser user, String password) { | |
| if (!user.isCredentialEnabled("password")) { | |
| throw new RuntimeException("Password does not exists or authorization is not enabled"); | |
| } | |
| protected void canUpdatePassword(AbstractUser user, String password) { | |
| if (!user.isCredentialEnabled("password")) { | |
| throw new IllegalStateException("Password credential does not exist or is disabled"); | |
| } |
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 651 to 654, the method throws a generic RuntimeException with an
unclear message; replace it with a more specific exception (for example
IllegalStateException or a custom AuthorizationException) and update the message
to be precise and user/developer friendly (e.g., "Password credential is not
present or password authentication is not enabled for this user"). Ensure to
import or create the chosen exception type and use it consistently.
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Outdated
Show resolved
Hide resolved
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
Outdated
Show resolved
Hide resolved
# Conflicts: # nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
- Renamed variable `userr` to `domainUser` for improved clarity and consistency. - Added null and empty checks for `enabledCredentialsAttribute` to prevent potential runtime issues.
tuplle
approved these changes
Sep 6, 2025
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (2)
652-659: Null-safety + clearer messaging in verification.
Align message with meaning, guard null stored password; current code may log misleadingly and can NPE with some encoders.- private boolean verifyPasswords(AbstractUser user, String password) { - if (password == null) { - throw new IllegalArgumentException("confirmation password is not set"); - } - - log.trace("Verifying password for user [{}]", user.getUsername()); - return passwordEncoder.matches(password, user.getPassword()); - } + private boolean verifyPasswords(AbstractUser user, String password) { + if (password == null) { + throw new IllegalArgumentException("Old password is not set"); + } + String stored = user.getPassword(); + if (stored == null) { + log.warn("User [{}] has no stored password to verify against", user.getUsername()); + return false; + } + log.trace("Verifying old password for user [{}]", user.getUsername()); + return passwordEncoder.matches(password, stored); + }
661-669: Use a specific exception and fix grammar; clarify which password is validated.
Prefer IllegalStateException and precise text; also clarify “new password” null.- protected void canUpdatePassword(AbstractUser user, String password) { - if (!user.isCredentialEnabled("password")) { - throw new RuntimeException("Password does not exists or authorization is not enabled"); - } - - if (password == null) { - throw new IllegalArgumentException("Password is not set"); - } - } + protected void canUpdatePassword(AbstractUser user, String password) { + if (!user.isCredentialEnabled("password")) { + throw new IllegalStateException("Password credential does not exist or is disabled"); + } + if (password == null) { + throw new IllegalArgumentException("New password is not set"); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(2 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser(), and both setPassword() and setCredential() ultimately reference the same credential system.
Applied to files:
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
📚 Learning: 2025-09-04T11:09:31.264Z
Learnt from: renczesstefan
PR: netgrif/application-engine#350
File: application-engine/src/main/java/com/netgrif/application/engine/startup/runner/SuperCreatorRunner.java:56-63
Timestamp: 2025-09-04T11:09:31.264Z
Learning: In SuperCreatorRunner.java, when creating a user with PasswordCredential via setCredential("password", passwordCredential), there's no need to also call setPassword() as it would be redundant - the PasswordCredential already contains the password information needed by UserServiceImpl.createUser().
Applied to files:
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java
🧬 Code graph analysis (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (2)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java (1)
Data(24-381)nae-object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java (1)
Data(16-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (5)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/web/responsebodies/User.java (4)
18-18: Constant visibility is appropriate; name is clear.
Public key will help downstream clients reference the attribute consistently.
40-52: Nice null-safe, non-mutating extraction of enabled credential types.
Handles null maps/entries and avoids mutating domain attributes.
61-61: Using getFullName() is correct.
Matches documented semantics.
62-67: Defensive copy + only include when non-empty — LGTM.
Prevents leaking empty attributes and avoids DTO/domain aliasing.nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java (1)
411-419: Swap parameters for changePassword method signature
No direct callers found; safe to reorder parameters to (oldPassword, newPassword).
Comment on lines
+264
to
+275
| @Override | ||
| public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) { | ||
| canUpdatePassword(user, newPassword); | ||
|
|
||
| if (!verifyPasswords(user, oldPassword)) { | ||
| throw new IllegalArgumentException("Old password does not match."); | ||
| } | ||
|
|
||
| log.debug("Setting password for user [{}]", user.getUsername()); | ||
| user.setPassword(passwordEncoder.encode(newPassword)); | ||
| return saveUser(user); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add tests for self-change vs admin-change paths.
Cover: non-admin correct/incorrect old password; admin with null oldPassword; disabled password credential.
I can draft unit tests for UserServiceImpl.changePassword(), mocking PasswordEncoder and SecurityContext. Want me to add them?
Admin bypass not implemented; service always requires old password.
This contradicts PR behavior (“admin doesn’t confirm old password”) and will block admin flows. Also, reuse setPassword() for consistency.
@Override
- public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) {
- canUpdatePassword(user, newPassword);
-
- if (!verifyPasswords(user, oldPassword)) {
- throw new IllegalArgumentException("Old password does not match.");
- }
-
- log.debug("Setting password for user [{}]", user.getUsername());
- user.setPassword(passwordEncoder.encode(newPassword));
- return saveUser(user);
- }
+ public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) {
+ canUpdatePassword(user, newPassword);
+
+ // Admins may change passwords without supplying the old password
+ AbstractUser actor = getLoggedUser();
+ boolean isAdmin = actor.getAuthoritySet().contains(authorityService.getOrCreate(Authority.admin));
+ if (!isAdmin) {
+ if (!verifyPasswords(user, oldPassword)) {
+ throw new IllegalArgumentException("Old password does not match.");
+ }
+ }
+
+ log.debug("Changing password for user [{}]", user.getUsername());
+ setPassword(user, newPassword);
+ return saveUser(user);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) { | |
| canUpdatePassword(user, newPassword); | |
| if (!verifyPasswords(user, oldPassword)) { | |
| throw new IllegalArgumentException("Old password does not match."); | |
| } | |
| log.debug("Setting password for user [{}]", user.getUsername()); | |
| user.setPassword(passwordEncoder.encode(newPassword)); | |
| return saveUser(user); | |
| } | |
| @Override | |
| public AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword) { | |
| canUpdatePassword(user, newPassword); | |
| // Admins may change passwords without supplying the old password | |
| AbstractUser actor = getLoggedUser(); | |
| boolean isAdmin = actor.getAuthoritySet().contains(authorityService.getOrCreate(Authority.admin)); | |
| if (!isAdmin) { | |
| if (!verifyPasswords(user, oldPassword)) { | |
| throw new IllegalArgumentException("Old password does not match."); | |
| } | |
| } | |
| log.debug("Changing password for user [{}]", user.getUsername()); | |
| setPassword(user, newPassword); | |
| return saveUser(user); | |
| } |
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
around lines 264-275, the method always enforces verifying the old password
which prevents admins from bypassing confirmation; update the logic to skip
verifyPasswords when the caller is an admin (check
SecurityContext/Authentication for admin role or platform admin flag), still
call canUpdatePassword(user, newPassword), and handle null oldPassword safely;
replace the direct user.setPassword(passwordEncoder.encode(newPassword)) call
with the existing setPassword(...) helper for consistency, then persist with
saveUser(user).
Comment on lines
+411
to
+419
| /** | ||
| * Resets password for user. | ||
| * | ||
| * @param user user | ||
| * @param newPassword new password | ||
| * @param oldPassword old password | ||
| * @return the updated user | ||
| */ | ||
| AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify admin bypass and parameter intent in Javadoc.
The UI hides old-password for admins; document that oldPassword may be null for admins and tighten throws.
- /**
- * Resets password for user.
- *
- * @param user user
- * @param newPassword new password
- * @param oldPassword old password
- * @return the updated user
- */
+ /**
+ * Changes the user's password.
+ * Non-admin callers must supply the current password; admins may pass {@code null} for {@code oldPassword}.
+ *
+ * @param user the target user
+ * @param newPassword the new password (raw)
+ * @param oldPassword the current password (raw), or {@code null} when invoked by an admin
+ * @return the updated user
+ * @throws IllegalArgumentException if {@code newPassword} is null or (for non-admin callers) {@code oldPassword} does not match
+ * @throws IllegalStateException if the password credential does not exist or is disabled
+ */
AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Resets password for user. | |
| * | |
| * @param user user | |
| * @param newPassword new password | |
| * @param oldPassword old password | |
| * @return the updated user | |
| */ | |
| AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword); | |
| /** | |
| * Changes the user's password. | |
| * Non-admin callers must supply the current password; admins may pass {@code null} for {@code oldPassword}. | |
| * | |
| * @param user the target user | |
| * @param newPassword the new password (raw) | |
| * @param oldPassword the current password (raw), or {@code null} when invoked by an admin | |
| * @return the updated user | |
| * @throws IllegalArgumentException if {@code newPassword} is null or (for non-admin callers) {@code oldPassword} does not match | |
| * @throws IllegalStateException if the password credential does not exist or is disabled | |
| */ | |
| AbstractUser changePassword(AbstractUser user, String newPassword, String oldPassword); |
🤖 Prompt for AI Agents
In
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
around lines 411–419, update the method Javadoc to clarify that oldPassword may
be null when the caller is an administrator (UI hides old-password for admins)
and tighten the @throws tags to explicitly document error cases: add @throws
IllegalArgumentException when newPassword is invalid or when oldPassword is
required but missing/mismatched, and add @throws
org.springframework.security.access.AccessDeniedException when the caller lacks
permission to change the password. Keep the description concise and reflect that
implementations must validate inputs accordingly.
18 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request focuses on the functionality of changing user passwords in the admin application.
Implements NAE-2173
How Has Been This Tested?
I tested the code manually. By clicking in the admin application in the user details section. Tested cases:
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Improvements
Documentation