admin endpoint for listing and revoking tokens#1001
Conversation
WalkthroughAdds admin APIs and backend support to list and revoke auth tokens (filters: token, hash, outpoint, txid). Changes touch OpenAPI/Protobuf specs, token cache list/revoke logic, indexer service surface, gRPC handler wiring and permissions, domain outpoint validation, and tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Admin gRPC Handler
participant Service as Indexer Service
participant Cache as Token Cache
participant Crypto as Crypto/Signature
Client->>Handler: ListTokens(token/hash/outpoint/txid)
Handler->>Service: ListTokens(...)
alt token provided
Service->>Crypto: extractTokenHash(token)
Crypto-->>Service: hash
else hash provided
Service-->>Service: use provided hash
end
Service->>Cache: list(hash, outpoint, txid)
Cache-->>Service: []TokenEntry
Service-->>Handler: ListTokensResponse
Handler-->>Client: ListTokensResponse
Client->>Handler: RevokeTokens(token/hash/outpoint/txid)
Handler->>Service: RevokeTokens(...)
Service->>Cache: revoke(hash, outpoint, txid)
Cache-->>Service: revoked count
Service-->>Handler: RevokeTokensResponse
Handler-->>Client: RevokeTokensResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/core/application/indexer_exposure_test.go (1)
927-1025: Avoid cross-subtest state coupling inTestListAndRevokeTokens.Line 927 reuses one mutable
indexeracross many subtests, so assertions depend on execution order. Prefer creating a fresh indexer/cache per subtest to keep tests isolated and robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/indexer_exposure_test.go` around lines 927 - 1025, The tests share a single mutable indexer instance (created with newTestIndexer and referenced as indexer) across multiple t.Run subtests which couples state and makes assertions order-dependent; fix by instantiating a fresh indexer in each subtest (call newTestIndexer inside each t.Run) and recreate any required tokens within that subtest using createAuthToken before calling ListTokens or RevokeTokens so each subtest operates on an isolated cache/state.internal/core/application/token_cache.go (1)
192-194: MakeTokenEntry.Outpointsorder deterministic.Line 192–194 appends from map iteration, so output order is random. Sort
entry.Outpointsbefore appending to results for stable API responses.🔧 Proposed fix
for op := range outpoints { entry.Outpoints = append(entry.Outpoints, op.String()) } + sort.Strings(entry.Outpoints) result = append(result, entry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/token_cache.go` around lines 192 - 194, The loop appends map iteration results from the variable outpoints into TokenEntry.Outpoints causing nondeterministic order; fix by collecting the outpoint keys into a slice, sort the slice with sort.Strings, then append or assign the sorted values to entry.Outpoints (referencing the TokenEntry.Outpoints field and the outpoints map) so API responses are stable and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api-spec/openapi/swagger/ark/v1/admin.openapi.json`:
- Around line 1651-1671: The RevokeTokensRequest schema currently allows an
empty object but the API requires at least one filter; update the
RevokeTokensRequest JSON schema to include an anyOf constraint that requires at
least one of the properties (hash, outpoint, token, txid). Specifically, add an
"anyOf" array with entries like {"required":["hash"]},
{"required":["outpoint"]}, {"required":["token"]}, {"required":["txid"]} inside
the RevokeTokensRequest definition so generated clients/validators will reject
empty revoke requests.
In `@api-spec/protobuf/ark/v1/admin.proto`:
- Around line 113-116: ListTokensRequest currently exposes a raw token filter on
the GET /v1/admin/tokens endpoint; remove the secret `token` filter from the GET
path and either (A) change the ListTokens RPC binding to use POST with the token
in the request body (update the HTTP option for rpc ListTokens to use POST) or
(B) keep GET for non-secret fields and add a separate RPC (e.g., LookupToken or
ListTokensByHash) that accepts a token_hash in the body; update the protobuf
service and method options accordingly and ensure `ListTokensRequest.token` is
no longer bound to a GET URL (or replace it with a hashed field) so raw secrets
are never placed in URLs or logs.
In `@internal/core/application/indexer.go`:
- Around line 1020-1032: The RevokeTokens handler currently allows all filters
to be empty and can revoke the entire cache; add a service-layer guard in
indexerService.RevokeTokens to require at least one non-empty filter
(token/hash/outpoint/txid) before calling tokenCache.revoke. Use
resolveTokenFilter(token, hash) and normalizeOutpoint(outpoint) as before, but
after resolving h and op check that h != "" || op != "" || txid != "" (or
equivalent from resolveTokenFilter output) and return an error (e.g.,
ErrInvalidRequest or a new descriptive error) when all are empty to prevent
unfiltered revocation. Ensure the guard runs before invoking i.tokenCache.revoke
so no full-cache revocations can occur if upstream validation is bypassed.
- Around line 995-1003: The normalizeOutpoint flow accepts structurally invalid
txids because Outpoint.FromString doesn't validate the txid content; update
Outpoint.FromString (used by normalizeOutpoint) to explicitly validate the txid
part: split the input into txid and vout, ensure txid is non-empty, exactly 64
hex chars, and hex-decodes to 32 bytes (return an error if not), and validate
vout is a non-negative integer within expected range; keep normalizeOutpoint
unchanged other than relying on the stricter FromString so op.String() only
returns canonical, validated outpoints.
In `@internal/interface/grpc/handlers/adminservice.go`:
- Around line 511-515: The ListTokens and RevokeTokens gRPC handlers currently
map all errors from a.indexerService.ListTokens / RevokeTokens to
codes.Internal; change them to inspect the returned error and translate known
validation errors from the service layer (those produced by normalizeOutpoint,
extractTokenHash, and resolveTokenFilter) into a gRPC status with
codes.InvalidArgument, otherwise keep codes.Internal; implement this by checking
the error type or sentinel (or matching error text/unwrap chain) after calling
a.indexerService.ListTokens and a.indexerService.RevokeTokens and returning
status.Errorf(codes.InvalidArgument, ...) for validation failures and
status.Errorf(codes.Internal, ...) for others so client input errors are not
reported as server errors.
---
Nitpick comments:
In `@internal/core/application/indexer_exposure_test.go`:
- Around line 927-1025: The tests share a single mutable indexer instance
(created with newTestIndexer and referenced as indexer) across multiple t.Run
subtests which couples state and makes assertions order-dependent; fix by
instantiating a fresh indexer in each subtest (call newTestIndexer inside each
t.Run) and recreate any required tokens within that subtest using
createAuthToken before calling ListTokens or RevokeTokens so each subtest
operates on an isolated cache/state.
In `@internal/core/application/token_cache.go`:
- Around line 192-194: The loop appends map iteration results from the variable
outpoints into TokenEntry.Outpoints causing nondeterministic order; fix by
collecting the outpoint keys into a slice, sort the slice with sort.Strings,
then append or assign the sorted values to entry.Outpoints (referencing the
TokenEntry.Outpoints field and the outpoints map) so API responses are stable
and deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cd8ef91-7453-4987-b7e2-cd9bfd244a5d
⛔ Files ignored due to path filters (4)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/admin_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (10)
api-spec/openapi/swagger/ark/v1/admin.openapi.jsonapi-spec/protobuf/ark/v1/admin.protointernal/core/application/indexer.gointernal/core/application/indexer_admin_test.gointernal/core/application/indexer_exposure_test.gointernal/core/application/token_cache.gointernal/core/application/token_cache_test.gointernal/interface/grpc/handlers/adminservice.gointernal/interface/grpc/permissions/permissions.gointernal/interface/grpc/service.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/indexer.go`:
- Around line 1012-1023: ListTokens (and the adjacent admin listing/revocation
endpoint function) currently call tokenCache.list which filters out expired
entries because extractTokenHash ignores embedded expiry; add an includeExpired
boolean parameter to tokenCache.list and the internal token cache
iteration/filtering functions (the methods in token_cache.go that skip
now.After(expiresAt)) and thread an includeExpired argument from ListTokens (and
the other admin method around lines 1026-1040) so admin calls pass
includeExpired=true; keep existing semantics for non-admin callers by passing
false, and ensure resolveTokenFilter/normalizeOutpoint usage is unchanged while
only adding and forwarding the new flag.
- Around line 990-997: resolveTokenFilter currently ignores the supplied raw
hash when a token is present; change it to enforce AND semantics: if only token
is provided, return i.extractTokenHash(token); if only hash is provided,
validate/canonicalize the raw hash (e.g., ensure hex characters, correct length,
normalize to lowercase) and return it; if both token and hash are provided, call
i.extractTokenHash(token) to get tokenHash, canonicalize the provided hash the
same way, compare them and return tokenHash only when they match, otherwise
return an error. Use the existing resolveTokenFilter and extractTokenHash
identifiers when updating logic and reuse the same canonicalization rules for
both paths.
In `@internal/core/domain/vtxo.go`:
- Around line 26-37: The parsed txid is validated with hex.DecodeString but the
original casing is preserved when assigning k.Txid, causing mismatch with
canonical forms used by normalizeOutpoint()/Outpoint.String(); after decoding
the txid bytes, re-encode to a canonical lowercase hex string (e.g., via
hex.EncodeToString on the decoded bytes) and assign that canonical value to
k.Txid instead of the original txid variable so all stored outpoints match the
normalized form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0282780-1e15-4127-8515-063f25fc3c64
⛔ Files ignored due to path filters (3)
api-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**
📒 Files selected for processing (9)
api-spec/openapi/swagger/ark/v1/admin.openapi.jsonapi-spec/protobuf/ark/v1/admin.protointernal/core/application/indexer.gointernal/core/application/indexer_admin_test.gointernal/core/application/indexer_exposure_test.gointernal/core/application/token_cache.gointernal/core/domain/vtxo.gointernal/interface/grpc/handlers/adminservice.gointernal/interface/grpc/handlers/adminservice_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api-spec/openapi/swagger/ark/v1/admin.openapi.json
- internal/core/application/indexer_admin_test.go
- internal/core/application/token_cache.go
- internal/core/application/indexer_exposure_test.go
Code Review — Admin Token Listing & RevocationGood PR overall. The design is sound: proper auth gating via macaroon permissions, signature verification on token filters, input validation, and a safety check preventing filter-less revocation. Tests are thorough. A few issues to address: Security1. 2. Permission entity inconsistency — investigate (medium) 3. Token hash returned to admin — acceptable but worth noting Correctness4. Expiry extraction from map is non-deterministic (bug) var expiresAt time.Time
for _, exp := range outpoints {
expiresAt = exp
break
}This grabs the expiry from an arbitrary map entry. If outpoints within the same hash could ever have different expiry times (e.g., if 5. Test CoverageTests are solid — covers happy paths, error paths, expiry behavior, filter combinations, the cap at 6. Missing test: Code Quality7. Duplicated filter logic between 8. Minor
VerdictThe core security properties are correct: endpoints are permission-gated, token signature is verified, revoke requires at least one filter. The main items to address are the permission entity question (#2) and the fragile expiry extraction (#4). The rest are improvements that can be done in follow-up. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/core/application/indexer.go (1)
978-985:⚠️ Potential issue | 🟠 MajorEnforce AND semantics when both
tokenandhashare set.This still returns the token-derived hash unconditionally, so a conflicting raw
hashis silently ignored. That breaks the advertised filter semantics and also leaves raw hash inputs unvalidated/case-sensitive when no token is supplied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/indexer.go` around lines 978 - 985, The current resolveTokenFilter(token, hash) always returns the token-derived hash and ignores a provided raw hash; change it to enforce AND semantics: if token != "" call extractTokenHash(token) to get tokenHash (propagating any error); if hash != "" normalize/validate the raw hash the same way tokenHash is produced (e.g., apply the same normalization/validation logic used by extractTokenHash) and then compare tokenHash vs normalized raw hash—if they differ return an error indicating a mismatch; if only one is set return the normalized/validated value; ensure resolveTokenFilter returns the validated/normalized hash when hash is supplied without a token and preserves existing error propagation from extractTokenHash.internal/core/application/token_cache.go (1)
143-150:⚠️ Potential issue | 🟠 MajorExpired tokens are still unreachable through these admin cache helpers.
extractTokenHash()intentionally ignores token TTL, but every list/revoke path here still drops expired buckets before matching. Admins therefore still cannot inspect or revoke an expired token until the sweeper removes it.Also applies to: 203-209, 237-244, 290-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/token_cache.go` around lines 143 - 150, The admin helpers currently drop expired buckets by checking expiresAt from outpoints before attempting to match token hashes, which prevents extractTokenHash (which ignores TTL) from finding expired tokens; change the logic in the list/revoke helper loops that iterate over outpoints so you first compute/match the token hash using extractTokenHash against the admin-provided token identifier and only then apply the TTL check for normal lookup behavior—i.e., do not skip/continue on now.After(expiresAt) before matching; instead allow matching against expired buckets (so admins can inspect/revoke), and apply expiry-only when deciding whether to include tokens in non-admin lists or for normal-use validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/indexer.go`:
- Around line 1000-1011: ListTokens currently returns the raw slice from
i.tokenCache.list(h, op, txid) which can exceed the admin cap and has no
deterministic ordering; update ListTokens to take the returned []TokenEntry,
sort it into oldest-first (deterministic) order and then truncate it to a
maximum of 50 entries before returning. Locate the call in ListTokens and
operate on the slice returned by tokenCache.list, applying a stable sort on
TokenEntry timestamp/createdAt field (or equivalent) and then return at most 50
items (e.g., slice[:min(len,50)]) so the endpoint enforces the cap and ordering.
In `@internal/core/application/token_cache.go`:
- Around line 130-142: The list method currently short-circuits when hash != ""
by returning c.listByHash(hash) before applying outpointStr/txid filters; change
tokenCache.list so that when hash is provided it first obtains candidates from
c.listByHash(hash) and then applies the remaining outpointStr and txid filters
(same filter logic used for the full scan) before returning; apply the identical
fix to the other method referenced in the comment (the similar early-return
block around lines 224-236) so both tokenCache.list and the other
listing/revoking function enforce the AND semantics of hash + outpoint/txid.
---
Duplicate comments:
In `@internal/core/application/indexer.go`:
- Around line 978-985: The current resolveTokenFilter(token, hash) always
returns the token-derived hash and ignores a provided raw hash; change it to
enforce AND semantics: if token != "" call extractTokenHash(token) to get
tokenHash (propagating any error); if hash != "" normalize/validate the raw hash
the same way tokenHash is produced (e.g., apply the same
normalization/validation logic used by extractTokenHash) and then compare
tokenHash vs normalized raw hash—if they differ return an error indicating a
mismatch; if only one is set return the normalized/validated value; ensure
resolveTokenFilter returns the validated/normalized hash when hash is supplied
without a token and preserves existing error propagation from extractTokenHash.
In `@internal/core/application/token_cache.go`:
- Around line 143-150: The admin helpers currently drop expired buckets by
checking expiresAt from outpoints before attempting to match token hashes, which
prevents extractTokenHash (which ignores TTL) from finding expired tokens;
change the logic in the list/revoke helper loops that iterate over outpoints so
you first compute/match the token hash using extractTokenHash against the
admin-provided token identifier and only then apply the TTL check for normal
lookup behavior—i.e., do not skip/continue on now.After(expiresAt) before
matching; instead allow matching against expired buckets (so admins can
inspect/revoke), and apply expiry-only when deciding whether to include tokens
in non-admin lists or for normal-use validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4aef07d-37b8-4638-9960-99326d443b8a
📒 Files selected for processing (8)
internal/core/application/indexer.gointernal/core/application/indexer_admin_test.gointernal/core/application/indexer_exposure_test.gointernal/core/application/token_cache.gointernal/core/application/token_cache_test.gointernal/core/domain/vtxo.gointernal/interface/grpc/handlers/adminservice.gointernal/interface/grpc/handlers/adminservice_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/interface/grpc/handlers/adminservice_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/core/domain/vtxo.go
- internal/core/application/indexer_exposure_test.go
- internal/core/application/token_cache_test.go
2 participants
closes #1000
Summary by CodeRabbit
New Features
Bug Fixes
Tests