Skip to content

feat(web): rework users interface#1999

Merged
imobachgs merged 39 commits intostorage-config-uifrom
rework-users
Feb 18, 2025
Merged

feat(web): rework users interface#1999
imobachgs merged 39 commits intostorage-config-uifrom
rework-users

Conversation

@dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Feb 15, 2025

Apart from eliminating the step introduced in #1821 to force users to enter a password for root, this PR significantly revamps the interface of the Authentication page (formerly the Users page) to reduce its complexity and make interactions more intuitive.

Summarizing a lot, the key changes include improving the first user form and moving the root user authentication methods to a new form. This allows users to easily set or clear authentication methods without having to navigate complex actions. I.e., tables and most dynamic actions have been eliminated, making the page much easier to interact with and enabling users to quickly find the options they need.

Authentication page Root user form
localhost_8080_ (21) localhost_8080_ (22)

However, there are still areas for improvement:

  • Enhance validations and the underlying code that supports them.
  • Decouple the input with suggestions from the FirstUserForm (move it to the core namespace).
  • Use a simpler widget/input for adding public SSH keys. While the current one works, it is somewhat complex and not well-suited for these form’s use cases.
  • Enable the ability to add more than one public SSH key.

Note for reviewers

Warning

After commit c7c424c this PR needs changes done at #2005 (not merged at the time of writing).

Please jump into the branch and use the interface as part of the review process.

If interested in the details, check commits.


Related to https://trello.com/c/7ewFvR0X (internal link)

With the authentication model now defined, stop enforcing the root
password as a mandatory step that was hijacking the user workflow. This
reverts the changes introduced in
#1787 and
#1821.

Related to https://trello.com/c/7ewFvR0X (internal link)
Because it was actually not working and makes no sense.
For aligning items to "flex-start" to avoid inputs fields filling all
the avaible width. Most probably this is a workaround and there is a
better way to do it, but let's use it unitl discovering such an
approach.
Now it uses simpler layout which is better to follow.
Useful for displaying a set of actions collapsed in a menu while keeping
the main one visible.
Replace runtime checks with TypeScript discriminated unions to ensure
that both `href` and `onClick` can't be missing, but both can still be
provided.

Also, explicitly declare the `children` prop as required in the type
definition.
For better clarity and user experience.
While writing a test for FirstUserForm, it was found that the "Cancel"
form button should be a link instead, as it navigates to another route
rather than performing an action.

After changing it to a link using the `component="a"` PatternFly prop,
it was discovered that React Testing Library couldn't find the "Cancel"
link using `getByRole`. According to
https://www.w3.org/TR/html-aria/#el-area-no-href, a link without an
`href` has a "generic" implicit role instead of the "link" role. This is
why `getByRole` didn't work: Page.Cancel was rendering a link without
the `href` attribute, but relying on the `onClick` event for navigation.

This commit fixes the Page.Cancel by relying on core/Link component for
producing a link instead of a button AND use `href` for navigation,
making the link more semantically correct and ensuring it can be
properly queried in tests.

See testing-library/react-testing-library#1306
And refactor a bit the form itself by removing no longer needed code.
Still pending to extract the username suggestions logic to a more
generic, reusable "InputWithSuggestions" component.
To avoid it failing after change made in commit
a3be42d
Which still pending a unit test but has been tested manually to check it
properly mutates the root user information according with user input
through the form.
Simplify the interface by reducing interactions to a single "Edit"
action, leading users to a form where they can set either a password or
an SSH key. This approach is more streamlined compared to using a table
and popup dialogs, and aligns better with the first user section.
Which were used for setting the root user authentication methods via
popup dialogs.
Just a slightly better description and a different button variant.
Added basic unit tests for handling password and SSH key. Included
validation for empty password and SSH key fields.

There are still a few improvements to be made, including adding more
comprehensive tests to cover edge cases and updating the SSH key widget
to support multiple file uploads.
Use "isWidthLimited" and "maxWidth" forms props instead of forcing the
width via a CSS override.
Which also impacts the switch components. These can be revisited later
if the new look&feel doesn't feel appropriate.
@dgdavid dgdavid marked this pull request as ready for review February 17, 2025 09:51
For both, the menu item and the page title.
Making users aware that resource is using a hashed password and allowing
them to change it for a plain one instead.

