You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Implements fine-grained admin permissions, requested in EclipseFdn/open-vsx.org#7760. Previously a user had a single global role (admin/privileged/none) and any admin action required the full admin role - there was no way to give someone narrower access as admin functionality grows.
A user can now be granted one or more individual permissions without the full admin role. admin still implies every permission, including ones added later - it stays a superuser flag, not a bundle that needs updating. See doc/permissions.md for the full reference: what each permission unlocks, which dashboard page it gates, and the REST API for managing them.
Backend
New Permission enum, one bucket per admin area (manage_namespaces, manage_extensions, manage_publishers, manage_scans, manage_consistency, manage_rate_limits, manage_caches, manage_search_index, manage_settings, view_reports), stored per-user in user_data_permission (migration V1_80), fetched via @Fetch(FetchMode.JOIN) so it costs no extra query over what a user lookup already did.
PUT /admin/user/{provider}/{loginName}/access replaces the previous POST .../role endpoint and manages permissions too. The body is the access the user should end up with - { "role": ..., "permissions": [...] } - applied in one transaction under one audit entry, so a rejected request leaves the user exactly as they were rather than half-changed. Both fields are required: a partial body is a 400, not a request to strip what it omits. Like the role endpoint before it, this stays checkAdminUser()-gated (full admin only, not delegable) - granting access is itself a privilege-escalation action.
Every other AdminService#checkAdminUser() call site across AdminAPI, ConsistencyAPI, ScanAPI, FileDecisionAPI, RateLimitAPI and ExtensionSizeOverrideAPI is generalized to checkPermission(Permission), mapped to the bucket matching that endpoint's admin-dashboard page. GET /admin/namespace/{name} takes either manage_namespaces or manage_extensions, since the Size Overrides page resolves a namespace through it before an override can be created.
SecurityConfig's /admin/** path rule loosens from hasAuthority(ROLE_ADMIN) to a check for any admin access, since the real per-endpoint decision now lives in checkPermission. AdminEndpointPermissionCheckTest walks the bytecode of every /admin-mapped handler to prove none of them forgets to authorize for itself.
Fixed a LazyInitializationException on every login this surfaced in testing: UserService#findLoggedInUser loads the user with a bare entityManager.find() outside any transaction, and every real deployment config sets spring.jpa.open-in-view: false - fine for role, a plain column, fatal for a LAZY collection. permissions is EAGER (with the @Fetch(JOIN) above to keep it cheap, since findLoggedInUser runs on nearly every authenticated request app-wide, not just admin ones).
Webui
New Access Control admin dashboard page (first entry in the Administration nav group): search for a user, edit their global role and individually granted permissions from one place. Edits are drafted and applied together on Save as a single request, with Reset to drop them; selecting a different user with unsaved edits asks before discarding. Role editing moved off Publisher admin entirely - a different concern (moderating a publisher's content) that was never really the right home for it, even before permissions existed. Gated on role === 'admin' directly, not a permission bucket, matching the backend.
The admin dashboard shell shows for any user with at least one granted permission (not only role === 'admin'), and both the sidebar/overview and each route are gated per-page by the permission that page's endpoints require - a user only ever sees entry points they can use. Cross-page links follow the same rule, and Namespace admin loads extensions from the public API for a user without manage_extensions rather than 403-ing against the admin one.
AdminService.updateUserRole/updateUserPermission are replaced by updateUserAccess(provider, login, { role, permissions }). Breaking for consumers of the published package; hasPermission and hasAnyAdminAccess are exported so a deployment with its own menu content can gate an admin entry the same way.
Extracted the user-search hook shared between Publisher admin and Access Control to src/hooks/use-admin-user-search.ts, instead of duplicating it.
Docs
Added doc/permissions.md, linked from the README: every permission, what it unlocks, what stays admin-only, and the REST API.
Test plan
./gradlew compileJava compileTestJava
./gradlew spotlessCheck
./gradlew test - full suite green, including build-and-push-server CI (Testcontainers/Postgres), which exercises the new UserDataPermissionsFetchTest regression test for the login fix
CI green: analyse (lint), build-and-push-server, build-and-push-webui (vitest), build-cli
Rebased onto latest main to resolve conflicts (the branch had drifted behind, including the license-header harmonization and the personal-access-token-via-Authorization-header work).
Conflicts were in AdminAPI.java (8 hunks - all the same shape: upstream added HttpServletRequest-based Bearer-token resolution via HttpHeadersUtil.resolveAccessToken, merged with this PR's checkPermission calls), SecurityConfig.java (the webUi.getFrontendRoutes() refactor vs. this PR's .authenticated() change), and webui/CHANGELOG.md (both added entries under ## [next]).
Also caught and fixed while rebasing:
The new migration collided with two migrations upstream added in the meantime (V1_76, V1_77) - renamed to V1_78.
AdminService#checkPermission(String, Permission) still had the Optional.of(tokenValue) NPE-on-null-token bug that upstream's checkAdminUser(String) had just been fixed to avoid (Optional.ofNullable) - ported the same fix, which two of upstream's new regression tests for that exact bug then caught immediately. Updated those two tests' expected error message to the new checkPermission text.
Re-verified after rebasing: ./gradlew compileJava compileTestJava, spotlessCheck, and the full non-Testcontainers test suite all pass (1414/1414; the 128 failures in a full ./gradlew test run are all pre-existing AbstractPostgresContainerTest/Testcontainers classes that need Docker, unavailable in this environment).
Admin functionality has grown to the point where the all-or-nothing
ADMIN role no longer fits: every admin action required full ADMIN,
with no way to give someone narrower access (e.g. only extension
moderation, or only viewing reports).
Adds a Permission enum (one bucket per admin area: namespaces,
extensions, publishers, scans, consistency, rate limits, caches,
search index, settings, reports) stored per-user in a new
user_data_permission table, grantable/revocable individually via a
new admin-only /admin/user/{provider}/{loginName}/permission endpoint.
ADMIN keeps implying every permission, including ones added later, so
it stays a superuser flag rather than being replaced.
AdminService#checkAdminUser call sites across AdminAPI, ConsistencyAPI,
ScanAPI, FileDecisionAPI and RateLimitAPI are generalized to
checkPermission(Permission), except role/permission management itself,
which stays ADMIN-only since granting access is itself a
privilege-escalation action. SecurityConfig's /admin/** path rule
loosens from ROLE_ADMIN to authenticated(), since the real
authorization decision now happens per-endpoint.
The admin dashboard shows the shell for any user with at least one
granted permission and gates each page individually, instead of
gating the whole dashboard on role === 'admin'.
Fixes two AdminAPITest cases that only passed before because the old
coarse ROLE_ADMIN security rule blocked the request before it reached
the controller: testGetUsersNotAdmin hit a URL with no mapping
(/admin/users instead of /admin/user/search), and
testDeleteExtensionNotAdmin sent no request body for an endpoint that
requires one.
Refs EclipseFdn/open-vsx.org#7760
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ement
The fine-grained permission system had no UI to actually grant or
revoke a permission - only the raw backend endpoint. Role editing was
also still on Publisher admin, which is about moderating a publisher's
content (agreements, revocations), not about granting internal admin
capabilities - a different concern that doesn't belong on the same
page, and was the wrong home for it even before permissions existed.
Add an "Access Control" page (Administration nav group) that searches
for a user and edits both their global role and their individually
granted permissions from one place. Move role editing off Publisher
admin entirely rather than duplicating it. Extract the shared
search-by-login/role hook (previously publisher-specific in name only)
to src/hooks so both pages use the same implementation.
Gated on role === 'admin' directly rather than a permission bucket,
like the backend endpoints it calls: granting access is itself
privilege escalation, not delegable.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
prettier wants this on a single line since it fits within the
project's printWidth; the multi-line form was a lint failure in CI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion on login
UserService#findLoggedInUser loads the user with a bare
entityManager.find() outside any @transactional method. Every real
deployment config sets spring.jpa.open-in-view: false, so no Hibernate
session survives past that call - fine for role, a plain column the
find() already materializes, but fatal for a LAZY collection: nothing
is left to initialize it from once a caller (UserAPI#getUserData,
AdminService#checkPermission for a non-admin user) reads it.
The test profile doesn't set open-in-view, defaulting to Spring Boot's
true, which is why nothing caught this: it never reproduces with the
session kept open for the whole request/test. Added a regression test
that forces open-in-view: false and persists/reloads a user exactly
the way findLoggedInUser does, outside any transaction.
permissions moves back into equals/hashCode, safe now that it is
EAGER and holds no back-reference to its owner the way tokens/
memberships do.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
findLoggedInUser runs on essentially every authenticated request
app-wide (UserAPI, LocalRegistryService, RegistryAPI,
TrustedPublishingAPI, AdminService) - not just admin endpoints.
EAGER's default "secondary select" strategy issued a second query on
every one of those calls to fetch a collection that is empty for
nearly all of them.
@fetch(FetchMode.JOIN) folds it into the same query as a LEFT JOIN
instead, so a user with no permissions costs the same single query as
before this field existed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents the role/permission layers added by the fine-grained admin
permissions work: what each of the 10 permissions unlocks, which
dashboard page it gates, that role/permission management itself stays
admin-only, and the underlying REST endpoints.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The sidebar and overview previously listed every built-in page
regardless of what the user could actually use, relying only on
Guard (inside each route) to show "not authorized" for the rest.
Add an optional permission/adminOnly to RouteEntry, set to match
each route's existing Guard exactly, and filter navConfig by it
before building the sidebar/overview - dropping a group entirely
once none of its pages remain visible. Contributed pages are
unaffected: they carry neither field, so they always show, as
before. Route-level Guard enforcement is unchanged; this only
affects what is listed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Granted permissions never reached the Access Control page. The user
search goes through a jOOQ query that selects user_data columns only,
so the UserData objects it builds carry an empty permission set: every
checkbox rendered unchecked, revoking was impossible, and re-granting
answered 400 "already has the permission". The service now loads the
page's users as mapped entities to fill them in, one query per page.
Permission-only users had no way into the dashboard either - both
account menus still gated the entry on role === 'admin', as did the
review moderation UI, whose endpoint now takes manage_extensions. The
predicate behind all of them moves to src/permissions.ts and is
exported, so a deployment with its own menu content can use it too.
/admin/** goes from .authenticated() back to a real gate: callers must
hold the admin role or some permission. It reads the user per request
rather than going by login-time authorities, so a granted permission
takes effect immediately. A new test walks the bytecode of every
handler mapped under /admin and fails if one never reaches
checkPermission/checkAdminUser, which is what the coarse gate cannot
catch on its own.
Also: forgetUser left permissions on the anonymized row while clearing
the role; a failed role change reverted to the role the search result
loaded with rather than the one in effect; the permission endpoint had
no endpoint-level tests, including that holding a permission does not
let you grant one; and the new regression test leaked its fixture into
the shared database.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Alongside the Copilot review above (each finding answered in thread), I ran an independent adversarial review of this PR. It confirmed the user-search bug as the top issue and raised four things the first review did not. Everything below is in 846a038.
Raised only by the second pass
/admin/** had lost its filter-level backstop. This PR had dropped the rule from hasAuthority("ROLE_ADMIN") to .authenticated(), because a permission-only user has no authorities at all (OAuth2UserServices.getAuthorities derives them purely from the role) and would otherwise be rejected before any handler could run. The audit found no endpoint actually open today, but two consequences: a future handler that forgets its check becomes reachable by any logged-in user, and request body deserialization, validation and ReadOnlyEndpointAspect now run before authorization. The second one was visible in this PR's own diff, where a non-admin delete test had to gain a request body because it started returning 400 from body resolution instead of 403.
Both are addressed. /admin/** is gated again, on an AuthorizationManager requiring the admin role or at least one permission. It resolves the user per request rather than using login-time authorities, so granting someone a permission takes effect immediately instead of after a re-login - the staleness that usually comes with putting this in the authorities. Admins short-circuit on the existing ROLE_ADMIN authority, so the common case costs no lookup. The test that had to gain a body has been reverted to sending none, which now asserts the gate refuses before deserialization.
Since a coarse gate cannot tell whether a given handler checks the right capability, AdminEndpointPermissionCheckTest walks the bytecode of every handler mapped under /admin and fails if one never reaches checkPermission/checkAdminUser, following same-class delegation so the report endpoints' private helper still counts. I verified it fails as intended by removing one check and watching it name ConsistencyAPI#listChecks.
forgetUser left permissions on the tombstone. The anonymize branch clears the role and every identifying field, but the new user_data_permission rows survived, so a forgotten user remained an account holding admin capabilities - and one that ?role=none would list as having no role at all. Now cleared alongside the role, with a test.
A failed role change reverted to the wrong value.onError reset to user.role from the search result, which is never refetched while the component stays mounted. None to Privileged (succeeds) to Admin (fails) left the toggle showing "No role" while the server held privileged. It now reverts to the role in effect at the time of the change, with a regression test.
Found while fixing
RepositoryServiceSmokeTest asserts that every public RepositoryService method is exercised, so the new findUsersById had to be registered there - it would have failed in CI only, since that suite needs Docker.
Still open
The jOOQ codegen for user_data_permission (being run separately - it needs a live migrated Postgres), and the provider-ambiguous deep link, which I have argued in thread is better fixed across both admin search pages at once.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Permission-specific navigation has inaccessible cross-links, a security regression test does not reach its intended authorization layer, and one linked acceptance criterion remains unresolved.
The linked issue explicitly lists “Base role assigned to all users” as an acceptance criterion, but this migration only adds the permission table and the new documentation still defines users with no role. Existing and newly created users therefore remain nullable-role accounts. Either add and migrate a documented base role, or clarify in the linked requirement that the implicit no-role state is the intended base role.
The caller authenticated with user("test_user"), whose principal is a
plain UserDetails, so the /admin/** gate refused the request before
checkAdminUser ever ran. The assertion held for the wrong reason:
replacing the endpoint's checkAdminUser with a delegable
checkPermission - the exact escalation the test exists to catch - left
the whole suite passing.
It now authenticates as an IdPrincipal holding a permission, which the
gate admits, and asserts the error body as well, since a refusal by
the gate carries none. Verified against the same mutation: the test
fails, where before it did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bytecode walk recorded only the called method's name, so a handler
calling anything named checkPermission would have satisfied a test whose
whole point is that AdminService was reached. It now matches the call's
owner too; verified by pointing it at a different owner and watching the
test fail.
On Access Control, MUI syncs the search box to the option just picked,
which happens before the discard confirmation is answered. Declining left
the box naming the rejected user while the card below still showed the one
being edited, so that sync is now skipped while the draft is dirty - the
confirmed switch sets the box itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…page
The admin namespace payload names every extension, inactive and
soft-deleted ones included, so a viewer without manage_extensions - who now
loads them through the public API - hit an error dialog for each one the
public API has nothing to return for.
An extension that viewer cannot see is an absence rather than a failure, so
the extension list gains omitUnavailable: the entry is left out and no
error is reported. Only that branch sets it; everywhere else an
unresolvable extension is still worth reporting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addressed the three "previously missed" items from the latest review.
Restrict authorization-call detection to AdminService (AdminEndpointPermissionCheckTest) — correct, and it mattered: the walk recorded only the called method's name, so any method named checkPermission would have satisfied a test whose whole claim is that the handler reaches AdminService. It now matches the call's owner too. I verified the strengthened version actually bites by pointing ADMIN_SERVICE at a different owner and watching everyHandlerUnderAdminAuthorizesForItself fail, then restored it. All nine real call sites across AdminAPI, ConsistencyAPI, ScanAPI, FileDecisionAPI, RateLimitAPI, ExtensionSizeOverrideAPI and DownloadBackfillAPI go through an AdminService field, so nothing legitimate regressed.
Handle MUI reset events when rejecting dirty user switches — confirmed. MUI syncs the input to the picked option's label before the confirmation is answered, so "Keep editing" left the box naming the rejected user while the card below still showed the one being edited. That sync is now skipped while the draft is dirty; the confirmed switch sets inputValue itself, so nothing else needed to change. Added the selection-based case alongside the existing Clear one.
Filter inactive extensions from the namespace fallback — real, but the suggested fix is not implementable as written: the client cannot filter to active entries, because the admin payload is a bare name → url map with no active flag. I confirmed the mismatch server-side (LocalRegistryService#getNamespace(name, true) uses findAllExtensionNames, while the public detail fetch resolves only active versions).
Fixed from the other end instead: an extension this viewer cannot see is an absence, not a failure. NamespaceExtensionList gains omitUnavailable, which leaves the entry out without reporting it, and only the public-fallback branch on Namespace admin sets it — everywhere else an unresolvable extension is still worth a dialog. The list already dropped failed entries, so the change is confined to the error path.
Worth noting why I did not trim the payload server-side per permission, which was the other obvious route: the "Delete namespace" button appears only when the payload's extension map is empty, so a trimmed map would offer Delete for a namespace that still holds inactive extensions.
These case conversions use the JVM default locale. Under a Turkish locale, permission names containing i convert to dotted/dotless-I variants, so API parsing and persisted-value loading fail (and serialization can emit invalid names). Use locale-independent enum conversion in both directions.
omitUnavailable suppressed every fetch failure, so a network error or a 5xx
would have dropped cards silently and left a partial list looking like the
whole namespace. Only the 404 that means "this viewer cannot see it" is
omitted now; everything else still reaches handleError.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On the locale finding (Permission.java:32, "Use locale-independent permission enum case conversion"): not acting on this one.
The analysis is technically right — toUpperCase()/toLowerCase() follow the JVM default locale, and under tr-TR a name carrying an i does fail to resolve. I had a fix and a regression test that reproduced it, so the behaviour is confirmed rather than assumed.
It is being left as is deliberately: the server runs under a known locale, so the failing configuration does not arise in practice. The same pattern has been in UserData.Role#valueOfIgnoreCase since #1847 for the same reason.
forgetUser, revokePublisherContributions and revokePublisherTokens moved
from checkAdminUser() to checkPermission(MANAGE_PUBLISHERS) in this branch.
All three write the state updateUserAccess deliberately reserves for full
admins - forgetUser clears the target's role and permissions outright - so
a MANAGE_PUBLISHERS holder could enumerate admins through the user search,
forget each one, and leave an installation with nobody able to grant access
back.
They now refuse a target holding any role or permission unless the caller
is a full admin. Ordinary publisher support, which is what the capability
is for, is unaffected.
The endpoint scan could not catch this: it proves a handler calls some
check, never which Permission it passes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Permission serialization is also locale-sensitive. Under a Turkish locale, enum names containing I are emitted with a dotless ı, corrupting database values and API responses so they no longer match the documented permission names. Normalize with Locale.ROOT.
Bound deep-link user search to prevent unbounded requests
A deep link automatically fetches every matching page until it finds an exact login. Because the server search is a substring match over login and full name and pages contain only 25 users, a valid short login—or a missing/generic URL—can trigger an unbounded sequence of requests and retain the entire result set. Resolve deep links through an exact user lookup (or otherwise bound this scan) instead of walking all search pages.
Access Control kept sending the drafted permissions when the role being
saved was admin, even though the checkboxes are disabled and the page says
the grants do not apply. They were stored, and resurfaced as standalone
grants the next time anyone demoted the user. Only what is saved and
compared is emptied now; the draft survives so changing the role back
restores the ticks.
Deep-link resolution pulled another page whenever the exact login was
absent, with no cap - a short or stale link substring-matches nearly every
account, so it walked the whole registry behind an empty placeholder.
Re-picking the user already on screen offered to discard a draft it would
not have discarded: MUI only skips onChange for the identical option
object, and a refetch replaces those, while the details card is keyed on
provider/login and never remounts.
searchUsers collected permissions with the two-argument toMap, which an
entity returned twice by a join fetch would answer with an
IllegalStateException - a 500 on the whole user search.
PermissionConverter threw on a stored value this build has no constant
for, e.g. a grant left behind by a rolled-back release. The collection is
EAGER on the path every authenticated request takes, and the security
filter chain loads the same row, so that was a 500 on everything that user
did. It now reads as no permission, with a warning naming the value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard added for the privilege hole refuses to revoke or forget a user
holding a role or permissions unless the caller is a full admin, but
Publisher admin still rendered all three buttons to everyone - so a
delegated manage_publishers user got a confirmation dialog that could only
end in a 403.
The page now shows why instead, using the target's access already carried
in the search result. The danger zone moves into its own component rather
than growing another nesting level around the existing markup.
doc/permissions.md said manage_publishers covers revocation and erasure
without the exception, which is now stated where the rule is explained.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The details card heading carries the login as well, so findByText('octocat')
matched two elements and the test errored instead of asserting.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The extracted component used a line layout prettier rejects, in three
places it reflows itself. Applied exactly what the hook asked for.
The new tests matched "Forget user" by text, which hits both the section
heading and the button, so they errored on ambiguity rather than asserting.
Queried by role instead, which also distinguishes the control from its
label.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Document admin access for analytics downloads backfill endpoint
doc/permissions.md:47
This “admin-only” reference omits POST /admin/api/analytics/downloads/backfill, which still calls checkAdminUser for both session and token authentication (DownloadBackfillAPI.java:72-113). Documenting only access changes and Publisher restrictions makes the section incomplete for operators deciding which actions can be delegated.
The section reads as a complete list of what cannot be delegated, and the
backfill endpoint is the third thing on it - admin role by session or
token, unchanged by the permission split because it has no dashboard page
for a permission to map to.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fair — the section reads as a complete list of what cannot be delegated, and that endpoint is a third entry on it. Documented in 25c09fe.
For the record on what it is rather than what it looked like: POST /admin/api/analytics/downloads/backfill is not a regression or an inconsistency introduced here — this branch never touched DownloadBackfillAPI (git diff <merge-base>..HEAD -- server/src/main/java/org/eclipse/openvsx/analytics/ is empty). It was admin-only before the permission split and still is.
It was left out of the conversion because every permission maps to an admin dashboard page and this endpoint has none; it ingests an access-log file with an admin token. view_reports would have been the nearest bucket and is the wrong one, since that covers reading reports rather than writing analytics data. The new paragraph says exactly that, so the omission reads as a decision.
After the conversion, two call sites still require the role: PUT /admin/user/{provider}/{loginName}/access, deliberately, and this one.
No access change either way — the endpoint sits under the permitAll/admin/api/** matcher, so the handler's own checkAdminUser is its only gate; the token path goes through useAccessToken with an Administration action, which rejects namespace- and extension-scoped tokens, and an absent token resolves to a null user and 403s.
- Soften PublishingConfig#maxOverrideSize's javadoc from "hard ceiling" to
"write-time ceiling", matching doc/configuration.md's existing caveat that
lowering it does not reach a default or override already stored above it.
- The Size overrides page requires manage_extensions, but sourced its
client-side ceiling from GET /admin/settings, which requires
manage_settings - a permission split the fine-grained admin roles (#2269)
make real. A user with only manage_extensions got a silent 403 there and
never saw the validation. Moves maxOverrideSize onto
GET /admin/size-overrides (SizeOverrideListJson), which the page already
has permission for.
- extension-registry-types.ts: fix "the default above" reading as a noun
phrase in the Settings.maxOverrideSize doc comment.
- settings.tsx: stop flooring the ceiling's MB conversion in the validation
message - a 1.5 MiB ceiling reported "at most 1 MB", understating what the
server actually accepts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ze ceiling (#2303)
* fix: validate publish size limits against the scanning limit
Restores ovsx.publishing.max-override-size (removed in #2297 because it
bounded only overrides, leaving the registry-wide default unbounded) as a
hard, service-wide ceiling: it now bounds both the admin-adjustable default
and every size override, so neither path can drift past it independently.
Startup additionally fails if that ceiling exceeds
ovsx.scanning.max-archive-size-bytes, since scanning silently skips whatever
is past its own archive-size limit instead of failing closed - closing the
gap in #2302 where a publish-accepted package could be larger than what
scanning actually inspects.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: drop the scan-limit cross-check, it compared the wrong units
Review feedback on #2303: the publish-side ceiling bounds the compressed
.vsix size on disk (ExtensionService#createExtensionFile writes the upload
as-is), while ovsx.scanning.max-archive-size-bytes bounds the sum of
uncompressed ZIP entry sizes (ArchiveUtil#enforceArchiveLimits reads
ZipEntry#getSize, which the API defines as uncompressed). Comparing the two
at startup doesn't guarantee a package under the ceiling is fully scanned -
a small, highly-compressible package can still exceed the uncompressed scan
limit. Closing that gap for real needs a different approach, so the
cross-check is dropped rather than left making a claim it can't back up.
Also restores the "whole number of bytes" wording the settings page's size
validation message dropped when the ceiling range was added to it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: remove stale scan-validation claims left in docs/comments
Review feedback: PublishingConfig#maxOverrideSize's javadoc and a
PublishingConfigTest comment still claimed the startup check against
ovsx.scanning.max-archive-size-bytes, which was already dropped in the
previous commit. Reword both to describe only what's actually enforced now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: close review findings on permission mismatch and wording
- Soften PublishingConfig#maxOverrideSize's javadoc from "hard ceiling" to
"write-time ceiling", matching doc/configuration.md's existing caveat that
lowering it does not reach a default or override already stored above it.
- The Size overrides page requires manage_extensions, but sourced its
client-side ceiling from GET /admin/settings, which requires
manage_settings - a permission split the fine-grained admin roles (#2269)
make real. A user with only manage_extensions got a silent 403 there and
never saw the validation. Moves maxOverrideSize onto
GET /admin/size-overrides (SizeOverrideListJson), which the page already
has permission for.
- extension-registry-types.ts: fix "the default above" reading as a noun
phrase in the Settings.maxOverrideSize doc comment.
- settings.tsx: stop flooring the ceiling's MB conversion in the validation
message - a 1.5 MiB ceiling reported "at most 1 MB", understating what the
server actually accepts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: show the exact byte ceiling instead of a rounded MB approximation
Review feedback: rounding the MB conversion can overstate the ceiling near
a boundary - 2 MiB - 1 byte rounded to "2.00 MB", naming a value (2 MB) the
server actually rejects. The exact byte count is now the authoritative part
of the validation message, with the MB figure only an approximation
alongside it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <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
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.
Summary
Implements fine-grained admin permissions, requested in EclipseFdn/open-vsx.org#7760. Previously a user had a single global
role(admin/privileged/none) and any admin action required the fulladminrole - there was no way to give someone narrower access as admin functionality grows.A user can now be granted one or more individual permissions without the full
adminrole.adminstill implies every permission, including ones added later - it stays a superuser flag, not a bundle that needs updating. Seedoc/permissions.mdfor the full reference: what each permission unlocks, which dashboard page it gates, and the REST API for managing them.Backend
Permissionenum, one bucket per admin area (manage_namespaces,manage_extensions,manage_publishers,manage_scans,manage_consistency,manage_rate_limits,manage_caches,manage_search_index,manage_settings,view_reports), stored per-user inuser_data_permission(migrationV1_80), fetched via@Fetch(FetchMode.JOIN)so it costs no extra query over what a user lookup already did.PUT /admin/user/{provider}/{loginName}/accessreplaces the previousPOST .../roleendpoint and manages permissions too. The body is the access the user should end up with -{ "role": ..., "permissions": [...] }- applied in one transaction under one audit entry, so a rejected request leaves the user exactly as they were rather than half-changed. Both fields are required: a partial body is a 400, not a request to strip what it omits. Like the role endpoint before it, this stayscheckAdminUser()-gated (full admin only, not delegable) - granting access is itself a privilege-escalation action.AdminService#checkAdminUser()call site acrossAdminAPI,ConsistencyAPI,ScanAPI,FileDecisionAPI,RateLimitAPIandExtensionSizeOverrideAPIis generalized tocheckPermission(Permission), mapped to the bucket matching that endpoint's admin-dashboard page.GET /admin/namespace/{name}takes eithermanage_namespacesormanage_extensions, since the Size Overrides page resolves a namespace through it before an override can be created.SecurityConfig's/admin/**path rule loosens fromhasAuthority(ROLE_ADMIN)to a check for any admin access, since the real per-endpoint decision now lives incheckPermission.AdminEndpointPermissionCheckTestwalks the bytecode of every/admin-mapped handler to prove none of them forgets to authorize for itself.LazyInitializationExceptionon every login this surfaced in testing:UserService#findLoggedInUserloads the user with a bareentityManager.find()outside any transaction, and every real deployment config setsspring.jpa.open-in-view: false- fine forrole, a plain column, fatal for aLAZYcollection.permissionsisEAGER(with the@Fetch(JOIN)above to keep it cheap, sincefindLoggedInUserruns on nearly every authenticated request app-wide, not just admin ones).Webui
role === 'admin'directly, not a permission bucket, matching the backend.role === 'admin'), and both the sidebar/overview and each route are gated per-page by the permission that page's endpoints require - a user only ever sees entry points they can use. Cross-page links follow the same rule, and Namespace admin loads extensions from the public API for a user withoutmanage_extensionsrather than 403-ing against the admin one.AdminService.updateUserRole/updateUserPermissionare replaced byupdateUserAccess(provider, login, { role, permissions }). Breaking for consumers of the published package;hasPermissionandhasAnyAdminAccessare exported so a deployment with its own menu content can gate an admin entry the same way.src/hooks/use-admin-user-search.ts, instead of duplicating it.Docs
doc/permissions.md, linked from the README: every permission, what it unlocks, what staysadmin-only, and the REST API.Test plan
./gradlew compileJava compileTestJava./gradlew spotlessCheck./gradlew test- full suite green, includingbuild-and-push-serverCI (Testcontainers/Postgres), which exercises the newUserDataPermissionsFetchTestregression test for the login fixanalyse(lint),build-and-push-server,build-and-push-webui(vitest),build-cli🤖 Generated with Claude Code