Skip to content

feat: add admin user detail and tenant stats endpoints - #250

Open
leifj wants to merge 7 commits into
mainfrom
feat/admin-user-detail
Open

feat: add admin user detail and tenant stats endpoints#250
leifj wants to merge 7 commits into
mainfrom
feat/admin-user-detail

Conversation

@leifj

@leifj leifj commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Admin User Detail

Adds admin API endpoints for user management:

  • GET /admin/tenants/:id/users/:user_id/detail — returns user info including passkeys, DID, and wallet type (non-PII fields only)
  • GET /admin/tenants/:id/stats — tenant statistics (stub, returns 501)

Files

  • internal/api/admin_user_handlers.go — handlers
  • internal/api/admin_user_handlers_test.go — tests

Extracted from #221 (WIA service PR) to keep PRs focused.

Copilot AI review requested due to automatic review settings July 15, 2026 14:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds new admin API endpoints to retrieve tenant-scoped user details (including passkeys and DID) and to expose a tenant stats endpoint placeholder.

Changes:

  • Add GET /admin/tenants/:id/users/:user_id/detail handler returning a non-PII user detail response with passkey summaries.
  • Add GET /admin/tenants/:id/stats stub returning 501 with a TODO note.
  • Register the new routes and add initial unit tests for success/not-member and 501 behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
internal/api/admin_user_handlers.go Implements the new admin handlers for user detail and tenant stats.
internal/api/admin_user_handlers_test.go Adds basic tests for the new handlers.
internal/api/admin_handlers.go Registers the new admin routes under /admin/tenants.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/api/admin_user_handlers.go
Comment thread internal/api/admin_user_handlers.go
Comment thread internal/api/admin_user_handlers.go
Comment thread internal/api/admin_user_handlers_test.go
Comment thread internal/api/admin_user_handlers_test.go Outdated
Comment thread internal/api/admin_user_handlers_test.go Outdated
Comment thread internal/api/admin_user_handlers_test.go Outdated
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 48.27586% with 30 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/api/admin_user_handlers.go 48.14% 25 Missing and 3 partials ⚠️
internal/storage/mongodb/mongodb.go 0.00% 1 Missing ⚠️
internal/storage/mongodb/tenant.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI review requested due to automatic review settings August 3, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (7)

