papegaaij opened a new pull request, #1562: URL: https://github.com/apache/wicket/pull/1562
[WICKET-7200](https://issues.apache.org/jira/browse/WICKET-7200) ## The problem `AnnotationsRoleAuthorizationStrategy` implemented three different rules for combining a class level annotation with a package level one, and none of them were documented: | Annotation | `@Target` | Behaviour before this PR | |---|---|---| | `@AuthorizeInstantiation` | `TYPE`, `PACKAGE` | class replaces package | | `@AuthorizeInstantiations` | `TYPE` | AND-ed on unconditionally, never overridden | | `@AuthorizeAction` | `TYPE`, **`PACKAGE`** | package annotation **never read** | | `@AuthorizeActions` | `TYPE` | class level only | | `@AuthorizeResource` | `TYPE`, `PACKAGE` | class **AND** package | Two consequences that users hit: 1. Because the annotations are `@Inherited` and the class lookup runs first, an annotation on a superclass in *any* package counts as the class level annotation and therefore suppresses the package level annotation of the subclass' own package: ```java // org/mycompany/base/SecuredPage.java @AuthorizeInstantiation("USER") public class SecuredPage extends WebPage { } // org/mycompany/admin/package-info.java @AuthorizeInstantiation("ADMIN") package org.mycompany.admin; // org/mycompany/admin/ReportPage.java -- requires USER, not ADMIN public class ReportPage extends SecuredPage { } ``` Moving a page into a package guarded by `package-info.java` has no effect when one of its superclasses is annotated. 2. `@AuthorizeAction` has declared `ElementType.PACKAGE` since February 2006, but the action path never looked at the package. A `@AuthorizeAction` in a `package-info.java` compiles, looks correct, and protects nothing. ## Why override, and not AND Override is the rule that was designed, and it was affirmed when someone asked for the opposite. The pre-2010 code carried the comment *"If roles are defined for the class, that overrides the package"*, and WICKET-3240 (*"…package==false, class==true returns true"*) asked for conjunctive behaviour and was resolved **without changing behaviour** — only short-circuiting the lookup, with the comment that is still there: *"Check class annotation first because it is more specific than package annotation"*. The conjunction on resources, by contrast, was never a design decision. WICKET-5749 introduced it in 7.0.0 as `class || package` with a missing annotation counting as *deny*. The follow-up commit *"non-annotated resources should be allowed, not denied"* corrected the missing-annotation case to *allow*, and since `true || anything` is always true, that forced `||` to become `&&` in the same edit purely to keep the annotation functional. That commit message says nothing about class-vs-package composition, and the annotation's javadoc still claimed it *"works analogously to AuthorizeInstantiation"* — which it did not. ## What this PR does One resolution path, shared by all three checks: gather the rules declared on the class, and fall back to the package only when the class says nothing. - **`@AuthorizeAction` / `@AuthorizeActions`** honour package level annotations, resolved **per action name**: a class level rule for `ENABLE` overrides only the package rule for `ENABLE`, and the package rule for `RENDER` still applies. Resolving wholesale would mean that annotating a class for one action silently unprotects it for the other. `@AuthorizeActions` gains `ElementType.PACKAGE`, without which a package cannot express more than one action rule. - **`@AuthorizeResource`** replaces the annotation of its package instead of being AND-ed with it. This also removes an unguarded `resourceClass.getPackage()` dereference, which the instantiation path already guarded. - **`@AuthorizeInstantiations`** participates in the override like every other rule, and gains `ElementType.PACKAGE`. Documentation, which is what prompted the ticket: - `AnnotationsRoleAuthorizationStrategy` carries the canonical rules, including the `@Inherited` pitfall, that package annotations do not cascade to subpackages, that `package-info.java` has to be compiled and shipped, and that an annotation without roles authorizes everybody and is therefore the way to exempt a class from its package. - Each annotation points there, and a few long-standing javadoc defects are fixed along the way: `@AuthorizeAction` claimed it *"must be embedded in the AuthorizeActions annotation"* (it works standalone, as `wicket-examples` and the tests show), `@AuthorizeActions`' example showed the singular annotation, `@AuthorizeInstantiations` had a self-referential `@see`, and both `roles()`/`deny()` documented their default as "an empty string" rather than an empty array. - The user guide section on annotations listed two of the five annotations and only class level usage. It now lists all five and has an *Annotations on packages* subsection with the `package-info.java` form, the resolution rule, the worked `@Inherited` trap and the caveats. ## Behaviour changes Both are loosening, and both affect only applications that annotate a class **and** its package: - a resource annotated at both levels with different roles previously required both sets of roles, and now requires only the roles of the class; - a class carrying only `@AuthorizeInstantiations` inside a package carrying `@AuthorizeInstantiation` previously required both, and now requires only the class ruleset. Package level `@AuthorizeAction` changes from no-op to enforced, which can only tighten authorization, and only for applications that already wrote an annotation that never worked. The public and protected API surface of `AnnotationsRoleAuthorizationStrategy` is unchanged, and the `@Target` additions are additive, so no dependent module is affected at compile time. ## Testing `mvn -pl wicket-auth-roles test` → 52 tests pass. The 41 pre-existing tests are untouched and unchanged. New `AnnotationsRolePackageTest` adds 11 tests over real fixture classes in real annotated packages. Mocks cannot be used here, because a generated mock does not live in the package of the class it mocks — which is also why the package level behaviour had never been covered. Nothing in the repository exercised a package level annotation before this PR. Reverting only the strategy to its previous version fails exactly 4 of the 11 new tests — the resource override, the `@AuthorizeInstantiations` override, package level actions, and per-action resolution. The other 7 pass either way, so they pin pre-existing behaviour, including the `@Inherited` trap. `mvn -pl wicket-auth-roles javadoc:javadoc` is clean, and `mvn -pl wicket-user-guide package -P guide` renders the new section with no new *possible invalid reference*. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
