Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate user account value #5305

Closed
wants to merge 3 commits into from
Closed

Validate user account value #5305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
063065b
webui: add test for user account creating
rvykydal Nov 8, 2023
1720d67
webui: add user account validation to accounts screen
rvykydal Nov 9, 2023
63fc98b
webui: update pixel test references for user account validation
rvykydal Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 65 additions & 4 deletions ui/webui/src/components/users/Accounts.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@

import cockpit from "cockpit";
import React, { useState, useEffect } from "react";
import { debounce } from "throttle-debounce";

import {
Form,
FormGroup,
FormHelperText,
HelperText,
TextInput,
Title,
} from "@patternfly/react-core";
Expand Down Expand Up @@ -55,6 +58,33 @@ export const accountsToDbusUsers = (accounts) => {
}];
};

const reservedNames = [
"root",
"bin",
"daemon",
"adm",
"lp",
"sync",
"shutdown",
"halt",
"mail",
"operator",
"games",
"ftp",
"nobody",
"home",
"system",
];

const isUserAccountWithInvalidCharacters = (userAccount) => {
return (
userAccount === "." ||
userAccount === ".." ||
userAccount.match(/^[0-9]+$/) ||
!userAccount.match(/^[A-Za-z0-9._][A-Za-z0-9._-]{0,30}([A-Za-z0-9._-]|\$)?$/)
);
};

