Allow spaces in user names and auto-generate login - #1158
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/components/modals/UserModals/AddUser.tsx" line_range="68" />
<code_context>
// useStates for TextInputs
const [userLogin, setUserLogin] = React.useState("");
+ const [loginTouched, setLoginTouched] = React.useState(false);
const [firstName, setFirstName] = React.useState("");
const [lastName, setLastName] = React.useState("");
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the loginTouched-based handler logic with a single hasCustomLogin flag, a useEffect-driven login generator, and extracted helper functions to simplify state and branching in this modal.
Using `loginTouched` plus three handlers does add avoidable complexity and splits the generation logic across multiple branches.
You can keep all behavior and simplify by:
1. **Use a single `hasCustomLogin` flag and centralize generation in `useEffect`.**
2. **Extract `generateLogin`/`sanitize` out of the component.**
### 1. Centralize login generation
Instead of recomputing login in each handler, track whether the user has overridden the login and recompute in one place:
```ts
// state
const [userLogin, setUserLogin] = React.useState("");
const [hasCustomLogin, setHasCustomLogin] = React.useState(false);
const [firstName, setFirstName] = React.useState("");
const [lastName, setLastName] = React.useState("");
// effect: auto-generate login from latest names when not customized
useEffect(() => {
if (!hasCustomLogin) {
setUserLogin(generateLogin(firstName, lastName));
}
}, [firstName, lastName, hasCustomLogin]);
```
Handlers then become straight updates with a single place for the generation logic:
```tsx
const handleFirstNameChange = (value: string) => {
setFirstName(value);
};
const handleLastNameChange = (value: string) => {
setLastName(value);
};
const handleLoginChange = (value: string) => {
setHasCustomLogin(value.length > 0);
setUserLogin(value);
};
```
This preserves:
- auto-generation when first/last change,
- user override when they type in login,
- clearing login (set `hasCustomLogin` false, effect re-applies generated login).
### 2. Extract sanitization/generation helpers
Move sanitization outside the component to reduce UI noise and keep behavior the same:
```ts
// utils/login.ts
export const sanitizeLoginPart = (s: string): string =>
s
.normalize("NFD")
.replace(/[\u0300-\u036f]/g, "")
.replace(/[^A-Za-z0-9._\-$]/g, "")
.toLowerCase();
export const generateLogin = (first: string, last: string): string => {
const sanitizedFirst = sanitizeLoginPart(first);
const sanitizedLast = sanitizeLoginPart(last);
if (!sanitizedFirst || !sanitizedLast) return "";
return sanitizedFirst.charAt(0) + sanitizedLast;
};
```
Then in the component:
```ts
import { generateLogin } from "./utils/login";
```
This keeps all functionality intact but reduces branching and “mental load” in the modal component.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3ae8d8d to
c5c5c3b
Compare
duzda
left a comment
duzda
left a comment
There was a problem hiding this comment.
Hi, please see notes, we should remove the unnecessary rules, honestly in this day and age, I wouldn't be surprised to see anyone with a ? in their name.
The API returns this JSON:
"takes_args": [
{
"cli_name": "login",
"deprecated_cli_aliases": [],
"label": "User login",
"doc": "User login",
"required": true,
"multivalue": false,
"primary_key": true,
"autofill": false,
"query": false,
"attribute": true,
"flags": [],
"alwaysask": false,
"sortorder": 2,
"cli_metavar": "STR",
"no_convert": false,
"deprecated": false,
"confirm": true,
"maxlength": 255,
"pattern_errmsg": "may only include letters, numbers, _, -, . and $, refer to 'ipa help user' for complete format description",
"pattern": "(?!^[0-9]+$)^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*[a-zA-Z0-9_.$-]?$",
"noextrawhitespace": true,
"class": "Str",
"name": "uid",
"type": "str"
}
],
"takes_options": [
{
"cli_name": "first",
"deprecated_cli_aliases": [],
"label": "First name",
"doc": "First name",
"required": true,
"multivalue": false,
"primary_key": false,
"autofill": false,
"query": false,
"attribute": true,
"flags": [],
"alwaysask": false,
"sortorder": 2,
"cli_metavar": "STR",
"no_convert": false,
"deprecated": false,
"confirm": true,
"noextrawhitespace": true,
"class": "Str",
"name": "givenname",
"type": "str"
},
{
"cli_name": "last",
"deprecated_cli_aliases": [],
"label": "Last name",
"doc": "Last name",
"required": true,
"multivalue": false,
"primary_key": false,
"autofill": false,
"query": false,
"attribute": true,
"flags": [],
"alwaysask": false,
"sortorder": 2,
"cli_metavar": "STR",
"no_convert": false,
"deprecated": false,
"confirm": true,
"noextrawhitespace": true,
"class": "Str",
"name": "sn",
"type": "str"
},No mention of any pattern for first and last name, the only pattern is for userLogin, which should be changed.
| * Generates a sanitized login from first + last name, | ||
| * matching FreeIPA's default: givenname[0] + sn (lowercased). | ||
| */ | ||
| export const generateLogin = (first: string, last: string): string => { |
There was a problem hiding this comment.
There's no need for this code, please don't generate login on the frontend, it will be handled by backend if you omit it.
There was a problem hiding this comment.
The issue is that the server's auto-generation (givenname[0] + sn) does not sanitize, it just concatenates as-is. So with sn="Rodriguez Perez", the server would try to create login mrodriguez perez, which then fails its own pattern validation because spaces aren't allowed in a uid.
Another example:
>> ipa user-add
First name: Anselmo
Last name: Luján Espósito
User login [aluján espósito]:
ipa: ERROR: invalid 'login': may only include letters, numbers, _, -, . and $, refer to 'ipa help user' for complete format description
The generateLogin function aims to fix that.
There was a problem hiding this comment.
Can you please raise an issue on the backend then?
| rules={[ | ||
| { | ||
| id: "ruleCharacters", | ||
| message: | ||
| "First name should not contain special characters or spaces", | ||
| validate: (v: string) => !formatWithoutSpaces.test(v), | ||
| message: "First name should not contain special characters", | ||
| validate: (v: string) => !nameInvalidChars.test(v), | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Please delete this code, all of it.
There was a problem hiding this comment.
I might need more context to understand why is this wrong.
There was a problem hiding this comment.
There is no pattern or first and last name, you can put anything you want there on the backend, we should follow that, I'm confident that people may have weird names, or use a placeholder.
| rules={[ | ||
| { | ||
| id: "ruleCharacters", | ||
| message: | ||
| "Last name should not contain special characters or spaces", | ||
| validate: (v: string) => !formatWithoutSpaces.test(v), | ||
| message: "Last name should not contain special characters", | ||
| validate: (v: string) => !nameInvalidChars.test(v), | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Please delete this code, all of it.
| @@ -87,7 +95,7 @@ const AddUser = (props: PropsToAddUser) => { | |||
| // User login: Valid characters in body (every char must be in set): letters, digits, '_', '-', '.', '$' | |||
| const userLoginFormatBody = /^[A-Za-z0-9._\-$]*$/; | |||
There was a problem hiding this comment.
The only pattern is for userLogin and it's this one: "(?!^[0-9]+$)^[a-zA-Z0-9_.][a-zA-Z0-9_.-]*[a-zA-Z0-9_.$-]?$"
There was a problem hiding this comment.
And it is being applied.
There was a problem hiding this comment.
Just read your main comment. Maybe I can replace this by the pattern provided in the metadata...
| const [addSpinning, setAddBtnSpinning] = React.useState<boolean>(false); | ||
|
|
||
| // Login is either the user-provided value or auto-generated from names | ||
| const userLogin = customLogin || generateLogin(firstName, lastName); |
There was a problem hiding this comment.
I believe this change is not needed?
There was a problem hiding this comment.
It is needed. This is because there is two ways to actually have a user name in the UI: either manually providing it itself (customLogin), or generated (generateLogin).
If we leave that field empty, 1) the ipa user-add command complains, because it is not able to generate a user name based on the data provided, and 2) it is possible from the UI to just "delete" the provided user name (generated or not), causing the problem mentioned here.
This is a compromise solution to ensure the user doesn't mess up with the UI.
There was a problem hiding this comment.
It would be nicer to put the generated login as placeholder into the input, instead of "typing" for the user.
There was a problem hiding this comment.
That sounds reasonable, will do...
c5c5c3b to
0c8d83a
Compare
I have changed the code to take the defined pattern for the user name. |
7b9ec97 to
a74e723
Compare
|
Hi, you still need to remove the patterns for first and last name, there are no patterns on the backend, let's follow that. |
The Add User modal previously rejected first/last names containing spaces (e.g., "Maria José", "Van den Berg"), which are valid values for the givenname/sn LDAP attributes in FreeIPA. This change: - Removes the whitespace restriction from name field validation, allowing composed names and multi-word surnames. - Auto-generates a sanitized user login (uid) from the first and last name as the user types, stripping diacritics, spaces, and other characters invalid for a login. This prevents the server-side ValidationError that occurred when no uid was provided and the auto-generated default contained spaces. - Allows the user to override the suggested login; clearing it re-triggers auto-generation from the current name values. Assisted-by: Claude <noreply@anthropic.com> Fixes: freeipa#1151 Signed-off-by: Carla Martinez <carlmart@redhat.com>
a74e723 to
4a9a0ca
Compare
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
LGTM, maybe mark somewhere that the generate login code is a temporary fix until the issue is fixed on the backend..
Just read your comment after merging the PR 🙈 But I can add it afterwards in any other PR 😅 |
3 participants
The Add User modal previously rejected first/last names containing spaces (e.g., "Maria José", "Van den Berg"), which are valid values for the givenname/sn LDAP attributes in FreeIPA.
This change:
Assisted-by: Claude noreply@anthropic.com
Fixes: #1151
Summary by Sourcery
Allow spaces in user first and last names in the Add User modal and auto-generate a sanitized login from those names, while still permitting manual override.
New Features:
Enhancements: