test(ui): cover licensing and the admin panel end to end by luizhf42 · Pull Request #7369 · shellhub-io/shellhub

luizhf42 · 2026-10-07T14:25:00Z

Covers the Playwright cases of Domain 21 (Licensing) and Domain 24 (Admin Panel) in shellhub-io/team#243, and fixes the 500 that the local-auth case ran into. Their [testcontainers] cases need an enterprise stack in qa.yml and are not part of this PR.

Summary

A password sign-in with local authentication turned off answered 500. ErrAuthMethodNotAllowed carries ErrCodeNotImplemented, and the converter had no case for that code:

 FromErrServiceToHTTPStatus(code)
   case ErrCodeConflict:       409
+  case ErrCodeNotImplemented: 501
   default:                    500

The mapping is shared, so cloud's SAML routes now answer 501 too while SAML is off, which is what the SAML sign-in spec already documented. AuthLocalUser also returned ErrAuthMethodNotAllowed when it could not read the system settings, so a database failure during password sign-in would have answered 501, which the API clients don't retry. It returns the store error now, and that case still answers 500. POST /api/login and the SAML re-auth path declare 501 now, because the test stacks validate responses.

New specs:

e2e/
├── license.spec.ts   enterprise: upload, no license, expired license, valid license on every guarded admin page
├── admin.spec.ts     "Login as User" opens the target's session in a new tab (enterprise, cloud)
│                     turning local auth off leaves only SSO and makes password sign-in answer 501 (enterprise)
├── helpers.ts        + required, moved from saml.ts
├── env.ts            requireEnv takes a hint for variables the stack does not set
└── seed.ts           + deleteLicenses

The expired license comes from the new TEST_EXPIRED_LICENSE secret, passed to Playwright as E2E_EXPIRED_LICENSE. The test fails without it, locally and in CI. #7375, stacked on this PR, replaces that secret and TEST_LICENSE with licenses the stack signs for each run.

Evidence

Before: TestAuthLocalUser "fails when local authentication is disabled" got 500.
After: it gets 501.
The AuthLocalUser service test for a failed SystemGet expects the store error instead of ErrAuthMethodNotAllowed.
e2e with the docker runner:
- enterprise license.spec.ts, with an expired license: 4 passed
- enterprise admin.spec.ts and saml.spec.ts: 13 passed
- cloud admin.spec.ts: 1 passed, 1 skipped (the local-auth test is enterprise only)
With LicenseGuard disabled, the no-license test fails on the redirect.

Merge Danger

Door: two-way

Blast Radius: small

API clients that sign in with a disabled method now get 501 instead of 500: local login with local auth off, and cloud's SAML routes with SAML off. Until #7375 merges, enterprise e2e in CI needs the TEST_EXPIRED_LICENSE secret, and PRs from forks, which get no secrets, fail that test.

github-actions · 2026-10-07T14:25:23Z

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

github-actions · 2026-10-07T17:17:30Z

Claude finished @luizhf42's task in 43s —— View job

Code Review

Gathered PR context
Reviewing (Standards, Spec, Correctness): three review agents are running now (repo rules, Go/React conventions, correctness). The Spec axis is skipped because the only issue cited, shellhub-io/team#243, is in a repo this run can't read.
Posting feedback

gustavosbarreto · 2026-10-07T18:15:53Z

/review

github-actions · 2026-10-07T18:16:15Z

Claude finished @gustavosbarreto's task in 3m 14s —— View job

Code Review Complete

Reviewed 15 files. 2 inline issues posted. Standards: 0 violations, 0 judgement calls. Spec: 0. Correctness: 2.

Standards

No findings. Comments, the commit messages, the e2e fixtures and the Go table test all follow the repo rules and the conventions skills.

Correctness

The worst defect is in server/api/pkg/echo/handlers/pkg/converter/converter.go:35-36. The new 501 mapping also covers AuthLocalUser's SystemGet failure path (server/api/services/auth.go:496). A database error during password sign-in now answers 501 ("method turned off"), and the API clients don't retry 501. The second finding is smaller: the 501.yaml example shows a message the server never sends, because respond replaces the body of every status of 500 or above with internal server error.

Spec

The Spec axis did not run. The only issue this PR cites is shellhub-io/team#243, and this run can't read that repo.

Additional notes

