Skip to content

Commit

Permalink
feat(web): request a root authentication method after selecting a pro…
Browse files Browse the repository at this point in the history
…duct (#1787)

## Problem

To ensure privileges in the installed systems, users must define at
least one authentication method for the `root` user. This can easily be
accomplished by visiting the "Users" page. However, since it is not
fully mandatory right now, users might overlook it and end up with an
unmanageable system.

## Solution

Makes a root authentication method mandatory in Agama web interface by
displaying the form for setting at least one of them as soon as it
detects there is none. I.e., at least once after selecting a product for
the first time.

Users can still change their chosen method or value later, but they
always must provide at least one to
ensure the installed system has an administrative user properly set.

## Testing

- Tested manually
- Added unit tests

---

Related to https://trello.com/c/YdZIRHoo (internal link)
  • Loading branch information
dgdavid authored Dec 2, 2024
2 parents f83615d + 95e50c6 commit 1376214
Show file tree
Hide file tree
Showing 29 changed files with 417 additions and 43 deletions.
8 changes: 4 additions & 4 deletions rust/agama-lib/share/profile.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -382,11 +382,11 @@
"examples": ["jane.doe"]
},
"password": {
"title": "User password (plain text or encrypted depending on the \"passwordEncrypted\" field)",
"title": "User password (plain text or encrypted depending on the \"encryptedPassword\" field)",
"type": "string",
"examples": ["nots3cr3t"]
},
"passwordEncrypted": {
"encryptedPassword": {
"title": "Flag for encrypted password (true) or plain text password (false or not defined)",
"type": "boolean"
}
Expand All @@ -399,10 +399,10 @@
"additionalProperties": false,
"properties": {
"password": {
"title": "Root password (plain text or encrypted depending on the \"passwordEncrypted\" field)",
"title": "Root password (plain text or encrypted depending on the \"encryptedPassword\" field)",
"type": "string"
},
"passwordEncrypted": {
"encryptedPassword": {
"title": "Flag for encrypted password (true) or plain text password (false or not defined)",
"type": "boolean"
},
Expand Down
4 changes: 2 additions & 2 deletions rust/agama-lib/src/users/http_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl UsersHTTPClient {
let rps = RootPatchSettings {
sshkey: None,
password: Some(value.to_owned()),
password_encrypted: Some(encrypted),
encrypted_password: Some(encrypted),
};
let ret = self.client.patch("/users/root", &rps).await?;
Ok(ret)
Expand All @@ -84,7 +84,7 @@ impl UsersHTTPClient {
let rps = RootPatchSettings {
sshkey: Some(value.to_owned()),
password: None,
password_encrypted: None,
encrypted_password: None,
};
let ret = self.client.patch("/users/root", &rps).await?;
Ok(ret)
Expand Down
2 changes: 1 addition & 1 deletion rust/agama-lib/src/users/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,5 @@ pub struct RootPatchSettings {
/// empty string here means remove password for root
pub password: Option<String>,
/// specify if patched password is provided in encrypted form
pub password_encrypted: Option<bool>,
pub encrypted_password: Option<bool>,
}
4 changes: 2 additions & 2 deletions rust/agama-lib/src/users/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,14 @@ mod test {
when.method(PATCH)
.path("/api/users/root")
.header("content-type", "application/json")
.body(r#"{"sshkey":null,"password":"1234","passwordEncrypted":false}"#);
.body(r#"{"sshkey":null,"password":"1234","encryptedPassword":false}"#);
then.status(200).body("0");
});
let root_mock2 = server.mock(|when, then| {
when.method(PATCH)
.path("/api/users/root")
.header("content-type", "application/json")
.body(r#"{"sshkey":"keykeykey","password":null,"passwordEncrypted":null}"#);
.body(r#"{"sshkey":"keykeykey","password":null,"encryptedPassword":null}"#);
then.status(200).body("0");
});
let url = server.url("/api");
Expand Down
2 changes: 1 addition & 1 deletion rust/agama-server/src/users/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ async fn patch_root(
} else {
state
.users
.set_root_password(&password, config.password_encrypted == Some(true))
.set_root_password(&password, config.encrypted_password == Some(true))
.await?
}
}
Expand Down
6 changes: 6 additions & 0 deletions rust/package/agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Sun Dec 1 21:53:21 UTC 2024 - David Diaz <[email protected]>

- Rename flag to set password as encrypted
(gh#agama-project/agama#1787).

-------------------------------------------------------------------
Fri Nov 29 12:14:25 UTC 2024 - Imobach Gonzalez Sosa <[email protected]>

Expand Down
2 changes: 1 addition & 1 deletion service/lib/agama/autoyast/root_reader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def read
return {} unless root_user

hsh = { "password" => root_user.password.value.to_s }
hsh["passwordEncrypted"] = true if root_user.password.value.encrypted?
hsh["encryptedPassword"] = true if root_user.password.value.encrypted?

public_key = root_user.authorized_keys.first
hsh["sshPublicKey"] = public_key if public_key
Expand Down
2 changes: 1 addition & 1 deletion service/lib/agama/autoyast/user_reader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def read
"password" => user.password.value.to_s
}

hsh["passwordEncrypted"] = true if user.password.value.encrypted?
hsh["encryptedPassword"] = true if user.password.value.encrypted?

{ "user" => hsh }
end
Expand Down
6 changes: 6 additions & 0 deletions service/package/rubygem-agama-yast.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Sun Dec 1 21:59:11 UTC 2024 - David Diaz <[email protected]>

- Rename flag to set password as encrypted
(gh#agama-project/agama#1787).

-------------------------------------------------------------------
Fri Nov 15 16:48:44 UTC 2024 - Ladislav Slezák <[email protected]>

Expand Down
6 changes: 6 additions & 0 deletions web/package/agama-web-ui.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Thu Nov 28 14:34:49 UTC 2024 - David Diaz <[email protected]>

- Request a root authentication method after selecting a product
(gh#agama-project#agama#1787).

-------------------------------------------------------------------
Tue Nov 26 09:30:09 UTC 2024 - Ladislav Slezák <[email protected]>

Expand Down
53 changes: 47 additions & 6 deletions web/src/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import App from "./App";
import { InstallationPhase } from "./types/status";
import { createClient } from "~/client";
import { Product } from "./types/software";
import { RootUser } from "./types/users";

jest.mock("~/client");

Expand All @@ -45,6 +46,7 @@ const microos: Product = { id: "Leap Micro", name: "openSUSE Micro" };
// list of available products
let mockProducts: Product[];
let mockSelectedProduct: Product;
let mockRootUser: RootUser;

jest.mock("~/queries/software", () => ({
...jest.requireActual("~/queries/software"),
Expand All @@ -65,14 +67,19 @@ jest.mock("~/queries/l10n", () => ({
jest.mock("~/queries/issues", () => ({
...jest.requireActual("~/queries/issues"),
useIssuesChanges: () => jest.fn(),
useAllIssues: () => ({ isEmtpy: true }),
useAllIssues: () => ({ isEmpty: true }),
}));

jest.mock("~/queries/storage", () => ({
...jest.requireActual("~/queries/storage"),
useDeprecatedChanges: () => jest.fn(),
}));

jest.mock("~/queries/users", () => ({
...jest.requireActual("~/queries/storage"),
useRootUser: () => mockRootUser,
}));

const mockClientStatus = {
phase: InstallationPhase.Startup,
isBusy: true,
Expand Down Expand Up @@ -104,6 +111,7 @@ describe("App", () => {
});

mockProducts = [tumbleweed, microos];
mockRootUser = { password: true, encryptedPassword: false, sshkey: "FAKE-SSH-KEY" };
});

afterEach(() => {
Expand Down Expand Up @@ -156,14 +164,47 @@ describe("App", () => {
mockClientStatus.isBusy = false;
});

it("renders the application content", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Outlet Content/);
describe("when there are no authentication method for root user", () => {
beforeEach(() => {
mockRootUser = { password: false, encryptedPassword: false, sshkey: "" };
});

it("redirects to root user edition", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText("Navigating to /users/root/edit");
});
});

describe("when only root password is set", () => {
beforeEach(() => {
mockRootUser = { password: true, encryptedPassword: false, sshkey: "" };
});
it("renders the application content", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Outlet Content/);
});
});

describe("when only root SSH public key is set", () => {
beforeEach(() => {
mockRootUser = { password: false, encryptedPassword: false, sshkey: "FAKE-SSH-KEY" };
});
it("renders the application content", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Outlet Content/);
});
});

describe("when root password and SSH public key are set", () => {
it("renders the application content", async () => {
installerRender(<App />, { withL10n: true });
await screen.findByText(/Outlet Content/);
});
});
});
});

describe("on the busy installaiton phase", () => {
describe("on the busy installation phase", () => {
beforeEach(() => {
mockClientStatus.phase = InstallationPhase.Install;
mockClientStatus.isBusy = true;
Expand All @@ -176,7 +217,7 @@ describe("App", () => {
});
});

describe("on the idle installaiton phase", () => {
describe("on the idle installation phase", () => {
beforeEach(() => {
mockClientStatus.phase = InstallationPhase.Install;
mockClientStatus.isBusy = false;
Expand Down
15 changes: 14 additions & 1 deletion web/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ import { useL10nConfigChanges } from "~/queries/l10n";
import { useIssuesChanges } from "~/queries/issues";
import { useInstallerStatus, useInstallerStatusChanges } from "~/queries/status";
import { useDeprecatedChanges } from "~/queries/storage";
import { ROOT, PRODUCT } from "~/routes/paths";
import { useRootUser } from "~/queries/users";
import { ROOT, PRODUCT, USER } from "~/routes/paths";
import { InstallationPhase } from "~/types/status";
import { isEmpty } from "~/utils";

/**
* Main application component.
Expand All @@ -44,6 +46,7 @@ function App() {
const { connected, error } = useInstallerClientStatus();
const { selectedProduct, products } = useProduct();
const { language } = useInstallerL10n();
const { password: isRootPasswordDefined, sshkey: rootSSHKey } = useRootUser();
useL10nConfigChanges();
useProductChanges();
useIssuesChanges();
Expand Down Expand Up @@ -84,6 +87,16 @@ function App() {
return <Navigate to={PRODUCT.progress} />;
}

if (
phase === InstallationPhase.Config &&
!isBusy &&
!isRootPasswordDefined &&
isEmpty(rootSSHKey) &&
location.pathname !== USER.rootUser.edit
) {
return <Navigate to={USER.rootUser.edit} state={{ from: location.pathname }} />;
}

return <Outlet />;
};

Expand Down
6 changes: 6 additions & 0 deletions web/src/assets/styles/app.scss
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,9 @@ button.remove-link:hover {
--pf-v5-c-notification-drawer__list-item--before--BackgroundColor: none;
}
}

form#rootAuthMethods {
.pf-v5-c-file-upload__file-select {
display: none;
}
}
3 changes: 2 additions & 1 deletion web/src/components/core/InstallButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { screen, waitFor, within } from "@testing-library/react";
import { installerRender, mockRoutes } from "~/test-utils";
import { InstallButton } from "~/components/core";
import { IssuesList } from "~/types/issues";
import { PRODUCT, ROOT } from "~/routes/paths";
import { PRODUCT, ROOT, USER } from "~/routes/paths";

const mockStartInstallationFn = jest.fn();
let mockIssuesList: IssuesList;
Expand Down Expand Up @@ -116,6 +116,7 @@ describe("InstallButton", () => {
["product selection progress", PRODUCT.progress],
["installation progress", ROOT.installationProgress],
["installation finished", ROOT.installationFinished],
["root authentication", USER.rootUser.edit],
])(`but the installer is rendering the %s screen`, (_, path) => {
beforeEach(() => {
mockRoutes(path);
Expand Down
8 changes: 5 additions & 3 deletions web/src/components/core/InstallButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,25 @@ import { Popup } from "~/components/core";
import { startInstallation } from "~/api/manager";
import { useAllIssues } from "~/queries/issues";
import { useLocation } from "react-router-dom";
import { PRODUCT, ROOT } from "~/routes/paths";
import { PRODUCT, ROOT, USER } from "~/routes/paths";
import { _ } from "~/i18n";
import { Icon } from "../layout";

/**
* List of paths where the InstallButton must not be shown.
*
* Apart from obvious login and installation paths, it does not make sense to
* show the button neither, when the user is about to change the product nor
* when the installer is setting the chosen product.
* show the button neither, when the user is about to change the product,
* defining the root authentication for the fisrt time, nor when the installer
* is setting the chosen product.
* */
const EXCLUDED_FROM = [
ROOT.login,
PRODUCT.changeProduct,
PRODUCT.progress,
ROOT.installationProgress,
ROOT.installationFinished,
USER.rootUser.edit,
];

const InstallConfirmationPopup = ({ onAccept, onClose }) => {
Expand Down
12 changes: 6 additions & 6 deletions web/src/components/core/Page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ type SectionProps = {
value?: React.ReactNode;
/** Elements to be rendered in the section footer */
actions?: React.ReactNode;
/** As short as possible yet as much as needed text for describing what the section is about, if needed */
description?: string;
/** A React node with a brief description of what the section is for */
description?: React.ReactNode;
/** The heading level used for the section title */
headerLevel?: TitleProps["headingLevel"];
/** Props to influence PF/Card component wrapping the section */
Expand Down Expand Up @@ -106,7 +106,7 @@ const Header = ({ hasGutter = true, children, ...props }) => {
*
* @example <caption>Simple usage</caption>
* <Page.Section>
* <EncryptionSummmary
* <EncryptionSummary />
* </Page.Section>
*
* @example <caption>Complex usage</caption>
Expand Down Expand Up @@ -137,7 +137,7 @@ const Section = ({
const hasTitle = !isEmpty(title);
const hasValue = !isEmpty(value);
const hasDescription = !isEmpty(description);
const hasHeader = hasTitle || hasValue;
const hasHeader = hasTitle || hasValue || hasDescription;
const hasAriaLabel =
!isEmpty(ariaLabel) || (isObject(pfCardProps) && "aria-label" in pfCardProps);
const props = { ...defaultCardProps, "aria-label": ariaLabel };
Expand Down Expand Up @@ -184,7 +184,7 @@ const Section = ({
*
* @example
* <Page.Actions>
* <Page.Action onCick={doSomething}>Let's go</Page.Action>
* <Page.Action onClick={doSomething}>Let's go</Page.Action>
* </Page.Actions>
*
*/
Expand Down Expand Up @@ -285,7 +285,7 @@ const Content = ({ children, ...pageSectionProps }: React.PropsWithChildren<Page
);

/**
* Component for structuing an Agama page, built on top of PF/Page/PageGroup.
* Component for structuring an Agama page, built on top of PF/Page/PageGroup.
*
* @see [Patternfly Page/PageGroup](https://www.patternfly.org/components/page#pagegroup)
*
Expand Down
Loading

0 comments on commit 1376214

Please sign in to comment.