[
https://issues.apache.org/jira/browse/WW-5698?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108667#comment-18108667
]
Lukasz Lenart commented on WW-5698:
-----------------------------------
Target release decided: *7.4.0*, as already set on the issue.
The Compatibility section above left three options open — 7.4.0, 8.0.0, or
gating behind {{struts.parameters.requireAnnotations.transitionMode}}. The
third turned out not to be a separate option. Transition mode already exists to
let an application enable {{requireAnnotations}} while it works through
annotating, so it is precisely the migration lever for this change; no new
opt-out flag is warranted.
It did need fixing first. {{requireAnnotationsTransitionMode}} was evaluated
_after_ the ModelDriven branch in {{isAuthorized(...)}}, so it never applied to
a ModelDriven action at all. That was harmless while the exemption authorized
everything, but once the exemption is scoped to the model it would have left
the affected actions — and only those actions — with no migration path, because
the lever built for this situation was unreachable for exactly the code that
needs it. The check is now evaluated first.
With that in place, an application affected by this change can set
{{requireAnnotations.transitionMode=true}} and its depth-0 action members keep
binding, which is the same path any non-ModelDriven action already has. That is
what makes 7.4.0 defensible rather than deferring to 8.0.0.
Implemented in [PR #1872|https://github.com/apache/struts/pull/1872].
> ModelDriven exemption in StrutsParameterAuthorizer also exempts the action's
> own members
> ----------------------------------------------------------------------------------------
>
> Key: WW-5698
> URL: https://issues.apache.org/jira/browse/WW-5698
> Project: Struts 2
> Issue Type: Task
> Components: Core
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> {{StrutsParameterAuthorizer.isAuthorized(...)}} exempts {{ModelDriven}}
> actions from the {{@StrutsParameter}} requirement:
> {code:java}// ModelDriven exemption: only exempt when the action explicitly
> implements ModelDriven
> // and the target is its model object. ...
> if (target != action && action instanceof ModelDriven) {
> return true;
> }
> {code}
> The intent is sound and is what makes model binding work: implementing
> {{ModelDriven}} and returning an object from {{getModel()}} is a type-level
> declaration that the model is request surface, so its properties do not each
> need annotating.
> The effect is wider than the intent. The method returns {{true}} for _any_
> parameter name, and the name is subsequently resolved by OGNL against the
> whole {{CompoundRoot}}, which holds the model on top of the action.
> Authorization is therefore decided about the model, while the resulting write
> may land on the action. The practical result is that the {{@StrutsParameter}}
> requirement does not apply to an action's own members once that action
> implements {{ModelDriven}}.
> h2. Observed
> Same unannotated setter, declared on the action class in both cases, with
> {{struts.parameters.requireAnnotations=true}}:
> {code}plain action parameter actionSecret=... -> not bound
> (correctly rejected)
> ModelDriven action parameter actionSecret=... -> bound
> {code}
> Both runs also bound a second, expected parameter, confirming the negative
> result is a real rejection rather than a harness that binds nothing.
> A related consequence is that framework members inherited from
> {{ActionSupport}} become reachable on {{ModelDriven}} actions in the same way
> — a parameter name of {{getText('some-key').property}} invokes
> {{ActionSupport.getText(String)}}, which is not annotated and is not part of
> any model. That particular call is inert, since it is a resource bundle
> lookup whose result is discarded, but it illustrates that the exempted
> surface is the whole stack rather than the model.
> h2. Proposed change
> Keep the exemption, but scope it to what it is meant to cover: authorize
> members of the model object, and continue to apply the annotation requirement
> to members of the action itself. {{resolveTarget(...)}} already distinguishes
> the two, so the information needed is present at the decision point.
> h2. Compatibility
> This is a behavioural change. An application with a {{ModelDriven}} action
> that currently relies on binding unannotated members declared on the action
> will stop binding them once the requirement applies, and will need those
> members annotated with {{@StrutsParameter}}. That is the same migration those
> members would have needed had the action not been {{ModelDriven}}, but it is
> still a change for existing applications, so it may belong in 8.0.0 rather
> than 7.4.0, or behind {{struts.parameters.requireAnnotations.transitionMode}}
> for a release. Worth deciding before the change is written.
> Related to WW-5697, which concerns method invocation during binding and has a
> separate cause and a separate fix; the two only overlap in that a
> {{ModelDriven}} action is the easiest way to reach both.
> The exemption is also currently undocumented. Whatever scope it ends up with
> should be stated in the {{@StrutsParameter}} and ModelDriven documentation,
> together with the advice that a model should be a request DTO rather than a
> domain or persistence object.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