I checked one Standards finding and dropped it: the claim that deleteLicenses should require exactly DELETE 1. LicenseSave inserts a new row on every upload (cloud/pkg/store/pg/license.go:28), so several rows are expected, and the current at-least-one check is right.
No cap dropped any findings.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

luizhf42 · 2026-10-07T19:13:20Z

/review

github-actions · 2026-10-07T19:13:47Z

Claude finished @luizhf42's task in 5m 8s —— View job

Code Review Complete

Reviewed 17 files. 2 inline issues posted. Standards: 0 violations, 0 judgement calls. Spec: 0. Correctness: 2.

Standards

No findings. The previous round's fixes (b51367f) hold up, and the comments, commit messages, e2e helpers and Go table tests follow the repo rules and conventions skills.

Correctness

The worst defect is at ui/apps/console/e2e/license.spec.ts:57-64. The upload test sends the real CI license (secrets.TEST_LICENSE) through the browser. With trace: "retain-on-failure", a failed run records that license in the trace and uploads it as a public artifact. The second finding is at converter.go:35-36: the new 501 mapping also covers a transient store or config error in cloud's SupportSAML, so the cloud SAML routes answer 501 ("SSO off") when SAML is on but its config can't be read. The fix belongs in a companion cloud PR.

Spec

The Spec axis did not run. The only issue this PR cites is shellhub-io/team#243, and this run can't read that repo.

Additional notes

