Check per-table read permission in lookup table tester - #26915
Open
dennisoelkers wants to merge 7 commits into
Open
Check per-table read permission in lookup table tester#26915dennisoelkers wants to merge 7 commits into
dennisoelkers wants to merge 7 commits into
Conversation
The lookup table tester endpoints only required the generic `lookuptables:read` permission (and the `GET` variant required no permission at all beyond authentication). A user without read access to a specific lookup table could therefore use the tester to extract data from it. Both endpoints now resolve the lookup table and check `lookuptables:read` for its id before performing the lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers both the GET and the POST endpoint: a user is only allowed to run the tester against a lookup table if they hold `lookuptables:read` for that table's id. Neither an unscoped `lookuptables:read` nor a grant for a different table is sufficient. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`LookupTableService#getTable` returns `null` for tables which aren't loaded, so the new permission check threw a `NullPointerException` (500) instead of the "doesn't exist" response for unknown table names. Skipping the instance-scoped check in that case doesn't expose any data: the lookup itself resolves the table through the same service and returns an error result when it isn't loaded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: This needs to be backported to previous, supported version.
Description
Motivation and Context
Prior to this PR, a user without read access to a specific lookup table could use the tester endpoint to extract data from it, due to a missing permissions check.
The lookup table tester endpoints (
GET/POST /tools/lookup_table_tester) now resolve the requested lookup table and checklookuptables:readfor its id before performing the lookup.Previously the
GETvariant required no permission beyond authentication, and thePOSTvariant only required the unscopedlookuptables:readpermission. Since the instance-scoped check now covers both endpoints, the unscoped@RequiresPermissionsannotation onPOSTwas dropped, so users who only hold a grant for an individual table can use the tester for that table.Table names which don't resolve to a loaded lookup table keep returning the existing "doesn't exist" response instead of failing the permission check, which cannot leak data: the lookup resolves the table through the same service and returns an error result when it isn't loaded.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: