Lukasz Lenart created WW-5698:
---------------------------------
Summary: 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
Fix For: 7.4.0
{{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)
