Skip to content

refactor(api,ssh,gateway,ui): replace edition booleans with SHELLHUB_EDITION enum#6666

Open
luizhf42 wants to merge 2 commits into
masterfrom
refactor/shellhub-edition-enum
Open

refactor(api,ssh,gateway,ui): replace edition booleans with SHELLHUB_EDITION enum#6666
luizhf42 wants to merge 2 commits into
masterfrom
refactor/shellhub-edition-enum

Conversation

@luizhf42

Copy link
Copy Markdown
Member

What

Replaces the SHELLHUB_ENTERPRISE and SHELLHUB_CLOUD boolean env vars with a single SHELLHUB_EDITION=community|enterprise|cloud enum, making the previously illegal fourth state (cloud=true, enterprise=false) unrepresentable by design.

Why

Two booleans encoded three editions but permitted a fourth, invalid combination guarded only by a runtime check in bin/docker-compose. IsEnterprise() meant "at least enterprise" (true on cloud too), and the same cloud || enterprise expression appeared under five different local names in the frontend. The build layer already used an enum (ARG EDITION=community|enterprise), so build-time and runtime representations were inconsistent.

Closes shellhub-io/team#156

Changes

  • pkg/envs: added Edition type (Community, Enterprise, Cloud) and CurrentEdition() that reads SHELLHUB_EDITION, normalizes (trim + lowercase), defaults to community, and hard-fails on invalid values. Reimplemented predicates: IsCommunity(), IsEnterprise() (now exact), IsCloud() (now exact), IsEnterpriseOrCloud() (new). Added envstest.SetEdition() and envstest.SetRawEdition() helpers that replace all call-order-coupled mock stubs across 35+ test blocks.
  • gateway/config.go: resolves SHELLHUB_EDITION once and derives EnableEnterprise/EnableCloud bools for the nginx template, which stays unchanged.
  • ssh/session: license+firewall check uses exact IsEnterprise(), asciinema saving uses IsEnterpriseOrCloud().
  • bin/docker-compose: resolves SHELLHUB_EDITION with validation (invalid value = hard exit), selects compose overlays and env files by edition. Deletes the cloud-requires-enterprise guard.
  • .env, .env.enterprise, docker-compose*.yml: two booleans collapsed to SHELLHUB_EDITION.
  • ui/apps/console/src/env.ts: ClientConfig.{enterprise, cloud} replaced with edition: Edition. Same four predicates as Go: isCommunity(), isEnterprise(), isCloud(), isEnterpriseOrCloud().
  • ui/apps/console/src/__mocks__/env.ts + test/setup.ts: manual mock with vi.fn()-backed getConfig, global vi.mock("@/env") in setup. Eliminates the 10-line vi.mock factory repeated in 29 test files.
  • ui/apps/console/src/utils/features.ts: deleted. isPremiumFeature() and hasMfaSupport() were thin wrappers; callers now import isEnterpriseOrCloud() directly.
  • Login.tsx SSO bug fix: isEnterprise() (exact match, excluded cloud) gated SAML SSO — changed to isEnterpriseOrCloud() so SAML works on cloud too.
  • docs: updated configuring.mdx and upgrading.mdx to reference SHELLHUB_EDITION.

Testing

  • Go pkg/envs tests cover normalization (trim, lowercase, invalid → error, empty → community) and all four predicates across all three editions.
  • Frontend: 183 test files, 2644 tests pass. Lint clean (0 errors).
  • tests/compose/wrapper.bats and tests/compose/with_cloud.bats updated and passing.
  • Companion cloud PR: https://github.com/shellhub-io/cloud/pull/2431
  • Manual smoke test across all three editions recommended — a checklist covering startup, login, sidebar, feature gates, SSH, admin panel, and nginx routes is available on request.

@luizhf42 luizhf42 requested review from a team as code owners July 15, 2026 18:50
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

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


Code Review Complete

Reviewed 84 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium).

