OF-3276: Autosetup should not try to modify admin user unless instructed#3328
OF-3276: Autosetup should not try to modify admin user unless instructed#3328guusdk wants to merge 1 commit into
Conversation
Only try to update the admin user when there are actual autosetup values provided. This prevents warnings being logged when autosetup is using a configuration that does not use the default 'admin' user.
📝 WalkthroughWalkthroughDuring auto-setup initialization, the default admin user's password and email configuration is now conditional on whether those settings are provided in the XML configuration. The code checks for the presence of Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 1
🧹 Nitpick comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java (1)
532-536: ⚡ Quick winLog the exception via logger instead of printing to stderr.
On Line 533,
e.printStackTrace()bypasses normal logging and Line 534-536 drops the throwable from log context. Prefer structured logging with the exception attached.Proposed fix
} catch (Exception e) { - e.printStackTrace(); logger.warn("There was an unexpected error encountered when " - + "setting the new admin information. Please check your error " - + "logs and try to remedy the problem."); + + "setting the new admin information. Please check your error " + + "logs and try to remedy the problem.", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java` around lines 532 - 536, Remove the direct stderr print and attach the caught exception to the logger in the catch block in XMPPServer.java: delete the e.printStackTrace() call and change the logger.warn call in the catch(Exception e) handler so it logs the same descriptive message with the Throwable (e) passed to the logger (e.g., logger.warn(..., e)) to preserve stacktrace and structured logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java`:
- Around line 520-527: The autosetup block in XMPPServer currently checks only
for null via JiveGlobals.getXMLProperty(...) which allows empty or whitespace
values to overwrite admin fields; update the logic around
UserManager.getInstance().getUser("admin") so that before calling
adminUser.setPassword(...) or adminUser.setEmail(...) you verify the retrieved
XML property is non-null and not blank (e.g., trim().length()>0 or using a
StringUtils.isNotBlank check) and skip calling setPassword/setEmail when the
value is blank, ensuring blank autosetup values are treated as absent.
---
Nitpick comments:
In `@xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java`:
- Around line 532-536: Remove the direct stderr print and attach the caught
exception to the logger in the catch block in XMPPServer.java: delete the
e.printStackTrace() call and change the logger.warn call in the catch(Exception
e) handler so it logs the same descriptive message with the Throwable (e) passed
to the logger (e.g., logger.warn(..., e)) to preserve stacktrace and structured
logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9437d94-ff5b-4f7f-8a3a-6401059c111f
📒 Files selected for processing (1)
xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java
| if (JiveGlobals.getXMLProperty("autosetup.admin.password") != null || JiveGlobals.getXMLProperty("autosetup.admin.email") != null) { | ||
| try { | ||
| User adminUser = UserManager.getInstance().getUser("admin"); | ||
| if (JiveGlobals.getXMLProperty("autosetup.admin.password") != null) { | ||
| adminUser.setPassword(JiveGlobals.getXMLProperty("autosetup.admin.password")); | ||
| } | ||
| if (JiveGlobals.getXMLProperty("autosetup.admin.email") != null) { | ||
| adminUser.setEmail(JiveGlobals.getXMLProperty("autosetup.admin.email")); |
There was a problem hiding this comment.
Treat blank autosetup values as absent before updating admin fields.
On Line 520 and Line 523/526, null-only checks allow empty/blank XML values to update the admin user. That still counts as a modification without a real value and can set an empty password/email.
Proposed fix
- if (JiveGlobals.getXMLProperty("autosetup.admin.password") != null || JiveGlobals.getXMLProperty("autosetup.admin.email") != null) {
+ final String autosetupAdminPassword = JiveGlobals.getXMLProperty("autosetup.admin.password");
+ final String autosetupAdminEmail = JiveGlobals.getXMLProperty("autosetup.admin.email");
+ if ((autosetupAdminPassword != null && !autosetupAdminPassword.isBlank())
+ || (autosetupAdminEmail != null && !autosetupAdminEmail.isBlank())) {
try {
User adminUser = UserManager.getInstance().getUser("admin");
- if (JiveGlobals.getXMLProperty("autosetup.admin.password") != null) {
- adminUser.setPassword(JiveGlobals.getXMLProperty("autosetup.admin.password"));
+ if (autosetupAdminPassword != null && !autosetupAdminPassword.isBlank()) {
+ adminUser.setPassword(autosetupAdminPassword);
}
- if (JiveGlobals.getXMLProperty("autosetup.admin.email") != null) {
- adminUser.setEmail(JiveGlobals.getXMLProperty("autosetup.admin.email"));
+ if (autosetupAdminEmail != null && !autosetupAdminEmail.isBlank()) {
+ adminUser.setEmail(autosetupAdminEmail);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/XMPPServer.java` around
lines 520 - 527, The autosetup block in XMPPServer currently checks only for
null via JiveGlobals.getXMLProperty(...) which allows empty or whitespace values
to overwrite admin fields; update the logic around
UserManager.getInstance().getUser("admin") so that before calling
adminUser.setPassword(...) or adminUser.setEmail(...) you verify the retrieved
XML property is non-null and not blank (e.g., trim().length()>0 or using a
StringUtils.isNotBlank check) and skip calling setPassword/setEmail when the
value is blank, ensuring blank autosetup values are treated as absent.
There was a problem hiding this comment.
@guusdk I get the metal rabbit's concern, but perhaps the risk of someone providing the element without a value is pretty low?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
Only try to update the admin user when there are actual autosetup values provided. This prevents warnings being logged when autosetup is using a configuration that does not use the default 'admin' user.