Skip to content

fix(ui): let admins edit users created through SAML - #7354

Merged
gustavosbarreto merged 1 commit into
masterfrom
fix/ui/edit-users-without-username
Oct 7, 2026
Merged

gustavosbarreto merged 1 commit into
masterfrom
fix/ui/edit-users-without-username

Conversation

@luizhf42

@luizhf42 luizhf42 commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Closes shellhub-io/team#278

Summary

Users created through SAML have an empty username, and the admin edit form refused every save for them: the schema ran validateUsername on the blank value, and the input's native required attribute blocked the submit before the schema ran.

 <EditUserModal user>
+  usernameRequired = !!user.username
-  userSchema("edit")
+  userSchema("edit", usernameRequired)    blank username skips validateUsername
   <UserFormFields
+    usernameRequired={usernameRequired}  drives the input's native required
   />
-  Change {user.username}'s details
+  Change {user.username || user.email}'s details

A typed username is still validated, and a user who has a username still cannot clear it. The payload sends username: "", which the cloud update endpoint keeps as is (shellhub-io/cloud#2631). MockModal now renders description so a test can see it.

The issue asked for a userSchema("edit") unit test. The tests sit at the EditUserModal level instead, because a schema test can't see the native required attribute. The e2e case waits for the SAML admin e2e specs.

Evidence

  • Before: in EditUserModal.test.tsx, "a user without a username" fails 3 tests:

    • saves an edit and keeps the username empty
    • does not mark the username input required
    • names the user by email in the description

    After: the file passes all 30 tests. The full console suite, lint (0 errors) and the build pass.

Merge Danger

Door: two-way

Blast Radius: admin

Only the admin user edit modal changes. Create mode is unchanged, because userSchema and UserFormFields default to requiring a username.

@luizhf42
luizhf42 requested a review from a team as a code owner October 6, 2026 20:00
@luizhf42
luizhf42 added this pull request to stack #7355 October 6, 2026 20:00
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 2m 55s —— View job


I'll analyze this and get back to you.

@luizhf42
luizhf42 removed this pull request from stack #7355 October 7, 2026 11:39
@luizhf42
luizhf42 changed the base branch from fix/server/users-without-username to fix/api/saml-users-without-username October 7, 2026 11:39
@luizhf42
luizhf42 requested a review from a team as a code owner October 7, 2026 11:39
@luizhf42
luizhf42 force-pushed the fix/ui/edit-users-without-username branch 2 times, most recently from a9e4f33 to 166eb60 Compare October 7, 2026 11:45
@luizhf42
luizhf42 changed the base branch from fix/api/saml-users-without-username to master October 7, 2026 11:45
@otavio

otavio commented Oct 7, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@luizhf42 luizhf42 self-assigned this Oct 7, 2026
@gustavosbarreto

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@luizhf42
luizhf42 marked this pull request as draft October 7, 2026 14:03
@luizhf42
luizhf42 marked this pull request as ready for review October 7, 2026 14:03
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude finished @luizhf42's task in 3m 23s —— View job


Code Review Complete

Reviewed 5 files. 2 inline issues posted. Standards: 1 violation, 0 judgement calls. Spec: did not run. Correctness: 1.

Standards

The new tests in EditUserModal.test.tsx:325-371 call the static userEvent API. react-conventions asks for userEvent.setup() once per test.

Correctness

In userSchema.ts:58, a username that is only spaces still runs validateUsername, which trims it and reports "Username is required". This blocks Save for a SAML user even though the field is optional. The fix is to compare v.username.trim().

Spec

The Spec axis did not run. The PR closes shellhub-io/team#278, which is in another repo, and this run cannot read it.

Additional notes

  • Backend: checked against the backend. Cloud's UserAdminUpdate validates Username with omitempty,username, so username: "" passes and the stored username stays empty, as the PR describes. CE has no admin update endpoint that could reject it.
  • Reopening the modal: useDrawerForm picks up the new schema before reset runs when the modal reopens for a different user.
  • Judgement calls: the review agents also raised four judgement calls, all low severity. None reached the bar for an inline comment, so none were posted.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Comment thread ui/apps/console/src/pages/admin/users/userSchema.ts Outdated
Comment thread ui/apps/console/src/pages/admin/users/__tests__/EditUserModal.test.tsx Outdated
Users created through SAML have an empty username, and the admin edit
form refused every save for them. The schema ran `validateUsername` on
the blank value, and the input's native `required` attribute blocked the
submit before the schema ran. The modal now derives `usernameRequired`
from the user being edited and passes it to both, so a blank username
is accepted only for a user who has none. A typed username is still
validated, and an existing one still cannot be cleared.

The update endpoint keeps the current username when the request sends
an empty one, so the payload's `username: ""` leaves the user as is.

The description named the user by username and read "Change 's details"
for these users. It falls back to the email. `MockModal` now renders
`description` so a test can see it.

Fixes: shellhub-io/team#278
@luizhf42
luizhf42 force-pushed the fix/ui/edit-users-without-username branch from 166eb60 to 0a4d755 Compare October 7, 2026 17:40
@gustavosbarreto

Copy link
Copy Markdown
Member

/review

1 similar comment
@luizhf42

luizhf42 commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude finished @luizhf42's task in 1m 6s —— View job


Code Review Complete

I reviewed 5 files for Standards (repo rules, React conventions, tests, code smells) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

The Spec axis did not run. The PR closes shellhub-io/team#278, which is in another repo, and this run cannot read it.

Both findings from the previous round are fixed in 0a4d755: the blank-username check now uses v.username.trim(), and the new tests call userEvent.setup().

@gustavosbarreto
gustavosbarreto merged commit 9a15de8 into master Oct 7, 2026
33 of 34 checks passed
@gustavosbarreto
gustavosbarreto deleted the fix/ui/edit-users-without-username branch October 7, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants