From be37d944f18c4e1d883eaf3350ccc80314a0cd3c Mon Sep 17 00:00:00 2001 From: kolaente Date: Sat, 29 Aug 2026 00:36:12 +0200 Subject: [PATCH] refactor(api-tokens): share one HasPermission lookup across caldav, feeds and mcp --- pkg/models/api_tokens.go | 43 ++++++++++++++++++----------------- pkg/models/api_tokens_test.go | 18 +++++++++++++++ pkg/modules/mcp/scope.go | 19 +++------------- pkg/modules/mcp/scope_test.go | 12 ++++++++++ 4 files changed, 55 insertions(+), 37 deletions(-) diff --git a/pkg/models/api_tokens.go b/pkg/models/api_tokens.go index bbaf8007d..b58d5619e 100644 --- a/pkg/models/api_tokens.go +++ b/pkg/models/api_tokens.go @@ -233,34 +233,35 @@ func (t *APIToken) Delete(s *xorm.Session, a web.Auth) (err error) { return nil } -// HasCaldavAccess checks whether the token has the caldav access permission. +// HasPermission reports whether the token grants permission within group. +// Both sides are canonicalised the same way CanDoAPIRoute does it, so a +// hyphenated group slug stored on the token still matches. +func (t *APIToken) HasPermission(group, permission string) bool { + if t == nil { + return false + } + group = canonicalAPITokenGroup(group) + for storedGroup, perms := range t.APIPermissions { + if canonicalAPITokenGroup(storedGroup) == group && slices.Contains(perms, permission) { + return true + } + } + return false +} + func (t *APIToken) HasCaldavAccess() bool { - perms, has := t.APIPermissions["caldav"] - if !has { - return false - } - return slices.Contains(perms, "access") + return t.HasPermission("caldav", "access") } -// HasFeedsAccess checks whether the token has the feeds access permission. func (t *APIToken) HasFeedsAccess() bool { - perms, has := t.APIPermissions["feeds"] - if !has { - return false - } - return slices.Contains(perms, "access") + return t.HasPermission("feeds", "access") } -// HasMCPAccess checks whether the token has the mcp access permission. -// The MCP endpoint uses POST, GET, and DELETE on the same path (streamable-HTTP -// transport), so CanDoAPIRoute can't gate it — the MCP entry handler calls -// this directly after the middleware skips the route check. +// HasMCPAccess is called inline by the MCP entry handler: the streamable-HTTP +// transport uses POST, GET and DELETE on one path, which CanDoAPIRoute's exact +// (method, path) match cannot express. func (t *APIToken) HasMCPAccess() bool { - perms, has := t.APIPermissions["mcp"] - if !has { - return false - } - return slices.Contains(perms, "access") + return t.HasPermission("mcp", "access") } // GetTokenFromTokenString returns the full token object from the original token string. diff --git a/pkg/models/api_tokens_test.go b/pkg/models/api_tokens_test.go index 2366f75cc..c41c965bf 100644 --- a/pkg/models/api_tokens_test.go +++ b/pkg/models/api_tokens_test.go @@ -246,6 +246,24 @@ func TestAPIToken_HasMCPAccess(t *testing.T) { }) } +func TestAPIToken_HasPermission(t *testing.T) { + t.Run("nil token", func(t *testing.T) { + var token *APIToken + assert.False(t, token.HasPermission("tasks", "read_all")) + }) + t.Run("nil permissions", func(t *testing.T) { + assert.False(t, (&APIToken{}).HasPermission("tasks", "read_all")) + }) + t.Run("hyphenated group key is canonicalised", func(t *testing.T) { + token := &APIToken{ + APIPermissions: APIPermissions{"time-entries": {"read_all"}}, + } + assert.True(t, token.HasPermission("time_entries", "read_all")) + assert.True(t, token.HasPermission("time-entries", "read_all")) + assert.False(t, token.HasPermission("time_entries", "create")) + }) +} + func TestAPIToken_GetTokenFromTokenString(t *testing.T) { t.Run("valid token", func(t *testing.T) { s := db.NewSession() diff --git a/pkg/modules/mcp/scope.go b/pkg/modules/mcp/scope.go index 0ef276b9a..1f66b6dd7 100644 --- a/pkg/modules/mcp/scope.go +++ b/pkg/modules/mcp/scope.go @@ -18,7 +18,6 @@ package mcp import ( "errors" - "slices" "code.vikunja.io/api/pkg/models" ) @@ -29,20 +28,8 @@ import ( // the client sees a structured failure rather than a JSON-RPC protocol error. var ErrScopeDenied = errors.New("mcp: tool not authorized for this token") -// tokenAuthorizes returns true iff the token's APIPermissions map contains -// op.Permission() under the given resource's scope group. This is the -// (group, permission) lookup that gates both tools/list visibility and -// tools/call invocation; it intentionally duplicates rather than shares -// CanDoAPIRoute's logic because MCP doesn't have a path/method to match — -// the registry already owns the (resource, op) → (group, permission) mapping. -// -// A nil token or nil APIPermissions returns false (slices.Contains on a nil -// slice is also false, so the second case is naturally handled). Defensive -// checks here keep the dispatcher's "fail closed" contract even if the entry -// handler somehow forgets to attach a token. +// tokenAuthorizes maps an (mcp resource, op) pair onto the (group, permission) +// pair the API token model stores; a nil token denies. func tokenAuthorizes(token *models.APIToken, resourceName string, op Op) bool { - if token == nil { - return false - } - return slices.Contains(token.APIPermissions[resourceName], op.Permission()) + return token.HasPermission(resourceName, op.Permission()) } diff --git a/pkg/modules/mcp/scope_test.go b/pkg/modules/mcp/scope_test.go index 84e298b18..8276fc995 100644 --- a/pkg/modules/mcp/scope_test.go +++ b/pkg/modules/mcp/scope_test.go @@ -65,6 +65,18 @@ func TestTokenAuthorizes_NoGroup(t *testing.T) { assert.False(t, tokenAuthorizes(token, "projects", OpCreate)) } +func TestTokenAuthorizes_HyphenatedGroupKey(t *testing.T) { + // The frontend snake_cases payloads, so a hyphenated slug must still match. + token := &models.APIToken{ + APIPermissions: models.APIPermissions{ + "time-entries": []string{"read_all"}, + }, + } + + assert.True(t, tokenAuthorizes(token, "time_entries", OpReadAll)) + assert.False(t, tokenAuthorizes(token, "time_entries", OpCreate)) +} + func TestTokenAuthorizes_NilPermissionsMap(t *testing.T) { // A token with nil APIPermissions should never authorize anything. token := &models.APIToken{APIPermissions: nil}