const CreateAccount = ({
idPrefix,
passwordPolicy,
Expand All @@ -63,14 +93,39 @@ const CreateAccount = ({
setAccounts,
}) => {
const [fullName, setFullName] = useState(accounts.fullName);
const [_userAccount, _setUserAccount] = useState(accounts.userAccount);
const [userAccount, setUserAccount] = useState(accounts.userAccount);
const [userAccountInvalidHint, setUserAccountInvalidHint] = useState("");
const [isUserAccountValid, setIsUserAccountValid] = useState(null);
const [password, setPassword] = useState(accounts.password);
const [confirmPassword, setConfirmPassword] = useState(accounts.confirmPassword);
const [isPasswordValid, setIsPasswordValid] = useState(false);

useEffect(() => {
setIsUserValid(isPasswordValid && userAccount.length > 0);
}, [setIsUserValid, isPasswordValid, userAccount]);
debounce(300, () => setUserAccount(_userAccount))();
}, [_userAccount, setUserAccount]);

useEffect(() => {
setIsUserValid(isPasswordValid && isUserAccountValid);
}, [setIsUserValid, isPasswordValid, isUserAccountValid]);

useEffect(() => {
let valid = true;
setUserAccountInvalidHint("");
if (userAccount.length === 0) {
valid = null;
} else if (userAccount.length > 32) {
valid = false;
setUserAccountInvalidHint(_("The user name is too long"));
} else if (reservedNames.includes(userAccount)) {
valid = false;
setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));
} else if (isUserAccountWithInvalidCharacters(userAccount)) {
valid = false;
setUserAccountInvalidHint(cockpit.format(_("The user name should usually only consist of lower case letters from a-z, digits and the following characters: $0"), "-_"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a hint for the future, it makes sense to add new validation methods to the DBus API, because ideally, we want all our user interfaces to use a similar logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I will create a Jira task for this improvement suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://issues.redhat.com/browse/INSTALLER-3783

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I was thinking about it as well, as a follow-up.
Hopefully with debouncing it won't have performance issues.

}
setIsUserAccountValid(valid);
}, [userAccount]);

const passphraseForm = (
<PasswordFormFields
Expand Down Expand Up @@ -119,9 +174,15 @@ const CreateAccount = ({
>
<TextInput
id={idPrefix + "-user-account"}
value={userAccount}
onChange={(_event, val) => setUserAccount(val)}
value={_userAccount}
onChange={(_event, val) => _setUserAccount(val)}
validated={isUserAccountValid === null ? "default" : isUserAccountValid ? "success" : "error"}
/>
<FormHelperText>
<HelperText component="ul" aria-live="polite" id={idPrefix + "-full-name-helper"}>
{userAccountInvalidHint}
</HelperText>
</FormHelperText>
</FormGroup>
{passphraseForm}
</Form>
Expand Down
147 changes: 145 additions & 2 deletions ui/webui/test/check-users
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import anacondalib

from installer import Installer
from users import Users, CREATE_ACCOUNT_ID_PREFIX, create_user
from testlib import nondestructive, test_main # pylint: disable=import-error
from password import Password
from review import Review
from users import Users, CREATE_ACCOUNT_ID_PREFIX, create_user, dbus_reset_users
from testlib import nondestructive, test_main, sit # pylint: disable=import-error

@nondestructive
class TestUsers(anacondalib.VirtInstallMachineCase):
Expand All @@ -31,6 +33,8 @@ class TestUsers(anacondalib.VirtInstallMachineCase):
i = Installer(b, self.machine)
u = Users(b, self.machine)

self.addCleanup(lambda: dbus_reset_users(self.machine))

i.open()

i.reach(i.steps.ACCOUNTS)
Expand All @@ -47,5 +51,144 @@ class TestUsers(anacondalib.VirtInstallMachineCase):
self.assertIn('"is-crypted" b false', users)
self.assertIn('"password" s "password"', users)

@staticmethod
def _test_user_account(user, installer, valid, invalid):
installer.check_next_disabled(disabled=False)

n_valid = len(valid) - 1
n_invalid = len(invalid) - 1

# Set valid and invalid value in turns and check the next button
for i in range(max(n_valid, n_invalid)):
i_valid = min(i, n_valid)
i_invalid = min(i, n_invalid)
user.set_user_account(invalid[i_invalid])
installer.check_next_disabled()
user.set_user_account(valid[i_valid])
installer.check_next_disabled(disabled=False)

def testAccessAndValidation(self):
b = self.browser
i = Installer(b, self.machine)
p = Password(b, CREATE_ACCOUNT_ID_PREFIX)
u = Users(b, self.machine)

self.addCleanup(lambda: dbus_reset_users(self.machine))

i.open()

i.reach(i.steps.ACCOUNTS)

# Fill in valid values
password = "password"
full_name = "Full Tester"
user_account = "tester"

p.set_password(password)
p.set_password_confirm(password)
u.set_full_name(full_name)
u.set_user_account(user_account)

p.check_password(password)
p.check_password_confirm(password)
u.check_full_name(full_name)
u.check_user_account(user_account)

# Test that you can move forward and going back keeps values
i.next()
i.back()
p.check_password(password)
p.check_password_confirm(password)
u.check_full_name(full_name)
u.check_user_account(user_account)

# Test that moving back and forward keeps values
i.back()
i.next()
p.check_password(password)
p.check_password_confirm(password)
u.check_full_name(full_name)
u.check_user_account(user_account)

# Test full name validation
u.set_full_name("")
i.check_next_disabled(disabled=False)
u.set_full_name(full_name)

# Test account validation
# FIXME this should be tested by unit tests?
invalid_user_accounts = [
"",
# reserved
"root", "system", "daemon", "home",
"33333",
".",
"..",
"-tester",
"123",
"$",
"$tester",
"tester?",
"longer_than_32_characteeeeeeeeeeeeeeeeeeers",
]
valid_user_accounts = [
"tester-",
"...",
"12.3",
"tester1",
"tester$",
"test-er",
"test_er",
"test.er",
"_tester",
".tester",
"_",
]
self._test_user_account(u, i, valid_user_accounts, invalid_user_accounts)
u.set_user_account(user_account)

# Test password validation
# No password set
p.set_password("")
p.set_password_confirm("")
p.check_pw_rule("length", "indeterminate")
p.check_pw_rule("match", "indeterminate")
i.check_next_disabled()
# Start which pw which is too short
p.set_password("abcd")
p.check_pw_strength(None)
i.check_next_disabled()
p.check_pw_rule("length", "error")
p.check_pw_rule("match", "error")
# Make the pw 6 chars long
p.set_password("ef", append=True, value_check=False)
i.check_next_disabled()
p.check_password("abcdef")
p.check_pw_rule("length", "success")
p.check_pw_rule("match", "error")
p.check_pw_strength("weak")
# Set the password confirm
p.set_password_confirm("abcde")
p.check_pw_rule("match", "error")
p.set_password_confirm("abcdef")
p.check_pw_rule("match", "success")
p.check_pw_rule("length", "success")
p.check_pw_strength("weak")
i.check_next_disabled(disabled=False)

# Test password strength
p.set_password("Rwce82ybF7dXtCzFumanchu!!!!!!!!")
p.check_pw_strength("strong")

# Test review values
p.set_password(password)
p.set_password_confirm(password)
u.set_full_name(full_name)
u.set_user_account(user_account)
r = Review(b)
i.reach(i.steps.REVIEW)
r.check_account(f"{full_name} ({user_account})")


if __name__ == '__main__':
test_main()
15 changes: 15 additions & 0 deletions ui/webui/test/helpers/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,21 @@ def set_user_account(self, user_account, append=False, value_check=True):
sel = "#accounts-create-account-user-account"
self.browser.set_input_text(sel, user_account, append=append, value_check=value_check)

@log_step(snapshot_before=True)
def check_user_account(self, user_account):
sel = "#accounts-create-account-user-account"
self.browser.wait_val(sel, user_account)

@log_step(snapshot_before=True)
def set_full_name(self, full_name, append=False, value_check=True):
sel = "#accounts-create-account-full-name"
self.browser.set_input_text(sel, full_name, append=append, value_check=value_check)

@log_step(snapshot_before=True)
def check_full_name(self, full_name):
sel = "#accounts-create-account-full-name"
self.browser.wait_val(sel, full_name)


def create_user(browser, machine):
p = Password(browser, CREATE_ACCOUNT_ID_PREFIX)
Expand Down
2 changes: 1 addition & 1 deletion ui/webui/test/reference
Open in desktop
Submodule reference updated 1 files
+ − TestUsers-testBasic-users-step-basic-pixels.png