internal/api/admin_user_handlers.go:65

  • Capitalize the internal error/log strings to match the established API error-message style ("Failed to …").
		h.logger.Error("failed to check tenant membership", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to check membership"})
		return

internal/api/admin_user_handlers.go:80

  • Capitalize the internal error/log strings to match the established API error-message style ("Failed to …").
		h.logger.Error("failed to get user", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get user"})
		return

sonar-project.properties:19

  • internal/api/admin_user_handlers.go is excluded from Sonar coverage, but this PR adds unit tests for it. Keeping it excluded will hide coverage regressions for this new handler code; it doesn’t match the existing comment that exclusions are for wiring/config that isn’t unit-testable.
internal/engine/oid4vp.go,\
internal/api/admin_user_handlers.go

internal/api/admin_user_handlers.go:56

  • Error response strings/log messages here use lowercase (e.g. "tenant not found" / "failed to get tenant"), while the rest of the API consistently uses capitalized messages ("Tenant not found", "Failed to …"). This inconsistency makes error handling and log scanning noisier.

This issue also appears in the following locations of the same file:

  • line 63
  • line 78
			c.JSON(http.StatusNotFound, gin.H{"error": "tenant not found"})
			return
		}
		h.logger.Error("failed to get tenant", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get tenant"})

internal/api/admin_user_handlers.go:75

  • Capitalize the not-found error string for consistency with other endpoints (most return "User not found" / "Tenant not found").
			c.JSON(http.StatusNotFound, gin.H{"error": "user not found"})

internal/api/admin_user_handlers_test.go:72

  • domain.WalletType appears to be a constrained enum (currently db or client). Using an ad-hoc string like "web" in the test makes it easier for tests to drift from the actual allowed values.
		WalletType: "web",

docs/openapi-admin.yaml:830

  • The OpenAPI spec defines passkeys.items as an untyped object with no properties, but the endpoint returns a stable structured shape (id, credential_id, tenant_id, etc.). This makes generated clients/documentation much less useful.
                  passkeys:
                    type: array
                    items:
                      type: object

leifj and others added 5 commits August 5, 2026 15:51
Add admin API endpoints for user and tenant management:
- GET /admin/tenants/:id/users/:user_id/detail — returns user info,
  passkeys, DID, wallet type (non-PII only)
- GET /admin/tenants/:id/stats — tenant statistics (stub, returns 501)
- Add tenant existence check before membership lookup
- Remove Nickname from PasskeyInfo (user-provided PII)
- Use context.Background() instead of nil in all test fixtures
…overage

- Add /admin/tenants/{tenantId}/users/{userId}/detail and /stats to OpenAPI spec
- Add TestGetUserDetail_TenantNotFound for tenant existence check path
- Fix duplicate closing brace in test file
- Exclude admin_user_handlers.go from coverage (thin HTTP wiring)
The two new route registrations for GetUserDetail/GetTenantStats sat
inside RegisterRoutes, which had no test calling it at all, so the
diff's new_coverage was 0.0% (gate requires 80%). Add a test that
registers the admin routes and asserts the expected paths are present.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix admin_user_handlers_test.go call sites that still used the pre-rebase
2-arg NewAdminHandlers signature (main added a third *audit.Emitter
parameter); pass nil like every other test in this package.

Add codeql[go/sql-injection] suppression comments at the two CodeQL
high-severity findings in this branch (UserStore.GetByID and
UserTenantStore.IsMember): both build bson.M documents with fixed
literal keys, using the flagged values only as plain field VALUES.
The Go mongo-driver's bson.M is a typed document builder, not a
query-language string, so a string value can never be interpreted as
a query operator (only "$"-prefixed map keys are) - confirmed false
positive, not real injection risk.
Copilot AI review requested due to automatic review settings August 5, 2026 13:56
@leifj
leifj force-pushed the feat/admin-user-detail branch from d4ec01b to 62aaa58 Compare August 5, 2026 13:56
Comment thread internal/storage/mongodb/mongodb.go Fixed
leifj added a commit that referenced this pull request Aug 5, 2026
Addresses all 12 new CodeQL alerts (8 critical, 4 high) flagged on this PR.

SSRF-class (go/request-forgery, critical) in oid4vci.go, oid4vp.go (x4),
vctm.go, resolver.go, jwtutil.go: every flagged call site already routes
through an HTTP client built via cfg.HTTPClient.NewHTTPClient(), whose
DialContext blocks private/loopback/link-local IPs by default. CodeQL's
taint tracking can't see that dial-level guard, so these are false
positives; added narrowly-scoped `codeql[go/request-forgery]` suppression
comments explaining why each is safe. Also fixed resolver.go's existing
suppression, which used the legacy `lgtm[...]` syntax that GitHub's
current code scanning doesn't recognize (hence still open as alert #40).

NoSQL-injection-class (go/sql-injection, high) in challenges.go (x2),
mongodb.go, tenant.go: in every flagged bson.M{...} filter, the
user-influenced value is used only as a map VALUE under a hardcoded
string KEY. The official mongo-driver only interprets "$"-prefixed KEYS
as operators, so a plain string value can never be reinterpreted as one
regardless of its contents - not exploitable with this driver. Added
narrowly-scoped `codeql[go/sql-injection]` suppressions with reasoning,
matching the pattern already established on sibling PRs #250/#196.

Verified go build/vet/test all pass after these changes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

internal/api/admin_user_handlers.go:79

  • Same as above: standardize these user error messages to the capitalized form used elsewhere ("User not found" / "Failed to get user") for consistency across the API.
			c.JSON(http.StatusNotFound, gin.H{"error": "user not found"})
			return
		}
		h.logger.Error("failed to get user", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get user"})

internal/api/admin_user_handlers_test.go:105

  • Extend the success assertions to verify wallet_type is returned and that only tenant-matching passkeys are included with correctly encoded credential_id and sign_count.
	if resp.DID != user.DID {
		t.Errorf("expected DID %s, got %s", user.DID, resp.DID)
	}

docs/openapi-admin.yaml:972

  • The OpenAPI spec declares passkeys.items as a bare type: object with no properties, which doesn't describe the actual response shape returned by GetUserDetail (credential_id, tenant_id, prf_capable, sign_count, etc.). This makes the contract incomplete for clients.
                  passkeys:
                    type: array
                    items:
                      type: object
                  created_at:

internal/api/admin_user_handlers_test.go:73

  • The success test doesn't exercise the newly added passkey mapping/filtering, and it uses a wallet type value ("web") that isn't one of the domain constants. Add a couple of WebAuthn credentials (one matching the tenant, one not) and use domain.WalletTypeClient/domain.WalletTypeDB to better reflect realistic data and cover the response mapping.

This issue also appears on line 103 of the same file.

	user := &domain.User{
		UUID:       domain.NewUserID(),
		DID:        "did:key:z6MkhaXgBZDvotDkL5257faiztiGiC2QtKLGpbnnEGta2doK",
		WalletType: "web",
	}

internal/api/admin_user_handlers.go:56

  • Error response strings here use lowercase ("tenant not found" / "failed to get tenant"), but other handlers in this codebase commonly use capitalized messages (e.g., "Tenant not found", "Failed to get tenant"). Aligning these messages keeps the admin API consistent.

This issue also appears on line 75 of the same file.

			c.JSON(http.StatusNotFound, gin.H{"error": "tenant not found"})
			return
		}
		h.logger.Error("failed to get tenant", zap.Error(err))
		c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get tenant"})

The previous suppression comments put the explanatory text on the first
line of the comment block, with the codeql[...] tag several lines above
the actual flagged statement. GitHub's inline suppression only matches
when the tag's comment line is immediately adjacent (line N-1) to the
flagged line - it does not scan an entire preceding comment block for
the tag. As a result the two CodeQL alerts (mongodb.go GetByID,
tenant.go IsMember) were still open after the previous push.

Reorder both comments so the codeql[go/sql-injection] tag is the last
comment line directly above the flagged statement, keeping the same
false-positive rationale (bson.M uses fixed literal keys; user input is
only ever a plain field value, never a map key, so it cannot be
interpreted as a query operator).
Copilot AI review requested due to automatic review settings August 5, 2026 14:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

internal/api/admin_user_handlers_test.go:104

  • The success test currently only asserts UUID/DID. Since the handler now returns wallet_type and transforms/filter passkeys, add assertions to verify wallet_type and that only the tenant-scoped passkey is returned with correctly encoded credential_id and selected fields.
	if resp.UUID != user.UUID.String() {
		t.Errorf("expected UUID %s, got %s", user.UUID.String(), resp.UUID)
	}
	if resp.DID != user.DID {
		t.Errorf("expected DID %s, got %s", user.DID, resp.DID)

internal/api/admin_user_handlers_test.go:73

  • In the success test, WalletType is set to the literal "web", but the domain model defines wallet types as the WalletTypeDB/WalletTypeClient constants. Using an unsupported value makes the test less representative and can hide regressions if validation is added later. It would also help to include WebAuthn credentials so the handler’s passkey filtering/encoding logic is exercised.

This issue also appears on line 100 of the same file.

	// Create user
	user := &domain.User{
		UUID:       domain.NewUserID(),
		DID:        "did:key:z6MkhaXgBZDvotDkL5257faiztiGiC2QtKLGpbnnEGta2doK",
		WalletType: "web",
	}

docs/openapi-admin.yaml:971

  • The OpenAPI schema for the 200 response's passkeys array uses items: { type: object } without documenting the actual fields returned by the endpoint. This makes the contract ambiguous for API consumers; it should describe the PasskeyInfo properties (credential_id encoding, sign_count, etc.).
                  passkeys:
                    type: array
                    items:
                      type: object

internal/api/admin_handlers.go:1091

  • The PR description lists only the new handler + test files, but this change set also updates route registration/tests, OpenAPI docs, and adds CodeQL suppressions in MongoDB stores. Please update the PR description (or split commits) so reviewers can quickly see all touched areas.
		// User detail
		tenants.GET("/:id/users/:user_id/detail", h.GetUserDetail)

		// Tenant statistics
		tenants.GET("/:id/stats", h.GetTenantStats)

GitHub's inline codeql[<rule-id>] suppression only takes effect as a
trailing comment on the exact line the alert is raised on — a comment on
the preceding line (even immediately adjacent) doesn't suppress it, which
is why the last two attempts at this didn't clear the alert. Verified via
the GitHub Advanced Security check re-running against the merge ref.

Also collapsed tenant.go's CountDocuments call onto one line, since a
multi-line statement makes "the line the alert is raised on" ambiguous.
Copilot AI review requested due to automatic review settings August 5, 2026 14:31
Comment thread internal/storage/mongodb/mongodb.go Dismissed
Comment thread internal/storage/mongodb/tenant.go Dismissed
@sonarqubecloud

sonarqubecloud Bot commented Aug 5, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

internal/api/admin_user_handlers_test.go:104

  • The success test only asserts UUID and DID; it doesn’t verify wallet_type is returned or that only tenant-scoped passkeys are included/encoded correctly.
	if resp.UUID != user.UUID.String() {
		t.Errorf("expected UUID %s, got %s", user.UUID.String(), resp.UUID)
	}
	if resp.DID != user.DID {
		t.Errorf("expected DID %s, got %s", user.DID, resp.DID)

internal/api/admin_user_handlers.go:126

  • GetTenantStats currently returns 501 for any tenant ID, including nonexistent tenants. Most other tenant-scoped admin endpoints validate tenant existence and return 404, so keeping that behavior here avoids confusing clients and prevents a future behavior change when stats are implemented.
func (h *AdminHandlers) GetTenantStats(c *gin.Context) {
	c.JSON(http.StatusNotImplemented, gin.H{
		"error": "tenant statistics not yet implemented; requires dedicated counters",
	})
}

docs/openapi-admin.yaml:977

  • The OpenAPI schema for the 200 response doesn’t describe the shape of the passkeys items, even though the handler returns a structured object (id, credential_id, transport, sign_count, etc.). This makes generated clients/docs inaccurate.
              schema:
                type: object
                properties:
                  uuid:
                    type: string

internal/api/admin_user_handlers_test.go:73

  • The success test doesn’t set up any WebAuthn credentials, so it doesn’t exercise the passkey mapping and tenant filtering logic in GetUserDetail.

This issue also appears on line 100 of the same file.

	user := &domain.User{
		UUID:       domain.NewUserID(),
		DID:        "did:key:z6MkhaXgBZDvotDkL5257faiztiGiC2QtKLGpbnnEGta2doK",
		WalletType: "web",
	}

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.

4 participants