Skip to content

[NAE-2173] Password change #345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Sep 6, 2025
Merged

[NAE-2173] Password change #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b9d18b1
NAE-2173 - implementacia fukcionality zmeny hesla pre pouzivatela
dominikvozr Aug 26, 2025
e86404b
NAE-2173 - revert RC6 -> SNAPSHOT
dominikvozr Aug 26, 2025
f4e7c5a
NAE-2173 - pull request fix, exception throwing if old password doesn…
dominikvozr Aug 27, 2025
e9d26c8
NAE-2173 - Add ATTR_ENABLED_CREDENTIALS constant
dominikvozr Aug 27, 2025
cba755d
[NAE-2173] Password change
tuplle Sep 5, 2025
ca8a6d0
[NAE-2173] Password change
tuplle Sep 5, 2025
bc3dd08
[NAE-2173] Password change
tuplle Sep 5, 2025
5690350
Merge remote-tracking branch 'origin/release/7.0.0-rev7' into NAE-2173
tuplle Sep 6, 2025
56cc11f
[NAE-2173] Password change
tuplle Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ...ibrary/src/main/java/com/netgrif/application/engine/objects/auth/domain/AbstractUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public abstract class AbstractUser extends AbstractActor {
protected String email;

/**
* URL or identifier of user's avatar image
* URL or identifier of the user's avatar image
*/
protected String avatar;

Expand Down Expand Up @@ -141,7 +141,7 @@ public void activateMFA(String type, String secret) {
}

/**
* Activates Multi-Factor Authentication with enabled state.
* Activates Multi-Factor Authentication with the enabled state.
*
* @param type MFA type identifier
* @param secret MFA secret key
Expand All @@ -154,7 +154,7 @@ public void activateMFA(String type, String secret, boolean enabled) {
* Checks if a credential is enabled.
*
* @param type credential type to check
* @return true if credential is enabled, false otherwise
* @return true if the credential is enabled, false otherwise
*/
public boolean isCredentialEnabled(String type) {
return false;
Expand Down Expand Up @@ -200,15 +200,15 @@ public Object getCredentialProperty(String type, String key) {
}

/**
* Returns user's first and last name concatenated.
* Returns a user's first and last name concatenated.
*/
@Override
public String getName() {
return String.join(" ", firstName, lastName).trim();
}

/**
* Returns user's full name including middle name if present.
* Returns the user's full name including middle name if present.
*/
@Override
public String getFullName() {
Expand Down
78 changes: 55 additions & 23 deletions ...object-library/src/main/java/com/netgrif/application/engine/objects/auth/domain/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package com.netgrif.application.engine.objects.auth.domain;

import com.netgrif.application.engine.objects.auth.domain.enums.UserState;
import com.netgrif.application.engine.objects.annotations.Indexed;
import com.netgrif.application.engine.objects.petrinet.domain.roles.ProcessRole;
import com.netgrif.application.engine.objects.utils.DateUtils;
import com.querydsl.core.annotations.QueryEntity;
import lombok.AllArgsConstructor;
Expand Down Expand Up @@ -30,19 +28,29 @@
@EqualsAndHashCode(callSuper = false)
public class User extends AbstractUser implements Serializable {

/** Flag indicating whether the user's email has been verified */
/**
* Flag indicating whether the user's email has been verified
*/
private boolean emailVerified;

/** Current state of the user (e.g., ACTIVE, INACTIVE) */
/**
* Current state of the user (e.g., ACTIVE, INACTIVE)
*/
private UserState state;

/** Timestamp when the user was created */
/**
* Timestamp when the user was created
*/
private LocalDateTime createdAt = LocalDateTime.now();

/** Timestamp of the last modification to the user */
/**
* Timestamp of the last modification to the user
*/
private LocalDateTime modifiedAt = LocalDateTime.now();

/** Map containing user's credentials with credential type as key */
/**
* Map containing user's credentials with credential type as key
*/
private Map<String, Credential<?>> credentials = new HashMap<>();

/**
Expand All @@ -57,6 +65,7 @@ public User() {

/**
* Constructor creating a User with a specified ObjectId
*
* @param id The ObjectId to be assigned to the user
*/
public User(ObjectId id) {
Expand All @@ -66,6 +75,7 @@ public User(ObjectId id) {

/**
* Retrieves the user's authentication token from credentials
*
* @return The token string if present, null otherwise
*/
public String getToken() {
Expand All @@ -75,6 +85,7 @@ public String getToken() {

/**
* Sets the user's authentication token
*
* @param token The token string to be stored
*/
public void setToken(String token) {
Expand All @@ -84,6 +95,7 @@ public void setToken(String token) {

/**
* Retrieves the encoded password from credentials
*
* @return The encoded password string if present, null otherwise
*/
@Override
Expand All @@ -97,6 +109,7 @@ public String getPassword() {

/**
* Sets the user's password (should be already encoded)
*
* @param password The encoded password string to store
*/
@Override
Expand All @@ -107,6 +120,7 @@ public void setPassword(String password) {

/**
* Sets the expiration date for the user's token
*
* @param expirationDate LocalDateTime when the token should expire
*/
public void setExpirationDate(LocalDateTime expirationDate) {
Expand All @@ -122,6 +136,7 @@ public void setExpirationDate(LocalDateTime expirationDate) {

/**
* Retrieves the token expiration date
*
* @return LocalDateTime of token expiration if present, null otherwise
*/
public LocalDateTime getExpirationDate() {
Expand All @@ -138,6 +153,7 @@ public LocalDateTime getExpirationDate() {

/**
* Checks if a specific credential type is enabled
*
* @param type The credential type to check
* @return true if the credential exists and is enabled, false otherwise
*/
Expand All @@ -149,6 +165,7 @@ public boolean isCredentialEnabled(String type) {

/**
* Returns a set of enabled MFA method names
*
* @return Set of strings representing enabled MFA methods
*/
public Set<String> getEnabledMFAMethods() {
Expand All @@ -166,6 +183,7 @@ public Set<String> getEnabledMFAMethods() {

/**
* Disables a specific credential type
*
* @param type The credential type to disable
*/
@Override
Expand All @@ -177,7 +195,8 @@ public void disableCredential(String type) {

/**
* Activates an MFA method with the given secret (enabled by default)
* @param type The MFA type to activate
*
* @param type The MFA type to activate
* @param secret The secret key for the MFA method
*/
@Override
Expand All @@ -187,8 +206,9 @@ public void activateMFA(String type, String secret) {

/**
* Activates an MFA method with the given secret and enabled state
* @param type The MFA type to activate
* @param secret The secret key for the MFA method
*
* @param type The MFA type to activate
* @param secret The secret key for the MFA method
* @param enabled Whether the MFA method should be enabled
* @throws IllegalArgumentException if type or secret is null or empty
*/
Expand All @@ -206,8 +226,9 @@ public void activateMFA(String type, String secret, boolean enabled) {

/**
* Sets a property for a specific credential type
* @param type The credential type
* @param key The property key
*
* @param type The credential type
* @param key The property key
* @param value The property value (must be Serializable)
*/
@Override
Expand All @@ -227,8 +248,9 @@ public void setCredentialProperty(String type, String key, Object value) {

/**
* Retrieves a property value for a specific credential type
*
* @param type The credential type
* @param key The property key
* @param key The property key
* @return The property value if found, null otherwise
*/
@Override
Expand All @@ -239,6 +261,7 @@ public Object getCredentialProperty(String type, String key) {

/**
* Checks if a specific credential type exists
*
* @param type The credential type to check
* @return true if the credential exists, false otherwise
*/
Expand All @@ -248,6 +271,7 @@ public boolean hasCredential(String type) {

/**
* Retrieves a credential by its type
*
* @param type The credential type
* @return The Credential object if found, null otherwise
*/
Expand All @@ -258,17 +282,19 @@ public Credential<?> getCredential(String type) {

/**
* Sets the entire credentials map
* @param credentials The new credentials map (null creates empty map)
*
* @param credentials The new credentials map (null creates an empty map)
*/
public void setCredentials(Map<String, Credential<?>> credentials) {
this.credentials = credentials == null ? new HashMap<>() : new HashMap<>(credentials);
}

/**
* Creates and sets a new string credential
* @param type The credential type
* @param value The credential value
* @param order The credential order
*
* @param type The credential type
* @param value The credential value
* @param order The credential order
* @param enabled Whether the credential should be enabled
*/
@Override
Expand All @@ -279,7 +305,8 @@ public void setCredential(String type, String value, int order, boolean enabled)

/**
* Sets a credential with the given key
* @param key The credential key
*
* @param key The credential key
* @param credential The credential object to set
*/
@Override
Expand All @@ -292,6 +319,7 @@ public void setCredential(String key, Credential<?> credential) {

/**
* Gets the value of a credential by its key
*
* @param key The credential key
* @return The credential value if found, null otherwise
*/
Expand All @@ -305,6 +333,7 @@ public Object getCredentialValue(String key) {

/**
* Removes a credential by its key
*
* @param key The credential key to remove
*/
public void removeCredential(String key) {
Expand All @@ -317,8 +346,9 @@ public void removeCredential(String key) {

/**
* Checks if a credential is set and has a value
*
* @param key The credential key to check
* @return true if credential exists and has a value, false otherwise
* @return true if a credential exists and has a value, false otherwise
*/
public boolean isCredentialSet(String key) {
if (this.credentials == null) {
Expand All @@ -330,6 +360,7 @@ public boolean isCredentialSet(String key) {

/**
* Gets all credential keys
*
* @return Set of credential keys, empty set if no credentials exist
*/
public Set<String> getCredentialsKeys() {
Expand All @@ -340,10 +371,11 @@ public Set<String> getCredentialsKeys() {
}

/**
* Checks if the user is in ACTIVE state
* @return true if user state is ACTIVE, false otherwise
* Checks if the user is in an ACTIVE state
*
* @return true if the user state is ACTIVE, false otherwise
*/
public boolean isActive() {
return this.state.equals(UserState.ACTIVE);
return this.state == UserState.ACTIVE;
}
}
}
31 changes: 31 additions & 0 deletions nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,19 @@ public List<Group> getUserGroups(AbstractActor actor) {
return groupService.findAllByIds(actor.getGroupIds(), Pageable.unpaged()).stream().toList();
}

@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);
}
Comment on lines +264 to +275
Copy link

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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?

⚠️ Potential issue

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).


@Override
public void addDefaultAuthorities(AbstractUser user) {
log.trace("Assigning default authorities to user [{}]", user.getUsername());
Expand Down Expand Up @@ -636,4 +649,22 @@ private void resolveRelatedProcessRoles(AbstractUser user) {
user.getAuthoritySet().addAll(getUserGroups(user).stream().map(Group::getAuthoritySet).flatMap(Set::stream).collect(Collectors.toSet()));
}

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());
}
Comment on lines +654 to +659
Copy link

@coderabbitai coderabbitai bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


protected void canUpdatePassword(AbstractUser user, String password) {
if (!user.isCredentialEnabled("password")) {
throw new RuntimeException("Password does not exists or authorization is not enabled");
}
Comment on lines +661 to +664
Copy link

@coderabbitai coderabbitai bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


if (password == null) {
throw new IllegalArgumentException("Password is not set");
}
}
}
10 changes: 10 additions & 0 deletions nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,14 @@ Page<AbstractUser> searchAllCoMembers(String query, Collection<ProcessResourceId
* @param roles collection of process roles to assign to admin users
*/
void updateAdminWithRoles(Collection<ProcessRole> roles);

/**
* 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);
Comment on lines +411 to +419
Copy link

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

}
Loading
Loading