Skip to content

NAS-144563 / 28.0.0-BETA.1 / Add E2E coverage for importing, editing, viewing and deleting a certificate - #14465

Merged
truenas-agent-developer merged 2 commits into
masterfrom
14457-import-certificate
Oct 16, 2026
Merged

truenas-agent-developer merged 2 commits into
masterfrom
14457-import-certificate

Conversation

@truenas-agent-developer

Copy link
Copy Markdown
Collaborator

Fixes #14457

Adds e2e/tests/certificates-import.e2e.ts. It covers the Certificates card on Credentials > Certificates: importing a certificate, editing it, viewing it and deleting it. CSR creation and deletion were already covered in certificates.e2e.ts.

The issue title says "the user details page". The body is the spec, so this PR follows the body; triage made the same call for the whole batch (#14454 to #14460).

What it covers

  • Import: fills Name, Certificate and Private Key in the Import Certificate panel and saves. The card's Name and CN cells must show the certificate, and the CN is parsed out of the PEM by middleware. certificate.query must hold the certificate and the key as pasted, cert_type_CSR: false, and add_to_trusted_store: false.
  • Edit from the row: renames a certificate and ticks "Add to trusted store". The record is read back by id, so the test proves an update happened rather than a delete plus an import. A bystander certificate has to come through unchanged.
  • View Certificate / View Key: opens both from the edit panel's Actions menu. The dialog has to show the imported certificate PEM and then the imported key PEM. Both items open the same dialog, so its content is the only thing that shows which one was asked for.
  • Delete from the row: goes through the shared confirm. The test polls the API until the target is gone, and checks that the bystander's row and record are still there.

Every test fails at the start if its rows would not fit on the card, which lists four certificates before its pager. This is the same check the CSR tests use.

The test certificate

e2e/fixtures/test-certificate.ts holds a fixed, test-only self-signed certificate and key, as the ticket asks. It is RSA 2048 with SHA256, CN and SAN e2e-import.example.com, CA:FALSE, valid from 2026-01-01 to 2076-01-01. It was generated with Python's cryptography and is not a credential for anything. The key is unencrypted PKCS#8, which is the form middleware re-exports an imported key in (truenas_crypto_utils.key.export_private_key). So the key the appliance hands back is the imported one plus a trailing newline, and the spec compares PEM after trimming whitespace. Middleware checks only that certificate names are unique (plugins/certificate/crud.py), so the target and the bystander can both carry the same PEM.

Testing

  • Nothing ran locally. This host has no Node toolchain and no browser, so lint, e2e:typecheck and the suite will all run first in CI. I checked the code by eye against the lint rules that have failed CI here before, and checked line length with a script.
  • Typecheck caveat: privatekey and add_to_trusted_store are on the app's Certificate interface. I could not confirm them against the generated certificate.query type, because the api-client is not on disk here.
  • The local review fell back to a subagent applying the rubric, because the shared reviewer cannot run on this host. That is weaker evidence than the required check. The subagent checked every data-test id against the templates and found nothing at MEDIUM or above. It also re-parsed the checked-in PEM and confirmed that the key matches the certificate.
  • The PR touches e2e/**, so the E2E workflow runs on it.

What I did not change, and why

  • One LOW from the local review: certificateLocators.importForm.addToTrustedStore is unused, because the import test leaves the flag at its default and the edit test sets it. I kept it so the import form's locators are complete, the way the other forms' are.
  • deleteCertificate repeats the shape of deleteCsr. The two cards raise the same confirm, but each has its own locators, so I kept them as separate flows.

Not covered: importing with a passphrase, the row's Download action, and what the trusted store does to the system once the flag is on.

🤖 Generated with Claude Code

@bugclerk bugclerk changed the title Add E2E coverage for importing, editing, viewing and deleting a certificate NAS-144563 / 28.0.0-BETA.1 / Add E2E coverage for importing, editing, viewing and deleting a certificate Oct 16, 2026
@bugclerk

Copy link
Copy Markdown
Contributor

/** The Import Certificate side panel. */
importForm: {
name: '[data-test="input-name"]',
addToTrustedStore: '[data-test="checkbox-add-to-trusted-store"]',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOW — importForm.addToTrustedStore isn't used anywhere: the import test leaves the flag at its default, and only editForm.addToTrustedStore gets .check()ed. The PR description says this is on purpose. Two cheap ways to make it earn its place: have the import test assert the box starts unchecked (await expect(page.locator(importForm.addToTrustedStore)).not.toBeChecked()), or drop it until a test needs it. 🙂

@github-actions

Copy link
Copy Markdown
Contributor

One finding: one LOW. No human review needed. 🎉

I checked every new locator against the templates and they all match: the cert-<name> row tag, the Name/CN cell ids, the kebab row-menu actions, the import-certificate footer button, the certificate-actions footer menu with view-certificate-or-csr / view-key, the view-certificate dialog shell with its textarea-certificate and close, and the hideCheckbox: true delete confirm. Nothing else on the page uses the dialog's textarea-certificate id while the edit panel is open, so viewDialog.pem won't hit a strict-mode collision.

The tests are well built ✨ The edit is read back by id so a delete plus re-import can't pass for an update, a bystander certificate runs beside both edit and delete, the delete is polled through the API, and the card-capacity guard fails early with a reason.

  • LOW — certificateLocators.importForm.addToTrustedStore is unused (inline comment). Either assert its default in the import test or drop it.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved. 1 finding(s): 1 LOW.

Nothing blocks and no human review is needed.

@codecov

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.63%. Comparing base (070d444) to head (6bb9bf1).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14465      +/-   ##
==========================================
- Coverage   89.63%   89.63%   -0.01%     
==========================================
  Files        1939     1939              
  Lines       71551    71551              
  Branches     9372     9372              
==========================================
- Hits        64137    64135       -2     
- Misses       7414     7416       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@truenas-agent-developer
truenas-agent-developer merged commit f0afc88 into master Oct 16, 2026
18 of 19 checks passed
@truenas-agent-developer
truenas-agent-developer deleted the 14457-import-certificate branch October 16, 2026 11:43
@bugclerk

Copy link
Copy Markdown
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Oct 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add E2E coverage for the user details page

2 participants