[otavio]

Summary

A provisioning key stored the names of its tags, and enrollment created any name the namespace lacked. After a rename, the key still held the old name, so the next device it enrolled brought the old tag back next to the renamed one. A delete answered 204 and the next enrollment re-created the tag. A deny access policy or firewall deny rule on the renamed tag then missed every device the key enrolled from that point on.

Keys now hold the tag itself, as public keys, access policies and firewall rules have since #7437 and #7537:

 provisioning_keys
-  tags text[]                                   names
+provisioning_key_tags (key_digest, namespace_id, tag_id)   migration 048
+  links every name a key held; creates a tag the namespace lacked

 CreateProvisioningKey / UpdateProvisioningKey
+  resolve each name to a tag, creating a missing one        API still takes names

 enroll(device, key)
-  resolve or create each name, then attach
+  attach key.TagIDs

 DeleteTag
   public keys, access policies, TagHolder (firewall rules)
+  provisioning keys not revoked
   ErrTagInUse -> 409, fields.name = `provisioning key "x"`

A revoked key does not count as a holder: it enrolls nothing, cannot be edited and has no delete route, so counting it would pin the tag forever. Deleting the tag drops it from the revoked key through the FK cascade.

The console's 409 message said is used in the device filter of .... A provisioning key has no device filter, so it now says is held by ....

The e2e tests are in #7603, stacked on this one.

Evidence

TestTagsAppliedByProvisioningKey (stacked PR), with a key that applies one tag:

• Before (cf7c08123, written against the expected behaviour first):

  a_renamed_tag:  expected: []string{"keyafter"}  actual: []string{"keybefore"}   # next enrolled device
  a_deleted_tag:  delete answered 204; next enrollment: Should be empty, but was [keygone]
                  []string{"keygone", "keybefore", "keyafter"} should not contain "keygone"
  

• After:

  --- PASS: TestTagsAppliedByProvisioningKey/a_renamed_tag_keeps_being_applied_under_its_new_name
  --- PASS: TestTagsAppliedByProvisioningKey/a_tag_a_provisioning_key_applies_is_not_deleted
  --- PASS: TestTagsAppliedByProvisioningKey/a_tag_only_a_revoked_provisioning_key_holds_is_deleted
  

Also:

• TestProvisioningKeyTagsMigration failed with no migration numbered 048 before the migration. It now passes: names become links, a missing tag is created, an existing one is reused, and the column is gone.
• TestPgStore/ProvisioningKeyStore/*: resolve and list report a renamed tag under its new name, and an update replaces the links.
• TestService_DeleteTag/fails_when_provisioning_keys_not_revoked_hold_the_tag, plus CreateProvisioningKey and UpdateProvisioningKey cases for name resolution and tag creation.
• server/api/services, server/api/routes/... and server/api/store/... pass. The full tests/ suite passes except the four TestCloud* cases, which fail at setup here because SHELLHUB_STRIPE_PUBLISHABLE_KEY is unset.
• ManageTagsModal.test.tsx passes. golangci-lint finds 0 issues in server/api and tests; eslint, tsc and knip pass.

Merge Danger

Door: one-way

Migration 048 drops provisioning_keys.tags after copying it into links. Its down migration rebuilds the array from the links, but under the tags' current names. Tags it created for names the namespace lacked stay after a rollback.

Blast Radius: tags

Deleting a tag that an active provisioning key applies now answers 409 instead of 204. Creating or editing a key with a new tag name now creates that tag at once instead of at the first enrollment.

Refs shellhub-io/team#243

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[otavio]

/review

[github-actions]

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

---

Code Review Complete

I reviewed all 26 files for Standards (repo rules, language conventions, tests, code smells), Spec (does the PR implement the issue?) and Correctness (security and runtime defects). I found no new issues, and the code is fine to merge as it stands.

This PR closes no issue, and its commits cite shellhub-io/team#243 only through Refs:. So the Spec review covered only the Domain 8 and Domain 11 lines this PR touches. All of them are met:

• "Automatic key tags inherited by device", "Provisioning key tag inheritance on enrollment" and "Re-registration inherits key's current tags":
  • auth.go and enrollment.go apply key.TagIDs.
  • Every path that resolves a key loads the Tags relation.
  • Re-registration resolves the key again, so it applies the key's tags as they are at that moment.
• "Renamed tag reflects in provisioning key tags": the key holds the tag's ID, and the name is read back through the provisioning_key_tags join.
• "the delete is now refused with 409 while a key not revoked holds the tag" and "A tag only a revoked provisioning key holds is deleted": met through tagHolders in tags.go and the ON DELETE CASCADE on tag_id.
• "Deleting a tag that no filter and no unrevoked provisioning key holds detaches it from every device…" and "Tags are namespace-scoped": met. Tags are resolved and created within the request's scope, and migration 048 joins on namespace_id.

Notes:

• I dropped four findings, and no cap dropped anything:
  • The ProvisioningKeyList store contract doesn't say keys come back with their tags or name its errors. An earlier round dropped this as older than this branch. List returned tags before this PR too, from the column.
  • Building the entity.ProvisioningKeyTag link inline instead of through a NewProvisioningKeyTag constructor, the way public keys and access policies do. It is used in one place, so the constructor would only be a different way to write it, not a fix.
  • Missing tags are created without CreateTag's name rules, because key tags are validated only with dive,required. An earlier round already noted this as a possible follow-up.
  • The m2m Tags relation has no ORDER BY. Earlier rounds already raised it.
• Correctness found nothing else. It checked:
  • Migration 048, up and down, including empty arrays, case-sensitive matching and the namespace join.
  • The composite-key m2m.
  • That the only ProvisioningKeyUpdate caller passes nil TagIDs unless the request names tags.
  • That enrollment resolves keys in the device's tenant.
  • That nothing left in pg/ uses the dropped tags column.
  • That the PR adds no routes and no goroutines.
• Cross-repo: cloud/ doesn't use entity.ProvisioningKey, the provisioning_keys SQL, resolveUnique or NewErrTagInUse outside its generated mocks, and the interface signatures are unchanged. I checked this by reading the source; I didn't build Cloud.
• I didn't run the test suites.