Skip to content

test: cover API keys and the legacy password upgrade end to end - #7367

Merged
otavio merged 5 commits into
test/e2e-ssh-legacy-modefrom
test/e2e-api-keys
Oct 7, 2026
Merged

otavio merged 5 commits into
test/e2e-ssh-legacy-modefrom
test/e2e-api-keys

Conversation

@otavio

@otavio otavio commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Covers Domain 18 (API Keys) of shellhub-io/team#243, along with the remaining Domain 1 API key and legacy password items, with testcontainers tests. Some open items were already asserted by #7363's tests, so this PR does not test them again.

tests/
├── environment/
│   ├── api_key.go              # ExpireAPIKeyIn: an API key expiring seconds from now
│   └── user.go                 # read and replace a user's stored password digest
├── api_key_test.go             # TestNamespaceAPIKeyAuthentication: tags, role, -1, expiry, instance prefix
├── api_key_ssh_test.go         # TestAPIKeySSHAccess: no grant, deny over allow, identity TTL, key deletion
├── password_hash_test.go       # TestLegacyPasswordDigest: SHA256 digest rehashed with bcrypt on login
└── identity_helpers_test.go    # enrollAPIKeyIdentity, expireIdentityAndRequireRefused (refactor commit)
Item Test
D18 tag operations TestNamespaceAPIKeyAuthentication/a_key_creates,_lists,_attaches,_detaches_and_deletes_tags
D18 + D1 expired API key rejected …/a_key_is_refused_once_its_expiry_passes
D18 non-expiring key (-1) …/a_key_that_never_expires_is_stored_without_an_expiry_and_authenticates
D18 key respects its role …/a_key_acts_only_within_its_role
D18 instance key rejected on regular routes …/a_key_carrying_the_instance_key_prefix_is_refused_on_a_namespace_route
D18 SSH denied without policy TestAPIKeySSHAccess/a_key_no_policy_names_is_refused
D18 SSH deny overrides allow TestAPIKeySSHAccess/a_deny_policy_naming_the_key_beats_the_allow_that_names_it
D18 SSH expired identity, identity within TTL TestAPIKeySSHAccess/a_key's_identity_is_accepted_until_its_expiry_and_refused_after
D18 deleting the key revokes its identities TestAPIKeySSHAccess/deleting_a_key_revokes_its_identities
D18 not matched by all-members #7363: TestAccessPolicyEvaluation/an_every-member_policy_grants_members_and_not_API_keys
D18 single-use consumed, rejected on second attempt #7363: TestSSHIdentityConnection, both single-use subtests
D18 skips RequireReauth #7363: TestAccessPolicyReauth/an_API_key_is_never_asked_to_re-authenticate
D1 SHA256 password upgrades to bcrypt TestLegacyPasswordDigest

Each refusal asserts a specific outcome: an HTTP status (401, 403 or 404), or the denial reason in the server log on the same line as the key's ID. The API key expiry test sets an expiry 10 seconds ahead and waits for it to pass. The SSH identity expiry test moves expires_at into the past. No test deletes a row to fake an expiry.

Not covered:

  • Instance key admin routes, creation, the creator losing admin, and instance key expiry (D18: 4 items, D1: 2). The routes are mounted only by cloud/internal/admin/routes/routes.go. The gateway proxies /admin/api* only when enterprise is enabled, and this suite runs the community stack.
  • D1 "API key role capped at creator's current role". b6689e5 deliberately stopped re-capping a key when its creator is demoted, and members.spec.ts asserts that. At creation, only administrators and owners hold APIKeyCreate, and no key can be owner, so the creator's role and the administrator ceiling are the same limit.
  • Renaming a tag. PATCH /api/tags/:name answers 200 with no body, but the spec declares the renamed tag. The strict-validating stack turns that into a 500.
  • The instance prefix subtest is weak. The server refuses an sh_admin_ key outside /admin/api before looking it up. An unknown key would get the same 401, so no server mutation can make this subtest fail.