This is a clean, well-executed refactor. The five reviewers independently confirmed the core risk — the IsEnterprise() semantic change from "at-least-enterprise" to exact enterprise — was handled correctly at every call site:

  • Every gate's truth table is preserved across community/enterprise/cloud. Enterprise-or-cloud callers moved to IsEnterpriseOrCloud(); the one remaining exact-IsEnterprise() (SSH license check) is intentionally exact. The documented firewall-on-cloud bug (ssh/session/session.go) is genuinely fixed, and no premium/paid feature is newly exposed to community.
  • Cross-repo (cloud/) is consistent — the companion checkout already uses SHELLHUB_EDITION and the new predicate semantics; no code there reads the retired env vars.
  • Test rework is sound — despite no global vi.resetAllMocks, every edition-overriding UI test resets getConfig in its own beforeEach, Vitest file isolation prevents cross-file leaks, and the Go envstest helpers restore DefaultBackend via t.Cleanup. No real coverage was dropped (deletions were redundant per-file vi.mock blocks).

Additional notes

  • CurrentEdition() panics lazily, not at startup, for ssh/cli (non-inline, architectural). CurrentEdition() now panics on an invalid value, but ssh and cli only evaluate it on first use — ssh/server/channels/utils.go:91 calls it inside a go func() with no defer recover(), so a genuinely invalid SHELLHUB_EDITION would crash the whole SSH process mid-session rather than failing at boot (the doc comment promises "fails at startup"). In practice bin/docker-compose rejects invalid values before containers start, so the supported deploy path is guarded — but direct/k8s deployments bypass that. Consider resolving the edition once in each binary's main(), or softening the doc comment to "on first evaluation."

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

Comment thread gateway/config.go Outdated
@luizhf42 luizhf42 self-assigned this Jul 15, 2026
@luizhf42 luizhf42 added area/ui area/api status/needs-review go Pull requests that update Go code area/docker area/docs product/cloud Issue/PR related to Cloud Edition release/breaking-change kind/refactor status/ready-for-testing docker Pull requests that update Docker code product/enterprise Issue/PR related to Enterprise Edition area/pkg and removed product/cloud Issue/PR related to Cloud Edition product/enterprise Issue/PR related to Enterprise Edition labels Jul 15, 2026
@luizhf42 luizhf42 force-pushed the refactor/shellhub-edition-enum branch from 35c7cfe to b4ad36a Compare July 15, 2026 19:15
@otavio

otavio commented Jul 15, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 9m 9s —— View job


Code Review Complete

Reviewed 84 files across code quality, security, testing, language patterns, and architecture. 1 inline issue posted (1 medium), plus 1 cross-repo note below.

Second pass on this refactor. The IsEnterprise() "at-least-enterprise" → exact semantic split remains correctly handled at every shellhub call site — the security and Go/TS-pattern reviewers re-verified each moved gate's truth table across community/enterprise/cloud (license check stays exact-enterprise, firewall/asciinema/recorder/SSO moved to IsEnterpriseOrCloud()), and the test rework is sound (envstest restores DefaultBackend via t.Cleanup; every edition-overriding UI test re-sets getConfig per test, no cross-test leak). The gateway normalization concern from the prior round is confirmed fixed in 3ec01bd.

Inline finding

  • ui/scripts/gen-config.sh:13 (medium) — embeds the raw SHELLHUB_EDITION into config.json without the lowercase/trim that pkg/envs, gateway/config.go, and bin/docker-compose all apply. Because the wrapper doesn't export the normalized value, a mis-cased value (Enterprise, " cloud ") reaches the UI container raw and every strict-equality predicate in env.ts falls through to community — the frontend counterpart of the gateway split already closed in 3ec01bd.

