fix: one copy of each @valtimo library in release bundles, value resolver 403, OperatonTask modify action - #903
Open
Klaas-Ritense wants to merge 10 commits into
Conversation
Opening a plugin configuration (*Enter data*) or a plugin action configuration in the released gzac-frontend / valtimo-frontend images failed with NullInjectorError: No provider for InjectionToken Provide plugin config; an array of plugin specifications. and the step then rendered only the generic fields instead of the plugin's own fields. On the action configuration step the same defect surfaced as `No provider for InjectionToken valtimoConfig`. The release bundle contained two compiled copies of every @valtimo/* library, each with its own InjectionToken. The release job in frontend_build_push_docker_image.yml deletes `workspaces` and rewrites @valtimo/* to `file:dist/valtimo/*` so the npm-installed @valtimo-plugins/* packages can resolve them through node resolution, while the application itself still resolved the same libraries to projects/valtimo/* source through the `paths` in tsconfig.json. Two resolution routes produced two instances, so the token the app module provided was not the one the plugin packages injected. Each app now has a tsconfig.app.prod.json that extends tsconfig.app.json and overrides `paths` with only the @src and @e2e/constants entries, dropping the 42 @valtimo/* mappings, and each app's `production` build configuration points at it. The app therefore resolves @valtimo/* exactly like the plugin packages do. This is a build-level defect, so it affected every plugin in the images, not a specific one; frontends that install @valtimo/* from npm (such as gzac-frontend-template and the images built from it) were never affected. Local development and unit tests are untouched: `serve` uses the unconfigured build target and karma uses tsconfig.spec.json. Because the app now bundles @valtimo/shared from the compiled dist, the `Stamp frontend version` step no longer reached the bundle and a release would have reported the committed 13.0.1. It now stamps every frontendLibraries constant it finds, in projects/valtimo source and in dist/valtimo, so both this and branches that still bundle the libraries from source keep working. Also fixes the value resolver management endpoints returning 403 for a ROLE_ADMIN token, which left the *Value resolver* input mode without options. The endpoints were subject to entity-level authorization on top of the administrator check that already guards them in ValueResolverHttpSecurityConfigurer and BuildingBlockHttpSecurityConfigurer, so they are now annotated with @RunWithoutAuthorization. Access remains restricted to administrators. The value-resolver module gained an `implementation` dependency on :backend:authorization for the annotation; authorization does not depend on value-resolver, so this introduces no cycle.
OperatonTaskService.setDueDate and removeDueDate both call requirePermission(task, MODIFY), but OperatonTaskActionProvider did not list MODIFY among its available actions. PbacRegistryService builds the registry that drives the *Access control* screens from those lists, so `modify` was not offered for the OperatonTask resource type and the permission could not be granted through the admin screens. Also brings the evenementenvergunning demo permissions up to date (changeset permissions-v4). The entries now use the `actions` array instead of the `action` field that has been deprecated since 12.2.0, which collapses the per-action duplicates into one entry per resource type, and the set is extended to cover the resource types the app actually uses: Dashboard, ZgwDocument, IkoView, ObjectManagement, ResourcePermission, Team, User, OperatonTask, OperatonTimer and Zaak. Every action granted is one its resource's action provider offers, including the OperatonTask `modify` above.
Contributor
|
Warning Review limit reached
Next review available in: 84 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBackend permission configuration now uses global role and permission files, while the previous PBAC files are removed. Value-resolver endpoints and a building-block endpoint bypass authorization checks. The value-resolver module adds a compile-only authorization dependency. Operaton task actions now include 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c78a81e5-db39-4d21-99a9-99a610baa03b
📒 Files selected for processing (11)
.github/workflows/frontend_build_push_docker_image.ymlbackend/apps/evenementenvergunning/src/main/resources/config/pbac/all.permission.jsonbackend/building-block/src/main/kotlin/com/ritense/buildingblock/web/rest/BuildingBlockValueResolverResource.ktbackend/core/src/main/kotlin/com/ritense/valtimo/operaton/authorization/OperatonTaskActionProvider.ktbackend/value-resolver/build.gradlebackend/value-resolver/src/main/kotlin/com/ritense/valueresolver/web/rest/ValueResolverResource.ktfrontend/angular.jsonfrontend/apps/dev/src/tsconfig.app.prod.jsonfrontend/apps/evenementenvergunning/src/tsconfig.app.prod.jsonfrontend/apps/gzac/src/tsconfig.app.prod.jsonfrontend/apps/valtimo/src/tsconfig.app.prod.json
The permissions and roles were deployed by the changeset deployers, which only save what a changeset contains. Bumping the changeset added the new permissions but left the rows of the previous changeset in place, so a permission dropped from the file stayed active after deployment — the concern CodeRabbit raised on this PR. Both files now use the format that GlobalPermissionImporter and GlobalRoleImporter read: a bare JSON array under config/global, with no changesetId wrapper. This is the format the dev app already uses, and PermissionDeployer has been deprecated in favour of the importer since 13.18.0. The importer reconciles: it saves what is new and, in afterImport, deletes the existing rows that the imported set does not contain. No library module ships permission files, so the app's own file is the only source and reconciling against the whole table is safe. The role file had to move as well. CaseDefinitionDeploymentService runs deployGlobal() before changelogDeployer.deployAll(), so with the roles left on the changeset deployer the permission import would run first and PermissionDto.toPermission would fail with "Missing role 'ROLE_ADMIN'" on an empty database. With both files under config/global they are handled in one pass, and the importers are topologically ordered, so GLOBAL_ROLE runs before GLOBAL_PERMISSION, which declares it in dependsOn(). `valtimo.changelog.pbac.clear-tables` matters here too: PermissionDeployer clears the table in before(), which runs after deployGlobal(), so it would wipe the imported rows. Only the valtimo app sets it, and that app keeps the changeset format, so it is unaffected. Verified with ./gradlew :backend:authorization:integrationTestingPostgresql — 77 tests, 0 failures, including GlobalPermissionImporterIntTest and GlobalRoleImporterIntTest.
…path ValueResolverResourceIT failed to load its application context: Error creating bean 'valtimoAuthorizationService' defined in AuthorizationAutoConfiguration Caused by: NoSuchBeanDefinitionException: No qualifying bean of type 'com.ritense.valtimo.contract.authentication.UserManagementService' Declaring :backend:authorization as `implementation` put it on the module's runtime classpath, so AuthorizationAutoConfiguration was discovered in the value-resolver test context. Its valtimoAuthorizationService needs a UserManagementService, which an IAM module supplies and this context has none of. The dependency exists only so @RunWithoutAuthorization can be compiled, so it is now `compileOnly` — the same pattern smartdocuments uses for :backend:plugin and zaken-api for :backend:form-flow. That keeps the annotation available at compile time while leaving every runtime classpath exactly as it was before, so the seventeen modules that depend on value-resolver do not start seeing AuthorizationAutoConfiguration. The annotation still takes effect: Kotlin's default retention is RUNTIME, and `javap` confirms RuntimeVisibleAnnotations on all three endpoints carries com.ritense.authorization.annotation.RunWithoutAuthorization, so the aspect matches in the applications that host these endpoints — all of which have :backend:authorization by way of valtimo-dependencies.
8.5.2 leaves the FreeMarker action configuration step (generate-mail-file / generate-mail-content) stuck on the loading spinner: loading$.next(false) only ran inside the merged context stream, and both sources were filtered away when there is no case or building-block context — which is what happens when the action is configured from Admin -> Processes. 8.5.3 fixes that and is published on npm. The duplicate-library fix in this branch removes the NullInjectorError so the configuration screens mount at all, but without this bump the action step would mount and then spin forever. Note that the `latest` dist-tag on this package points at 8.5.2-V12, a v12 build published after 8.5.3, so the exact pin here matters.
The frontend pin was moved to 8.5.3 but freemarkerPluginVersion still resolved com.ritense.valtimoplugins:freemarker 8.5.2 for the gzac, dev and evenementenvergunning apps, so the backend images would have shipped the old version while the frontend images shipped the new one. 8.5.3 is on Maven Central. The artifact is pom-only at every version (there is no freemarker-<version>.jar), and it resolves on the gzac runtime classpath; all three apps compile.
Picks up the additional bugfix released in 8.5.4, on both sides so the backend and frontend images stay in lockstep: - frontend/release-plugins.json: the npm pin the release image build applies - gradle.properties: com.ritense.valtimoplugins:freemarker for the gzac, dev and evenementenvergunning apps 8.5.4 is on npm. At the time of this commit the backend artifact had not yet appeared on Maven Central (the pom still 404s, metadata unchanged since 8.5.3), so the backend build will not resolve it until Central syncs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bfc380b8-2ae6-4afc-ad4c-92ab32c0c317
📒 Files selected for processing (7)
backend/apps/evenementenvergunning/src/main/resources/config/global/permission/all.permission.jsonbackend/apps/evenementenvergunning/src/main/resources/config/global/role/all.role.jsonbackend/apps/evenementenvergunning/src/main/resources/config/pbac/all.permission.jsonbackend/apps/evenementenvergunning/src/main/resources/config/pbac/all.role.jsonbackend/value-resolver/build.gradlefrontend/release-plugins.jsongradle.properties
💤 Files with no reviewable changes (2)
- backend/apps/evenementenvergunning/src/main/resources/config/pbac/all.permission.json
- backend/apps/evenementenvergunning/src/main/resources/config/pbac/all.role.json
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/value-resolver/build.gradle
…i-duplicate-libraries-and-config-fixes
…ergunning The global permission file granted ROLE_USER create/modify/delete/assign on com.ritense.team.domain.Team and view/view_list on com.ritense.valtimo.contract.authentication.User. Both entries are removed, so every entry in the file now targets ROLE_ADMIN. Addresses the review comment on this PR (#903 (comment)). Note that both entries were carried over verbatim from the pre-refactor config/pbac/all.permission.json, which granted exactly the same two ROLE_USER permissions. This therefore narrows the app's access control rather than correcting a widening introduced by this PR. No role holds Team or User permissions after this change, and GlobalPermissionImporter.afterImport deletes the rows that the imported set does not contain, so those permissions are removed on the next deployment. ROLE_USER itself stays declared in config/global/role/all.role.json.
backend/apps/valtimo set valtimo.changelog.pbac.clear-tables: true, which made RoleDeployer and PermissionDeployer truncate the role and permission tables before deploying a changeset. The property is removed so the default applies. Both readers already default to false (AuthorizationAutoConfiguration @value("${valtimo.changelog.pbac.clear-tables:false}")), as does the only other property in the family, valtimo.changelog.dashboard.clear-tables. The gzac and evenementenvergunning apps never set either one, so no change was needed there.
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
2 participants
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.
EC: https://github.com/generiekzaakafhandelcomponent/atlas-internal/issues/659
Plugin configuration screens in the released frontend images
Opening a plugin configuration (Enter data) or a plugin action configuration in
gzac-frontend/valtimo-frontendfailed withand the step then rendered only the generic fields instead of the plugin's own fields. On the
action configuration step the same defect surfaced as
No provider for InjectionToken valtimoConfig.Cause. The release bundle contained two compiled copies of every
@valtimo/*library, eachwith its own
InjectionToken. The release job deletesworkspacesand rewrites@valtimo/*tofile:dist/valtimo/*so the npm-installed@valtimo-plugins/*packages can resolve them throughnode resolution, while the app itself still resolved the same libraries to
projects/valtimo/*source through the
pathsintsconfig.json. Two resolution routes, two instances, so the tokenthe app module provided was not the one the plugin packages injected.
Fix. Each app gets a
tsconfig.app.prod.jsonthat extendstsconfig.app.jsonand overridespathswith only the@srcand@e2e/constantsentries — dropping the 42@valtimo/*mappings —and each app's
productionbuild configuration points at it. The app then resolves@valtimo/*exactly like the plugin packages do.
This is a build-level defect, so it affected every plugin in the images rather than a specific one,
and it is not a 13.41.0 regression (13.40.0 has the same structure). Frontends that install
@valtimo/*from npm —gzac-frontend-templateand the images built from it, including thoseproduced by
valtimo-configurator— were never affected. Local development and unit tests areuntouched:
serveuses the unconfigured build target and karma usestsconfig.spec.json.Follow-on change in the workflow. Because the app now bundles
@valtimo/sharedfrom thecompiled
dist, theStamp frontend versionstep no longer reached the bundle and a release wouldhave reported the committed
13.0.1. It now stamps everyfrontendLibrariesconstant it finds, inprojects/valtimosource and indist/valtimo, so this and branches that still bundle thelibraries from source both keep working.
Value resolver management endpoints returned 403 for admins
GET /api/management/v1/value-resolverand the case-definition and building-block key endpointsreturned 403 for a
ROLE_ADMINtoken, leaving the Value resolver input mode without options. Theywere subject to entity-level authorization on top of the administrator check that already guards
them in
ValueResolverHttpSecurityConfigurerandBuildingBlockHttpSecurityConfigurer, so all fourare now annotated
@RunWithoutAuthorization. Access remains restricted to administrators.value-resolvergained animplementationdependency on:backend:authorizationfor theannotation;
authorizationdoes not depend onvalue-resolver, so there is no cycle.modifyaction for OperatonTaskOperatonTaskService.setDueDateandremoveDueDateboth callrequirePermission(task, MODIFY), butOperatonTaskActionProviderdid not listMODIFY.PbacRegistryServicebuilds the registry behindthe Access control screens from those lists, so
modifywas never offered for theOperatonTaskresource type and the permission could not be granted through the admin screens.
evenementenvergunning permissions moved to the global importer
The demo app's permissions and roles were deployed by the changeset deployers, which only save what
a changeset contains — bumping the changeset added new permissions but left the previous changeset's
rows in place, so a permission dropped from the file stayed active. Both files now use the format
GlobalPermissionImporter/GlobalRoleImporterread: a bare JSON array underconfig/global, nochangesetIdwrapper. This is what the dev app already does, andPermissionDeployerhas beendeprecated in favour of the importer since 13.18.0. The importer reconciles — it saves what is new
and, in
afterImport, deletes the existing rows the imported set does not contain.Along the way the entries stop using the
actionfield deprecated since 12.2.0, which collapses theper-action duplicates into one entry per resource type, and the set is extended to the resource types
the app actually uses (Dashboard, ZgwDocument, IkoView, ObjectManagement, ResourcePermission, Team,
User, OperatonTask, OperatonTimer, Zaak).
Notes on why the role file moved too, and on
clear-tables:CaseDefinitionDeploymentServicerunsdeployGlobal()beforechangelogDeployer.deployAll().Leaving the roles on the changeset deployer would make the permission import run first and
PermissionDto.toPermissionfail withMissing role 'ROLE_ADMIN'on an empty database. With bothfiles under
config/globalthey are handled in one pass, and the importers are topologicallyordered so
GLOBAL_ROLEruns beforeGLOBAL_PERMISSION, which declares it independsOn().PermissionDeployer.before()clears the table whenvaltimo.changelog.pbac.clear-tablesis set,and it runs after
deployGlobal(), so it would wipe imported rows. Only the valtimo app setsthat flag, and that app keeps the changeset format, so it is unaffected. Worth knowing before
converting the valtimo app too.
and reconciling against the whole table is safe.
Verification
./gradlew testAll— green: 3067 tests, 0 failures, 0 errors, 1 skipped. Changed modules:core246,building-block202,value-resolver27, all passing../gradlew :backend:authorization:integrationTestingPostgresql— green: 77 tests, 0 failures,including
GlobalPermissionImporterIntTestandGlobalRoleImporterIntTest, which cover the formatthe permission and role files were moved to.
components52,decision18,layout13,task11,case5,access-control5,process4,form1.(resourceType, action)pair in the permission file was checked against itsResourceActionProvider; all are offered. Both file paths were checked against the importers'filename regexes.
main-X7M7SIAT.jsand a local baseline rebuild both contain the token twice and reproduce theerror at the same step; with the fix each of the four app variants builds clean, contains exactly
one provided token, and Mail Templates / Document generator / Text template all render. The
version stamp lands in the bundle.
Not verified here: the
ng test devkarma target hangs before launching a browser (0% CPU, nobrowser process, >11 min). Reproduced identically on a clean
origin/next-minorworktree, so it ispre-existing and unrelated — that target compiles the whole app but holds a single spec, and it uses
tsconfig.spec.json, which this PR does not touch.