Evidence

  • Before: these items had no test.
    After: the full tests/ suite passes on the tree before rebasing onto test: cover SSH legacy mode end to end #7366. After the rebase, it fails only in test: cover SSH legacy mode end to end #7366's TestSSHServerRestart, which fails the same way intermittently when run alone (unable to authenticate, after a 30-second wait). The new tests pass:

    TestNamespaceAPIKeyAuthentication  7/7
    TestAPIKeySSHAccess                4/4
    TestLegacyPasswordDigest           2/2
    TestSSHIdentityConnection          5/5  (helper extraction)
    
  • The tests fail when the behaviour breaks. I mutated the server code each subtest guards, ran the tests, then reverted:

    • APIKey.IsValid always true: the API key expiry subtest fails
    • apiKeyRoleLimit always administrator: the observer and operator subtests fail
    • SHA256 rehash skipped at login: the upgrade subtest fails
    • identity.Active not checked in the SSH auther: the identity expiry subtest fails
    • a matching deny ignored for API keys: the deny-over-allow subtest fails
    • an allow that does not match granting API keys anyway: the no-grant subtest fails
    • DeleteAPIKey deleting nothing: the key-deletion subtest fails
    • -1 read as a timestamp: the never-expires subtest fails, along with every other subtest that uses a -1 key

Merge Danger

Door: two-way

Blast Radius: tests

Test code only. The refactor commit moves helpers that #7363's identity tests and the enrollment tests call: tagNames now takes []models.Tag.

@otavio
otavio requested a review from a team as a code owner October 7, 2026 12:51
@otavio
otavio added this pull request to stack #7361 October 7, 2026 12:51
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@otavio otavio changed the title test/e2e api keys test: cover API keys and the legacy password upgrade end to end Oct 7, 2026
@otavio
otavio requested a review from a team as a code owner October 7, 2026 13:16
@otavio
otavio force-pushed the test/e2e-api-keys branch from 2a8609b to 6c06aff Compare October 7, 2026 13:31
otavio added 5 commits October 7, 2026 11:47
withAPIKey replaces the closure TestRoutesThatRefuseAPIKeys kept for the X-API-Key header, so the
next tests in the file send a key the same way. tagNames takes a tag list instead of a device, so
it reads the tags a namespace lists as well as the ones a device carries.

enrollAPIKeyIdentity is newAPIKeyIdentity with an expiry, for an API key identity that has to
expire. expireIdentityAndRequireRefused lifts the expired-identity steps out of
TestSSHIdentityConnection; the name says it expires the identity, because the caller's later
assertions depend on it having done so.
A key manages the namespace's tags, on their own and on a device, and acts within its role: an
observer key reads devices and nothing more, an operator key also manages tags, and only an
administrator key reaches the access policies. Renaming a tag is left out because PATCH
/api/tags/:name answers 200 with no body while the OpenAPI spec declares the renamed tag, so
the strict-validating stack turns it into a 500.

A key created with -1 is stored and listed with -1 and authenticates. -1 is the marker for a key
that never expires; were it read as a Unix time it would be a second before 1970.

The expiring key is created with a 30-day expiry, the only kind the API mints, and
ExpireAPIKeyIn then moves it 10 seconds ahead. That has to happen before the key's first use:
the server caches a key it authenticated for two minutes, with the expiry it read. The first
request then caches it, and the 401 that follows comes from the cached copy, since the server
checks the expiry on a cache hit too. The wait allows three times the TTL for the clock to pass it.
The server honours a key carrying the instance key prefix only under /admin/api, and refuses it
everywhere else before looking it up, so a plain prefixed key stands in for a real one. The
routes that mint and accept instance keys belong to the enterprise admin API, which the
community stack this suite runs on does not serve: the gateway proxies /admin/api only when
enterprise is enabled. The rest of the instance key cases need that stack.
With no policy naming it, an API key's identity is refused for want of a grant, and a deny policy
naming the key refuses it although an allow naming it let it in a moment before. The identity
works within its expiry and is refused once it passes, without being offered for enrollment again.

Deleting the key takes its identities and its access policies with it, through the foreign keys
that cascade from api_keys. The policy is created without the cleanup grant registers, since
there is nothing left to delete; the test asserts it is gone, and that the gateway now asks to
enroll the key as one it has never seen.
Every path that sets a password hashes it with bcrypt, so the test writes an unsalted SHA256
digest into the users row to stand in for an account created before bcrypt. A wrong password is
refused and leaves that digest alone; the right one logs in, the stored digest becomes a bcrypt
hash of the password, and the password keeps working against it.
@otavio
otavio force-pushed the test/e2e-api-keys branch from 6c06aff to 83b20fb Compare October 7, 2026 14:47
@otavio
otavio merged commit 56ccc97 into master Oct 7, 2026
41 of 47 checks passed
@otavio
otavio deleted the test/e2e-api-keys branch October 7, 2026 16:00
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.

1 participant