Allow to change username #21152
Allow to change username #21152
Conversation
WalkthroughWalkthroughThe core enhancement is the addition of an admin functionality to change user usernames within Home Assistant. This allows administrators to modify usernames directly via the UI, ensuring better user management. Key updates include introducing a new method, Changes
Sequence DiagramssequenceDiagram
participant Admin as Administrator
participant UI as User Interface
participant Auth as Authentication Module
participant Backend as Backend System
Admin->>UI: Initiate username change
UI->>Auth: Call `adminChangeUsername` with userId and new username
Auth->>Backend: Verify admin privileges
Backend-->>Auth: Return verification status
Auth-->>UI: Success/Failure result of username change
UI-->>Admin: Display success/error message through UI
sequenceDiagram
participant Admin as Administrator
participant UI as User Interface
participant Dialog as Dialog Module
participant Auth as Authentication Module
participant Backend as Backend System
Admin->>UI: Trigger user detail dialog
UI->>Dialog: Open dialog for user details
Admin->>UI: Select change username option
UI->>Admin: Prompt for new username
Admin->>UI: Enter new username
UI->>Auth: Invoke `adminChangeUsername` with new username
Auth->>Backend: Process username change
Backend-->>Auth: Return result of change
Auth-->>UI: Notify UI of change status
UI-->>Admin: Display success/error message
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedBiome
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Outside diff range comments (2)
src/data/auth.ts (1)
Line range hint
89-89: Refactor to avoid reassigning function parameters.Reassigning function parameters can lead to confusing and hard-to-maintain code. It's recommended to use local variables instead.
- url += "?"; + let modifiedUrl = url; + modifiedUrl += "?"; - url += "&"; + modifiedUrl += "&"; - url += `code=${encodeURIComponent(authCode)}`; + modifiedUrl += `code=${encodeURIComponent(authCode)}`;Also applies to: 91-91, 94-94, 97-97, 100-100
src/panels/config/person/dialog-person-detail.ts (1)
Line range hint
305-336: Handle user login status changes.This segment of code handles changes to a user's login status, including the creation of new user IDs and the deletion of existing ones. The use of non-null assertions (!) should be avoided where possible as they can lead to runtime errors if not handled properly.
- await this._params!.updateEntry({ user_id: user.id }); + await this._params?.updateEntry({ user_id: user.id });Tools
Biome
[error] 305-305: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (1)
src/panels/config/person/dialog-person-detail.ts (1)
Line range hint
305-338: Handling of user linkage and un-linkage in_allowLoginChangedmethodThis method handles the linking and unlinking of users to a person which is a sensitive operation. It's crucial to ensure that all null assertions are safely handled to prevent runtime errors.
- if (this._params!.entry) { - await this._params!.updateEntry({ user_id: user.id }); - } - this._params?.refreshUsers(); - this._user = user; - this._userId = user.id; - this._isAdmin = user.group_ids.includes(SYSTEM_GROUP_ID_ADMIN); - this._localOnly = user.local_only; + if (this._params?.entry) { + await this._params.updateEntry({ user_id: user.id }); + } + this._params?.refreshUsers(); + this._user = user; + this._userId = user.id; + this._isAdmin = user.group_ids.includes(SYSTEM_GROUP_ID_ADMIN); + this._localOnly = user.local_only;Tools
Biome
[error] 305-305: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
[error] 306-306: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
| .label=${`${this.hass!.localize( | ||
| "ui.panel.config.person.detail.allow_login" | ||
| )} | ||
| )}${this._user ? ` (${this._user.username})` : ""}`} |
There was a problem hiding this comment.
Ensure proper sanitization of this._user.username in UI contexts.
The current implementation directly uses this._user.username in the UI without any sanitization, which could lead to XSS vulnerabilities. Review and implement proper sanitization methods to handle the username safely.
- .label=${`${this.hass!.localize(
- "ui.panel.config.person.detail.allow_login"
- )}${this._user ? ` (${this._user.username})` : ""}`}
+ .label=${`${this.hass!.localize(
+ "ui.panel.config.person.detail.allow_login"
+ )}${this._user ? ` (${this.hass.localize(this._user.username)})` : ""}`}Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 141-141: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
| private async _changeUsername() { | ||
| if (!this._user) { | ||
| return; | ||
| } | ||
| const credential = this._user.credentials.find( | ||
| (cred) => cred.type === "homeassistant" | ||
| ); | ||
| if (!credential) { | ||
| showAlertDialog(this, { | ||
| title: "No Home Assistant credentials found.", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const newUsername = await showPromptDialog(this, { | ||
| inputLabel: this.hass.localize( | ||
| "ui.panel.config.users.change_username.new_username" | ||
| ), | ||
| confirmText: this.hass.localize( | ||
| "ui.panel.config.users.change_username.change" | ||
| ), | ||
| title: this.hass.localize( | ||
| "ui.panel.config.users.change_username.caption" | ||
| ), | ||
| defaultValue: this._user.username!, | ||
| }); | ||
| if (newUsername) { | ||
| try { | ||
| await adminChangeUsername(this.hass, this._user.id, newUsername); | ||
| this._params?.refreshUsers(); | ||
| this._user = { ...this._user, username: newUsername }; | ||
| showAlertDialog(this, { | ||
| text: this.hass.localize( | ||
| "ui.panel.config.users.change_username.username_changed" | ||
| ), | ||
| }); | ||
| } catch (err: any) { | ||
| showAlertDialog(this, { | ||
| title: this.hass.localize( | ||
| "ui.panel.config.users.change_username.failed" | ||
| ), | ||
| text: err.message, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation of the _changeUsername method
This method is well-implemented with error handling and user prompts. However, the use of a non-null assertion on this._user.username is potentially unsafe and should be replaced with optional chaining.
- const newUsername = await showPromptDialog(this, {
- inputLabel: this.hass.localize(
- "ui.panel.config.users.change_username.new_username"
- ),
- confirmText: this.hass.localize(
- "ui.panel.config.users.change_username.change"
- ),
- title: this.hass.localize(
- "ui.panel.config.users.change_username.caption"
- ),
- defaultValue: this._user.username!,
- });
+ const newUsername = await showPromptDialog(this, {
+ inputLabel: this.hass.localize(
+ "ui.panel.config.users.change_username.new_username"
+ ),
+ confirmText: this.hass.localize(
+ "ui.panel.config.users.change_username.change"
+ ),
+ title: this.hass.localize(
+ "ui.panel.config.users.change_username.caption"
+ ),
+ defaultValue: this._user?.username,
+ });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.
| private async _changeUsername() { | |
| if (!this._user) { | |
| return; | |
| } | |
| const credential = this._user.credentials.find( | |
| (cred) => cred.type === "homeassistant" | |
| ); | |
| if (!credential) { | |
| showAlertDialog(this, { | |
| title: "No Home Assistant credentials found.", | |
| }); | |
| return; | |
| } | |
| const newUsername = await showPromptDialog(this, { | |
| inputLabel: this.hass.localize( | |
| "ui.panel.config.users.change_username.new_username" | |
| ), | |
| confirmText: this.hass.localize( | |
| "ui.panel.config.users.change_username.change" | |
| ), | |
| title: this.hass.localize( | |
| "ui.panel.config.users.change_username.caption" | |
| ), | |
| defaultValue: this._user.username!, | |
| }); | |
| if (newUsername) { | |
| try { | |
| await adminChangeUsername(this.hass, this._user.id, newUsername); | |
| this._params?.refreshUsers(); | |
| this._user = { ...this._user, username: newUsername }; | |
| showAlertDialog(this, { | |
| text: this.hass.localize( | |
| "ui.panel.config.users.change_username.username_changed" | |
| ), | |
| }); | |
| } catch (err: any) { | |
| showAlertDialog(this, { | |
| title: this.hass.localize( | |
| "ui.panel.config.users.change_username.failed" | |
| ), | |
| text: err.message, | |
| }); | |
| } | |
| } | |
| } | |
| private async _changeUsername() { | |
| if (!this._user) { | |
| return; | |
| } | |
| const credential = this._user.credentials.find( | |
| (cred) => cred.type === "homeassistant" | |
| ); | |
| if (!credential) { | |
| showAlertDialog(this, { | |
| title: "No Home Assistant credentials found.", | |
| }); | |
| return; | |
| } | |
| const newUsername = await showPromptDialog(this, { | |
| inputLabel: this.hass.localize( | |
| "ui.panel.config.users.change_username.new_username" | |
| ), | |
| confirmText: this.hass.localize( | |
| "ui.panel.config.users.change_username.change" | |
| ), | |
| title: this.hass.localize( | |
| "ui.panel.config.users.change_username.caption" | |
| ), | |
| defaultValue: this._user?.username, | |
| }); | |
| if (newUsername) { | |
| try { | |
| await adminChangeUsername(this.hass, this._user.id, newUsername); | |
| this._params?.refreshUsers(); | |
| this._user = { ...this._user, username: newUsername }; | |
| showAlertDialog(this, { | |
| text: this.hass.localize( | |
| "ui.panel.config.users.change_username.username_changed" | |
| ), | |
| }); | |
| } catch (err: any) { | |
| showAlertDialog(this, { | |
| title: this.hass.localize( | |
| "ui.panel.config.users.change_username.failed" | |
| ), | |
| text: err.message, | |
| }); | |
| } | |
| } | |
| } |
Tools
Biome
[error] 392-392: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 404-404: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
|
Merging, as the parent PR has been merged. |
Proposed change
For: home-assistant/core#109057
Also saved the userid on the person when the user is created, to avoid weird UX.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
UI Enhancements
Translations