Skip to content

fix(policies): allow mixed properties in policy value contracts - #9453

Open
lfals wants to merge 4 commits into
LibreSign:mainfrom
lfals:fix/policy-value-openapi-contract
Open

lfals wants to merge 4 commits into
LibreSign:mainfrom
lfals:fix/policy-value-openapi-contract

Conversation

@lfals

@lfals lfals commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Generated API types reject valid policy objects such as { enabled: false, approvers: ['admin'] } and { mode: 'disabled' }.

The extractor maps array<string, mixed> to an object whose properties must themselves be objects. This restriction does not match those policy values.

Solution

  • Describe the object branch of LibresignEffectivePolicyValue as an open object, generating additionalProperties: true.
  • Reuse the shared alias in all four individual and four compound policy write endpoints.
  • Regenerate all affected OpenAPI specifications and TypeScript contracts.
  • Add contract regression tests for open object properties, nullability, and shared request/response value types.

The generated object branch now accepts { [key: string]: unknown }. Runtime policy validation, permissions, and persistence are unchanged.

Tests and evidence

  • OpenAPI generation completed using the extractor installed from the repository lockfile, including TypeScript generation.
  • vendor-bin/phpunit/vendor/bin/phpunit -c tests/php/openapi/phpunit.xml: 16 tests passed, 133 assertions.
  • npm run ts:check: passed.
  • Hey API 0.99.0 generation and strict TypeScript compilation passed for scalar values, explicit null, and objects containing booleans, strings, lists, and nested objects across the eight write types.
  • The same representative object fails compilation against the previous Hey API output because its boolean and list properties are rejected.
  • PHP syntax checks and git diff --check: passed.

Known limits

  • Psalm and the full backend/runtime integration suites were not executed.
  • The Hey API compilation fixture is a local generation experiment, not a committed SDK or CI test.
  • This change describes the JSON object contract; native PHP parameter types remain unchanged.

Compatibility and related work

This widens generated object-property types to match existing payloads. Consumers must narrow unknown property values when reading them.

This complements #9450, which makes value required and preserves explicit null. Required-field behavior is not included here. Both PRs touch the individual endpoint annotations and generated files; regenerate contracts after combining them.

The identity-document changes in #9449 and #9451 are outside this PR.

Signed-off-by: Luis Amorim <luisfelipeamorim@hotmail.com>

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes before merge. Static analysis fails with four new MismatchingDocblockParamType errors caused by the policy-value alias, and the regenerated OpenAPI 3.0.3 contract loses endpoint-specific metadata by placing description and default alongside $ref. Please address these at the source/generation boundary, regenerate OpenAPI and TypeScript artifacts, and verify Psalm, OpenAPI contract checks, and a representative client-generation case. Coordinate the generated contract changes with #9450, which updates the required-field semantics.

Comment thread lib/ResponseDefinitions.php Outdated
*
* @psalm-type LibresignPolicyScope = 'system'|'group'|'user'
* @psalm-type LibresignEffectivePolicyValue = null|bool|int|float|string|array<string, mixed>
* @psalm-type LibresignEffectivePolicyValue = null|bool|int|float|string|object

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Psalm CI is failing on this alias change. The new object type is used by four controller parameter annotations, while the native PHP signatures still accept array, not object. Psalm reports MismatchingDocblockParamType at lines 255, 294, 603 and 641 of PolicyController.php.

Please preserve the PHP-compatible array contract and fix the OpenAPI generation of array<string, mixed> so it produces unrestricted object properties. Alternatively, use another deterministic, tested generator-side mapping. Avoid widening the PHP signatures or suppressing Psalm errors.

Regenerate the contracts and verify that both Psalm and the generated TypeScript types pass.

Comment thread openapi-administration.json Resolved

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Follow-up to my earlier review: placing a finding on openapi-administration.json was the wrong review anchor because it is generated. This inline comment identifies the PHPDoc trigger and the extractor implementation that must be corrected; the generated contracts must only be regenerated.

*
* @param string $policyKey Policy identifier to persist at the system layer.
* @param null|bool|int|float|string|array<string, mixed> $value Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it.
* @param LibresignEffectivePolicyValue $value Policy value to persist as an explicit system rule, even when it equals the default. Null removes the explicit system rule: the built-in default applies and lower layers may override it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This annotation is the LibreSign source that triggers the generated OpenAPI issue. With @param LibresignEffectivePolicyValue $value, nextcloud/openapi-extractor v1.9.1 resolves the alias to $ref. Its src/ControllerMethod.php attaches the parameter description/default, and src/OpenApiType.php::toArray() serializes those fields alongside $ref. In OpenAPI 3.0.3, siblings of a Reference Object are ignored, so the generated contract loses this endpoint's description and explicit-null semantics.

Please fix this at the source/generator boundary, not in openapi*.json. The preferred extractor fix is to emit an OpenAPI 3.0-compatible Schema Object, such as allOf: [{ $ref: ... }] with the parameter-specific description and default outside the reference. Alternatively, keep a supported inline parameter type at the PHPDoc source if it preserves the intended schema without introducing the reference. Add a generator regression case for an alias-typed request parameter with a description and default, regenerate all specs, and verify both the resulting OpenAPI and a representative generated client (including explicit null).

The policy value alias stays an array so Psalm matches the PHP signature.
The extractor now maps array<string, mixed> to unrestricted objects and
places description and default on a schema object instead of beside $ref.

Signed-off-by: Luis Amorim <luisfelipeamorim@hotmail.com>
LibreSign#9450 now requires value. The regenerated contract keeps that requirement,
explicit null, and unrestricted object properties, with the description
outside $ref.

Signed-off-by: Luis Amorim <luisfelipeamorim@hotmail.com>

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes before merge because the global $ref wrapper appears to introduce schema/default inconsistencies outside the policy endpoints. Please verify the generated OpenAPI contract and a representative client-generation case, then correct any confirmed mismatch at the source/generator boundary. The policy-value correction itself should remain intact.

+ );
+ $schema = clone $this;
+ $schema->ref = null;
+ $schema->allOf = [...($schema->allOf ?? []), $reference];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The generalized $ref wrapper also affects schemas outside the policy endpoints.

In the regenerated openapi-full.json, properties such as file, settings, and coordinates now expose default: [] alongside allOf references to schemas declared as type: object. Previously, these defaults were ignored as $ref siblings.

This introduces an inconsistency between the effective schema and its declared default, potentially affecting generated clients.

Could we verify this with a focused schema-conformance and client-generation test? If the mismatch is confirmed, please correct it at the source/generator boundary and regenerate the contracts. The policy-value fix itself should be preserved.

Wrapping $ref made default: [] effective for file, settings, and coordinates
even though those schemas are objects. The extractor now drops a default that
does not match the referenced object and still keeps description and
conforming defaults outside $ref.

Signed-off-by: Luis Amorim <luisfelipeamorim@hotmail.com>
@lfals
lfals requested a review from vitormattos November 13, 2026 19:17

This branch has not been deployed

No deployments
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.

2 participants