Wording still need improvements.
Since #then and #catch methods return promises, their order is
important. This commit reverses them at FirstUserForm to prevent #then
from executing when it shouldn't and keeping errors visible for users to
fix before proceeding, rather than navigating away silently.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#chained_promises
)}
{changePassword && usingHashedPassword && (
<Content isEditorial>
{_("Using a hashed password.")}{" "}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice if someone finds a better wording for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could re-evaluate the wording of the whole section. But I don't feel inspired today so I suggest to merge as it is (specially since we need to build an image with all the functional changes ASAP) and think about it later.

After backend updates, defined authentication passwords are now exposed,
enabling more user-friendly forms. When editing previously created user
definitions, password fields will be prefilled with their values instead
of relying on switches to toggle password editing.

This improvement also helps users to check their previously entered
password, making the verification easier and avoiding the need to
re-enter it in case of doubt.
After backend updates, defined root passwords are now exposed, enabling
more user-friendly forms. When editing root authentication methods with
a previously set plain password fields will be prefilled with their
values instead of being empty.

In addition to fixing a form issue when editing the root user with an
already set plain password, this improvement also helps users to check
previously entered root password insted of forcing to re-enter it in
case of doubt.

It also fixes the order of #then and #catch in the mutation promise,
similar to the change made in the FirstUser form at
48858e7.
@dgdavid
Copy link
Contributor Author

dgdavid commented Feb 18, 2025

Warning

Please, note that after commit c7c424c this PR needs changes done at #2005 (not merged at the time of writing).

imobachgs and others added 6 commits February 18, 2025 08:25
It is not needed to rename the sshPublicKey after commit 6c796a3.

See 6c796a3
At IssuesDrawer component, to align it with the new name of the page.
To make it flow to the left, avoiding scrollbars appear and dissapear
according to its visibility state.
@ancorgs
Copy link
Contributor

ancorgs commented Feb 18, 2025

First couple of quick notes after an ever quicker manual test:

  • In the drawer with the issues it still says "users" instead of "Authentication"
  • We should take the opportunity to reword the issue with description "Defining a user, setting the root password or a SSH public key is required". That English is correct but quite hard to parse. I don't have a clear suggestion now but I will ask at IRC.

@dgdavid
Copy link
Contributor Author

dgdavid commented Feb 18, 2025

First couple of quick notes after an ever quicker manual test:

  • In the drawer with the issues it still says "users" instead of "Authentication"

Fixed at d695b6d

  • We should take the opportunity to reword the issue with description "Defining a user, setting the root password or a SSH public key is required". That English is correct but quite hard to parse. I don't have a clear suggestion now but I will ask at IRC.

For completeness, IIRC is something that comes from back end side.

@ancorgs

This comment was marked as resolved.

@dgdavid

This comment was marked as outdated.

@ancorgs

This comment was marked as outdated.

Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM. As mentioned, wording could be better at a couple of places but code-wise and functionality-wise this looks good. So let's merge.

## Problem

The users API does not expose the passwords. Initially, we implemented
like that for security
reasons, but giving that the password is send over an authenticated
HTTPS connection, we do not
think it is relevant. After all, you are sending/receiving the password
using the same channel.

Moreover, the D-Bus API is kind of inconsistent. The "first user" is
exposed as a single "object",
while the root data are separate D-Bus properties.

## Solution

- Make user/root passwords visible.
- Drop the autologin property, as it is not wanted anymore.

About the D-Bus API, this PR puts all the root information together
using a `RootUser` property.
Ideally, we should have a `SetRootUser` method too, but it will have to
wait a bit.

## Testing

- Added a new unit test
- Tested manually
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13388202322

Details

  • 25 of 51 (49.02%) changed or added relevant lines in 6 files are covered.
  • 72 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+47.4%) to 72.34%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rust/agama-server/src/users/web.rs 0 4 0.0%
service/lib/agama/dbus/users.rb 5 10 50.0%
rust/agama-lib/src/users/client.rs 0 17 0.0%
Files with Coverage Reduction New Missed Lines %
rust/agama-lib/src/users/client.rs 9 0.0%
rust/agama-server/src/users/web.rs 63 0.0%
Totals Coverage Status
Change from base Build 13334599182: 47.4%
Covered Lines: 19505
Relevant Lines: 26963

💛 - Coveralls

@imobachgs imobachgs merged commit cf854ce into storage-config-ui Feb 18, 2025
9 checks passed
@imobachgs imobachgs deleted the rework-users branch February 18, 2025 10:57
dgdavid added a commit that referenced this pull request Feb 18, 2025
These suggestions stop working at #1999 (specifically in commit
afb1a54) which started using a form internal state for keeping
input values instead of relying on the native Form API.

Sadly, there wasn't a test for the username suggestions and it was
broken silently.
dgdavid added a commit that referenced this pull request Feb 18, 2025
Add changes from #1999 and
#2015 to the web changes
file.
@imobachgs imobachgs mentioned this pull request Feb 26, 2025
imobachgs added a commit that referenced this pull request Feb 26, 2025
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.

4 participants