[
https://issues.apache.org/jira/browse/WICKET-7200?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109808#comment-18109808
]
ASF GitHub Bot commented on WICKET-7200:
----------------------------------------
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
> Align class-vs-package resolution across the wicket-auth-roles annotations
> --------------------------------------------------------------------------
>
> Key: WICKET-7200
> URL: https://issues.apache.org/jira/browse/WICKET-7200
> Project: Wicket
> Issue Type: Improvement
> Components: wicket-auth-roles
> Reporter: Emond Papegaaij
> Priority: Major
> Fix For: 11.0.0
>
>
> h3. Problem
> {{AnnotationsRoleAuthorizationStrategy}} implements three different rules for
> combining a class level annotation with a package level one, and none of them
> are documented:
> * {{@AuthorizeInstantiation}}: the class annotation replaces the package
> annotation.
> * {{@AuthorizeAction}}: the package annotation is never read at all, even
> though the
> annotation has declared {{ElementType.PACKAGE}} since February 2006. An
> {{@AuthorizeAction}} in a {{package-info.java}} compiles, looks correct, and
> protects
> nothing.
> * {{@AuthorizeResource}}: the class annotation and the package annotation are
> AND-ed.
> * {{@AuthorizeInstantiations}}: the ruleset is AND-ed on unconditionally and
> never
> participates in the override, and it is class level only.
> Two consequences that users hit in practice:
> # Because these 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.
> Moving a page into a package guarded by {{package-info.java}} has no effect
> when one
> of its superclasses is annotated:
> {code: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 {
> }
> {code}
> # Package level {{@AuthorizeAction}} silently does nothing, so a package that
> appears
> to be render protected is not.
> h3. Which rule is the intended one
> The override rule used for instantiation is the deliberate one. It carried
> the comment
> "If roles are defined for the class, that overrides the package" from the
> start, and
> WICKET-3240 asked for conjunctive behaviour and was resolved by keeping the
> override
> and only short-circuiting the lookup, with the comment "Check class
> annotation first
> because it is more specific than package annotation".
> The conjunction used for resources is not a design decision. WICKET-5749
> introduced it
> in Wicket 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", which forced {{||}} to become {{&&}}
> in the
> same edit purely to keep the annotation functional. That commit message does
> not
> mention class-vs-package composition at all, and the annotation's own javadoc
> still
> states that it "works analogously to AuthorizeInstantiation", which it does
> not.
> h3. Proposed change
> Standardise on the override rule: the rules on a class replace the rules on
> its
> package, and rules at the same level are combined with AND. Document it on
> {{AnnotationsRoleAuthorizationStrategy}}, on each annotation, and in the user
> guide.
> * {{@AuthorizeAction}} and {{@AuthorizeActions}}: honour package level
> annotations,
> resolved per action name, so a class level rule for {{ENABLE}} overrides only
> the
> package rule for {{ENABLE}} and the package rule for {{RENDER}} still applies.
> {{@AuthorizeActions}} gains {{ElementType.PACKAGE}}, without which a package
> cannot
> express more than one action rule.
> * {{@AuthorizeResource}}: the class level annotation replaces the package
> level one
> instead of being AND-ed with it. This also removes an unguarded
> {{resourceClass.getPackage()}} dereference, unlike the guarded instantiation
> path.
> * {{@AuthorizeInstantiations}}: participates in the override like every other
> rule,
> and gains {{ElementType.PACKAGE}}.
> h3. Compatibility
> Two behaviour changes, both loosening, both affecting 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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
