[Feature] Allow multiple users #6521
Closed
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
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
…gement Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
…m/KingIronMan2011/uptime-kuma into copilot/add-multiple-user-support
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
…omparison is type-safe
…ating, ensuring data integrity and clearer logging.
…support Add multi-user support for basic user management
fix: update dns monitor to evaluate full response list on CAA resolve…
Contributor
Author
|
Will take care of all failed checks |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds multi-user support to Uptime Kuma, allowing multiple admin users to be created and managed through a new user management dashboard. The implementation adds both frontend components and backend socket handlers for CRUD operations on users.
Key Changes:
- New user management UI with the ability to add, edit, activate/deactivate, and delete users
- Backend socket handlers for user CRUD operations with login validation
- Translation keys for the user management interface
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/router.js | Adds route for the new Users settings page |
| src/pages/Settings.vue | Registers the "Users" menu item in the settings navigation |
| src/lang/en.json | Adds 14 translation keys for user management UI messages |
| src/components/settings/Users.vue | New Vue component implementing the user management interface with add/edit/delete functionality |
| server/socket-handlers/user-management-socket-handler.js | New socket handler module implementing getUsers, addUser, updateUser, and deleteUser endpoints |
| server/server.js | Registers the user management socket handler with the server |
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
KingIronMan2011
force-pushed
the
feature/allow-multiple-users
branch
from
135587b to
00f129a
Compare
…prevent self-deactivation Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
…e-warning Add password validation, i18n support, self-deactivation prevention, and enhanced delete warnings to user management
Co-authored-by: KingIronMan2011 <[email protected]>
…d-comments [WIP] Ensure code style compliance and add documentation
Comment on lines
+28
to
+49
| socket.on("getUsers", async (callback) => { | ||
| try { | ||
| checkLogin(socket); | ||
| const users = await R.findAll("user"); | ||
| const userList = users.map(user => ({ | ||
| id: user.id, | ||
| username: user.username, | ||
| active: user.active, | ||
| timezone: user.timezone, | ||
| })); | ||
| callback({ | ||
| ok: true, | ||
| users: userList, | ||
| }); | ||
| } catch (e) { | ||
| log.error("user-management", e.message); | ||
| callback({ | ||
| ok: false, | ||
| msg: e.message, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical security issue: The user management endpoints lack proper authorization checks. Currently, ANY logged-in user can view, create, modify, and delete ALL other users. This allows any user to:
- View all usernames in the system
- Create new users
- Change other users' passwords
- Deactivate or delete other users
- Escalate privileges by creating new accounts
The checkLogin function only verifies that a user is authenticated, not that they have admin/elevated privileges. You need to implement authorization to restrict user management operations to admin users only. Consider adding a role/permission system or at minimum, an is_admin flag in the user table, and check it before allowing these operations.
Comment on lines
52
to
94
| socket.on("addUser", async (userData, callback) => { | ||
| try { | ||
| checkLogin(socket); | ||
|
|
||
| // Validate input | ||
| if (!userData.username || !userData.password) { | ||
| throw new Error("Username and password are required"); | ||
| } | ||
|
|
||
| // Validate password strength | ||
| validatePasswordStrength(userData.password); | ||
|
|
||
| // Check if username already exists | ||
| const existingUser = await R.findOne("user", " username = ? ", [ userData.username.trim() ]); | ||
| if (existingUser) { | ||
| throw new Error("Username already exists"); | ||
| } | ||
|
|
||
| // Create new user | ||
| const user = R.dispense("user"); | ||
| user.username = userData.username.trim(); | ||
| user.password = await passwordHash.generate(userData.password); | ||
| user.active = 1; | ||
|
|
||
| await R.store(user); | ||
|
|
||
| log.info("user-management", `User ${user.username} created by user ${socket.userID}`); | ||
|
|
||
| callback({ | ||
| ok: true, | ||
| msg: "User created successfully", | ||
| msgi18n: true, | ||
| userId: user.id, | ||
| }); | ||
| } catch (e) { | ||
| log.error("user-management", e.message); | ||
| callback({ | ||
| ok: false, | ||
| msg: e.message, | ||
| msgi18n: true, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical security issue: Same authorization vulnerability as in getUsers. Any logged-in user can add new users to the system without admin privileges.
Comment on lines
97
to
157
| socket.on("updateUser", async (userId, userData, callback) => { | ||
| try { | ||
| checkLogin(socket); | ||
|
|
||
| const user = await R.findOne("user", " id = ? ", [ userId ]); | ||
| if (!user) { | ||
| throw new Error("User not found"); | ||
| } | ||
|
|
||
| // Check if user is editing their own username | ||
| const isEditingSelf = Number(userId) === Number(socket.userID); | ||
| const usernameChanged = userData.username && userData.username.trim() !== user.username; | ||
|
|
||
| // Don't allow deactivating yourself | ||
| if (isEditingSelf && typeof userData.active !== "undefined" && !userData.active) { | ||
| throw new Error("Cannot deactivate your own account"); | ||
| } | ||
|
|
||
| // Update user fields | ||
| if (userData.username && userData.username.trim() !== user.username) { | ||
| // Check if new username already exists | ||
| const existingUser = await R.findOne("user", " username = ? AND id != ? ", [ | ||
| userData.username.trim(), | ||
| userId | ||
| ]); | ||
| if (existingUser) { | ||
| throw new Error("Username already exists"); | ||
| } | ||
| user.username = userData.username.trim(); | ||
| } | ||
|
|
||
| if (typeof userData.active !== "undefined") { | ||
| user.active = userData.active ? 1 : 0; | ||
| } | ||
|
|
||
| // Update password if provided | ||
| if (userData.password) { | ||
| // Validate password strength | ||
| validatePasswordStrength(userData.password); | ||
| user.password = await passwordHash.generate(userData.password); | ||
| } | ||
|
|
||
| await R.store(user); | ||
|
|
||
| log.info("user-management", `User ${user.username} updated by user ${socket.userID}`); | ||
|
|
||
| callback({ | ||
| ok: true, | ||
| msg: "User updated successfully", | ||
| msgi18n: true, | ||
| requiresLogout: isEditingSelf && usernameChanged, | ||
| }); | ||
| } catch (e) { | ||
| log.error("user-management", e.message); | ||
| callback({ | ||
| ok: false, | ||
| msg: e.message, | ||
| msgi18n: true, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical security issue: Same authorization vulnerability. Any logged-in user can update any other user's username, password, and active status without admin privileges.
Comment on lines
160
to
194
| socket.on("deleteUser", async (userId, callback) => { | ||
| try { | ||
| checkLogin(socket); | ||
|
|
||
| const user = await R.findOne("user", " id = ? ", [ userId ]); | ||
| if (!user) { | ||
| throw new Error("User not found"); | ||
| } | ||
|
|
||
| // Don't allow deleting yourself | ||
| if (Number(user.id) === Number(socket.userID)) { | ||
| throw new Error("Cannot delete your own account"); | ||
| } | ||
|
|
||
| // Permanently remove the user from the database | ||
| await R.trash(user); | ||
|
|
||
| // Disconnect all socket connections for this user | ||
| server.disconnectAllSocketClients(userId); | ||
|
|
||
| log.info("user-management", `User ${user.username} deleted by user ${socket.userID}`); | ||
|
|
||
| callback({ | ||
| ok: true, | ||
| msg: "User deleted successfully", | ||
| msgi18n: true, | ||
| }); | ||
| } catch (error) { | ||
| callback({ | ||
| ok: false, | ||
| msg: error.message, | ||
| msgi18n: true, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Critical security issue: Same authorization vulnerability. Any logged-in user can delete any other user without admin privileges.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
- Add validateUsername function with min/max length, character, and reserved name checks - Integrate validation in addUser and updateUser socket handlers - Add explicit deletion of API keys and SET NULL for monitors/maintenance in deleteUser - Add client-side validation in Users.vue for better UX - Add i18n error messages for validation failures - Add comprehensive backend tests for username validation - All new tests passing (6/6) Co-authored-by: KingIronMan2011 <[email protected]>
Replace technical jargon (NULL, user_id) with clear, plain language explanation Co-authored-by: KingIronMan2011 <[email protected]>
- Extract validateUsername and RESERVED_USERNAMES to server/user-validator.js - Remove code duplication across test, server, and client files - Add constants to Vue component for better maintainability - Remove duplicate i18n entries - First user (setup) remains unrestricted, only user management page validates - All tests passing (6/6) Co-authored-by: KingIronMan2011 <[email protected]>
Ensure trimmed username is used consistently in API calls to match validation Co-authored-by: KingIronMan2011 <[email protected]>
…-issues Add explicit relationship handling and username validation to user management
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
Co-authored-by: KingIronMan2011 <[email protected]>
…r-management Fix E2E test failures: reserved username validation and delete warning text
Owner
|
Duplicate of #6276 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
ℹ️ To keep reviews fast and effective, please make sure you’ve read our pull request guidelines
📝 Summary of changes done and why they are done
This Pull request would add support for making multiple users that all have the normal permissions since I didnt want to implement a permission system, it adds a little user dashboard where the main admin can make new users, change their username and password, this is a pretty simple approach in my opinion and with that very maintainable, I've tested everything locally and it works fine. Just as a warning I've used AI to make this completly except for translations, but checked the code if I found any issues the AI made. If you need more information just comment on this.
📋 Related issues
📄 Checklist
Please follow this checklist to avoid unnecessary back and forth (click to expand)
📷 Screenshots or Visual Changes