The agents found two low-severity judgement calls and I dropped both, since neither names a smell or a documented rule. One is that the afterEach in license.spec.ts has no guard for an unset adminToken. The other is that visitorContext in admin.spec.ts isn't closed in a finally.
I checked these and found no problems: test isolation (workers: 1, so DELETE FROM licenses can't race other specs), license restore (LicenseLoad reads the newest row, uncached), the teardown order in admin.spec.ts (local auth comes back on before SAML goes off), and secret exposure in the workflow (pull_request, not pull_request_target).
No cap dropped any findings.

To request another review round, comment /review.

luizhf42 · 2026-10-07T20:37:39Z

/review

github-actions · 2026-10-07T20:38:13Z

Claude finished @luizhf42's task in 5m 31s —— View job

Code Review Complete

Reviewed 17 files. 2 inline issues posted. Standards: 2 violations, 0 judgement calls. Spec: 0. Correctness: 0.

Standards

The worst violation is at server/api/services/auth.go:496-503. 74e0388 changed what AuthLocalUser returns: a SystemGet failure now comes back as the store error unchanged (500), and ErrAuthMethodNotAllowed now comes back only when local auth is off (501). Its doc comment (auth.go:41-51) still ends with "and an error, if any". code-style.md requires the comment to change in the commit that changes the behaviour, and to list every error. The second violation is in ui/apps/console/e2e/admin.spec.ts:106-111, which repeats expectLoginStatus from account.spec.ts. The e2e conventions say a helper moves to a shared module in e2e/ once a second spec needs it, and this is the second spec.

Spec

The Spec axis did not run. The only issue this PR cites is shellhub-io/team#243. This run can't read that repo, and no commit carries a Fixes: trailer.

Additional notes

Correctness found nothing new beyond earlier rounds. It checked these, and all hold:
- ErrCodeNotImplemented has one source, so nothing else in shellhub becomes 501.
- Store errors from SystemGet still answer a generic 500.
- Cloud's other NewErrAuthMethodNotAllowed user, admin SAMLMetadata with SAML off, moving from 500 to 501 fits "method turned off".
- E2E_EXPIRED_LICENSE reaches Playwright in both host and docker runner modes.
- deleteLicenses and composeExec pass constant arguments with no shell interpolation.
- The cleanup order in both specs is correct.
No cap dropped any findings.

To request another review round, comment /review.

luizhf42 · 2026-10-07T21:07:24Z

/review

github-actions · 2026-10-07T21:07:50Z

Claude finished @luizhf42's task in 4m 4s —— View job

Code Review Complete

Reviewed 19 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call. Spec: 0. Correctness: 0.

Standards

The only finding is a low-severity judgement call at server/api/services/errors.go:37-38. The new ErrCodeNotImplemented doc comment says the code is for a sign-in method "this instance has turned off". Cloud also returns it for SAML on an edition that never offered SAML, and from the SAMLMetadata route, which isn't a sign-in. Commit 9fc1894's own wording, "the instance does not offer the method", is more accurate.

Spec

The Spec axis did not run. The only issue this PR cites is shellhub-io/team#243. This run can't read that repo, and no commit carries a Fixes: trailer.

Additional notes

The fixes from the previous round hold:
- Every error the AuthLocalUser doc comment lists is returned, and none is missing.
- expectLoginStatus now lives in e2e/api.ts, and both specs import it.
Correctness found nothing new. It checked these, and all hold:
- POST /admin/api/license skips the license check, so the afterEach restore works after a delete or an expired upload.
- The expired-license message shows only after the 7-day grace period, which matches the E2E_EXPIRED_LICENSE hint.
- The admin pages' license cache lives only in memory, so each goto or reload sees the current license.
- No test in cloud's routes expects 500 for ErrAuthMethodNotAllowed.
- The guarded paths in the redirect check need no regex escaping.
No cap dropped any findings.

To request another review round, comment /review.

luizhf42 · 2026-10-07T21:15:08Z

/review

github-actions · 2026-10-07T21:16:45Z

Claude finished @luizhf42's task in 2m 15s —— View job

Code Review Complete

Reviewed 19 files on two axes: Standards (repo rules, language conventions, tests, code smells) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

The Spec axis did not run, because there is no issue to review against. The only issue the PR cites is shellhub-io/team#243, which this run can't read, and no commit has a Fixes: trailer.

The period-end cancellation test listed the subscription's draft invoices after the test clock read ready, and threw when there was none. When the test was written, the clock read ready with the final invoice still a draft, so the test finalized it to send `invoice.paid`. Since 2026-10-07 around 18:00 UTC the invoice no longer waits. The cloud e2e job failed this way on master (9a15de8) and on #7369 twice, with no billing change in shellhub or cloud since its last pass at 16:20. In the failed run Stripe sent `invoice.updated`, `invoice.finalized` and `invoice.paid` five seconds after creating the invoice, before `test_clock.ready`, and the server abandoned that `invoice.paid` as it should. Neither the server nor the pinned stripe-cli finalizes invoices, and Stripe's changelog does not mention it, so the cause is on Stripe's side and still unknown. The test now finalizes the invoice only when it is still a draft. The poll for the server's abandon line still fails the test when no `invoice.paid` arrives either way.

The license and admin specs need it too, and neither has anything to do with SAML.

`ErrAuthMethodNotAllowed` is the only error carrying `ErrCodeNotImplemented`, and `FromErrServiceToHTTPStatus` had no case for that code, so it fell through to 500. A password sign-in with local authentication turned off answered 500 instead of saying the method is off. The mapping is shared, so cloud's SAML routes now answer 501 too while SAML is disabled. The SAML sign-in spec already documented 501 for that case. 501 rather than 403 because nothing about the caller's permissions is wrong: the instance does not offer the method. `AuthLocalUser` also returned `ErrAuthMethodNotAllowed` when `SystemGet` failed, which would now answer 501 to a database outage. 501 says the method is off, and the API clients do not retry it, so a transient failure would look permanent. `AuthLocalUser` returns the `SystemGet` error as is now, and that error still answers 500. The login and SAML re-auth paths declare 501 now, since the test stacks validate responses and replace an undeclared status. The 501 example shows `internal server error`, because `respond` replaces the message of every status from 500 up. The admin SAML metadata route has no path in the spec.

The license is instance-wide and later specs on the same stack need a valid one, so `afterEach` uploads the license the stack mounts at `/etc/shellhub/license.dat` again. That file is the only valid license the stack has, and there is no endpoint to delete one. `deleteLicenses` accepts any non-zero count because every upload adds a row and the newest one wins. It throws on `DELETE 0`, since the stack always starts with a license. The upload test reloads after the delete so the page fetches the license again. The expired case needs a real license signed by the compiled-in key and more than 7 days past expiry, because inside the grace period the page shows a different message. It comes from the `TEST_EXPIRED_LICENSE` secret through `E2E_EXPIRED_LICENSE`, and the test fails without it, the same as the valid license the stack needs. Pull requests from forks get no secrets and fail there.

The server refuses to turn local authentication off while SAML is off, and to turn SAML off while local authentication is off. So the toggle test enables SAML first, and `afterEach` turns local authentication back on before disabling SAML. It restores with the admin's token, because `disableSaml` signs in with a password and would fail while local authentication is off. `afterEach` returns early when `beforeEach` failed before creating the admin, so the restore does not throw a second error that hides the first.

luizhf42 requested review from a team as code owners October 7, 2026 14:25