Additional notes

  • Cross-repo (cloud/) — likely a missed migration in companion PR #2431. cloud/internal/license/init.go:16 still gates the license-evaluator registration on if !envs.IsEnterprise(). Under this PR's new exact IsEnterprise(), that returns false on a SHELLHUB_EDITION=cloud instance, so RegisterLicenseEvaluator returns nil and the s.licenseEvaluator != nil device-acceptance check at api/services/device.go:538 is silently skipped on cloud — whereas previously (cloud set SHELLHUB_ENTERPRISE=true too) the evaluator was registered. Its siblings were correctly migrated to IsEnterpriseOrCloud() (cloud/internal/admin/init.go:19, cloud/internal/cloud/services/saml.go:39, cloud/internal/admin/services/system-auth.go:106), which is what flags this one as an oversight rather than intent. Worth confirming in the cloud PR that dropping the license evaluator on cloud is deliberate; if not, change the guard to !envs.IsEnterpriseOrCloud(). (Not in this repo, so no inline comment — flagging here for cross-repo visibility.)
  • The CurrentEdition() lazy-panic-in-SSH-goroutine concern (an invalid SHELLHUB_EDITION crashes the SSH process on first session rather than at boot) was already raised in the prior review round and is not re-reported here.

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

Comment thread ui/scripts/gen-config.sh Outdated
Replace `enterprise: boolean` + `cloud: boolean` in `ClientConfig`
with a single `edition: Edition` field. Add `isCommunity()`,
`isEnterprise()`, `isCloud()`, and `isEnterpriseOrCloud()` predicates
to `env.ts`, matching the Go vocabulary.

Delete `utils/features.ts` (`isPremiumFeature`/`hasMfaSupport`);
callers use `isEnterpriseOrCloud()` directly. Fix `Login.tsx` SSO
gate: was `isEnterprise()` (exact), now `isEnterpriseOrCloud()` so
SAML works on cloud too.

Add `src/__mocks__/env.ts` manual mock and a global `vi.mock("@/env")`
in `setup.ts` so test files no longer repeat a 10-line factory.
@luizhf42 luizhf42 force-pushed the refactor/shellhub-edition-enum branch from b4ad36a to 78e0a06 Compare July 15, 2026 20:10
@otavio

otavio commented Jul 15, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Code Review

  • Gathered PR context
  • Reviewing with 5 specialized agents
  • Posting feedback

@luizhf42

Copy link
Copy Markdown
Member Author

Re: the cross-repo note on cloud/internal/license/init.go:16 — this is intentional. The license evaluator enforces device limits based on a self-hosted enterprise license file; cloud enforces device limits via Stripe billing (internal/billing/), not license files. The spec explicitly classified this guard as exact IsEnterprise() so cloud stops running the license evaluator it never actually used (cloud has no license file in the store, so LicenseLoad always returned ErrNoDocumentsCanAcceptDevice returned false, nil → effectively a no-op that could only block).

@otavio

otavio commented Jul 15, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 9m 16s —— View job


Code Review Complete

Reviewed 84 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

The two items from the prior rounds — gateway edition normalization (gateway/config.go, fixed in 3ec01bd) and UI config normalization (ui/scripts/gen-config.sh, fixed in 78e0a06) — are both confirmed resolved. This round's five reviewers re-verified the core risk and found it sound:

  • Every moved gate preserves its truth table across community/enterprise/cloud. The IsEnterprise() "at-least-enterprise" → exact split is handled correctly at each call site (license check stays exact-enterprise; firewall, asciinema, recorder, and SAML/SSO moved to IsEnterpriseOrCloud()). No premium feature is newly exposed to community, and no security check is silently skipped on cloud.
  • Cross-repo (cloud/) compiles and is consistent — all consumed pkg/envs symbols still exported, cloud already migrated to SHELLHUB_EDITION, and no cloud code reads the retired env vars. The exact-IsEnterprise() guard in cloud/internal/license/init.go is intentional (cloud uses Stripe billing, not license files), as the author confirmed.
  • Test rework is sound — Go envstest restores DefaultBackend via t.Cleanup; every edition-overriding UI test re-sets getConfig per test under Vitest file isolation; no coverage was dropped.

A couple of non-blocking notes already raised in prior rounds still stand (not re-reported inline): CurrentEdition() panics lazily rather than at boot for the ssh binary, and the upgrade docs could add an explicit "SHELLHUB_ENTERPRISE/SHELLHUB_CLOUD are removed — rename to SHELLHUB_EDITION" callout for existing self-hosted users.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants