[SUPERSEDED — DO NOT MERGE] feat(store): owner-scoped API keys and agent tokens - #2
[SUPERSEDED — DO NOT MERGE] feat(store): owner-scoped API keys and agent tokens#20sm0s1z wants to merge 7 commits into
Conversation
Stop dropping tables on connect. Gate AutoMigrate behind SIRIUS_SCHEMA_AUTO_MIGRATE for local/dev only. Add schema_migrations_core ledger with cmd/migrate applying idempotent steps 001–007 so production can run forward-only migrations without bind-mounted go-api sources.
Use the GORM-backed v_id column in migration 005 so fresh schemas can complete the forward-only migration ledger.
…n seam Introduce sirius/module (ADR-004 / bifurcation 3.1) so API extensions register at compile time via modules_community.go, with RouteSetter adapters and an allow-all capability middleware hook. Golden route/module inventories lock Community behavior for future Pro overlays.
Address review gaps on the Module seam: atomic RegisterCommunity, nil/typed-nil rejection, single-mount lifecycle with unlocked callbacks, order-preserving RouteInventory, and CommunityModuleIDs() defensive copies. Remove the hand-maintained Community route replica so real compatibility checks live in sirius-api.
Persist OwnerSubjectID and Scopes on API key metadata so Pro can authorize and list keys per subject without breaking legacy full-access keys.
Pin OwnerSubjectID and EnrollingKeyID on AgentTokenMeta so enrollment can bind agents to student API keys and scope connected-agent lists.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 964d51d84e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {Version: "001_baseline", Name: "GORM AutoMigrate core models", Apply: applyBaseline}, | ||
| {Version: "002_source_attribution", Name: "Source attribution columns on junction tables", Apply: apply002}, | ||
| {Version: "003_enhanced_junction_tables", Name: "Ensure enhanced junction table shape", Apply: apply003}, | ||
| {Version: "004_add_sbom_schema", Name: "SBOM and fingerprint columns on hosts", Apply: apply004}, | ||
| {Version: "005_fix_critical_schema_issues", Name: "Critical schema fixes (ports.number, indexes)", Apply: apply005}, |
There was a problem hiding this comment.
When cmd/migrate is run against a pre-005 production database, this baseline applies the current Port model before the migration that converts legacy ports.id values into ports.number. AutoMigrate can fail while adding the non-null number column to populated rows; if it succeeds, apply005 sees that number exists and skips the conversion, leaving the real port numbers in id and invalid/default data in number. The legacy shape must be detected and transformed before applying the current model baseline.
Useful? React with 👍 / 👎.
| notes TEXT, | ||
| PRIMARY KEY (host_id, vulnerability_id, source) | ||
| )`, | ||
| `CREATE TABLE IF NOT EXISTS host_ports ( |
There was a problem hiding this comment.
On a fresh database, the baseline already creates host_ports from models.HostPort, whose primary key is only (host_id, port_id). Consequently this CREATE TABLE IF NOT EXISTS is a no-op and never installs the declared (host_id, port_id, source) key; HostRepository.LinkHostPort then fails when a second scanner source reports the same port for a host. This step needs to inspect and alter the existing primary key rather than relying on table creation.
Useful? React with 👍 / 👎.
| `ALTER TABLE ports DROP CONSTRAINT IF EXISTS ports_pkey CASCADE`, | ||
| `ALTER TABLE ports RENAME COLUMN id TO number`, | ||
| `ALTER TABLE ports ADD COLUMN IF NOT EXISTS id SERIAL`, |
There was a problem hiding this comment.
When this legacy branch runs on a database with existing host_ports rows, renaming the old numeric port identifier and adding a new serial id changes every referenced key. Dropping ports_pkey ... CASCADE also removes the foreign key, but the migration neither remaps host_ports.port_id to the new serial IDs nor recreates that foreign key, so existing relationships become orphaned or point at unrelated ports. Preserve and remap the junction rows as part of this conversion.
Useful? React with 👍 / 👎.
| statements := []string{ | ||
| vulnerabilityVIDIndexSQL, | ||
| `CREATE INDEX IF NOT EXISTS idx_hosts_ip ON hosts(ip)`, |
There was a problem hiding this comment.
For an upgrade from a schema that allowed duplicate non-empty v_id values, this unique-index statement fails and prevents every later migration from running. The historical 005 migration explicitly removed duplicate VIDs before enforcing uniqueness, so such databases are an expected upgrade state; the ledger version needs equivalent cleanup or another deterministic conflict-resolution step before creating the index.
Useful? React with 👍 / 👎.
| resp, err := s.GetValue(ctx, ConnectedAgentsKey(ownerSubjectID)) | ||
| if err != nil { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
When Valkey is unavailable, the context is canceled, or GET otherwise fails for reasons besides a missing key, this returns a successful empty list. Callers therefore hide all of an owner's connected agents and cannot distinguish an empty set from an infrastructure failure despite the function exposing an error result. Only the missing-key case should map to an empty list; other errors should be returned.
Useful? React with 👍 / 👎.
Alice and Bob scanning the same IP get distinct inventory rows; empty owner keeps Community/admin global behavior. Adds migration 008 unique (owner, ip) index.
SUPERSEDED — DO NOT MERGE WHOLESALE
This prototype mixes useful public SDK primitives with product-specific multi-user assumptions and breaking API changes. Preserve the branch as reference material, but implement the reviewed public contract from #3 and the versioned message/job contract from #4 on clean branches from current
main.Key extraction rules:
student, role, class, or Pro authorization in go-api;Original prototype included
OwnerSubjectID, key scopes, agent token owner/enrolling-key metadata, same-IP scoped host persistence, and migration work. Those concepts remain source material for #3; this PR is not the release vehicle.