[
https://issues.apache.org/jira/browse/WW-5678?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5678:
------------------------------
Component/s: Core
> Tidy up the SecurityMemberAccess package-matching helper names and visibility
> -----------------------------------------------------------------------------
>
> Key: WW-5678
> URL: https://issues.apache.org/jira/browse/WW-5678
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 8.0.0
>
>
> Follow-up to [WW-5674|https://issues.apache.org/jira/browse/WW-5674],
> deferred to 8.0.0 because part of it is a source-breaking change.
> {panel:title=Check WW-5675 before starting}
> Item 1 below may already be gone. WW-5675 carries a design note proposing
> that the two allowlist sets be replaced by a single precomputed union, which
> would delete the three-argument overload outright — and with it the name
> collision. Confirm the state of WW-5675 before planning any work here.
> {panel}
> h2. 1. The package-private overload shares a name with a public method
> WW-5674 left {{SecurityMemberAccess}} with three related static methods:
> {code:java}
> public static boolean isClassBelongsToPackages(Class clazz, Set
> matchingPackages) // public API
> static boolean isClassBelongsToPackages(Class clazz, Set first, Set
> second) // package-private
> static boolean isPackageBelongsToPackages(String packageName, Set
> first, Set second)
> {code}
> (Generics omitted above — the issue tracker mangles them.)
> The two-argument form is public; the three-argument overload deliberately is
> not, because it has a single caller ({{isClassAllowlisted}}) and its only
> test lives in the same package. Publishing it would freeze it as
> {{struts2-core}} API.
> Sharing a name across the visibility boundary is a hazard: a contributor
> tidying up "an inconsistent modifier" can widen the overload to {{public}} by
> reflex and never realise they have added permanent API surface. Giving the
> package-private overload a distinct name removes the trap. This part is not
> source-breaking and could land earlier if convenient.
> As noted above, WW-5675 may remove this method altogether. The two-set
> parameter exists only because a precomputed union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}} is not
> currently worthwhile under the prototype bean scope; once WW-5675 makes the
> parsed configuration shared, the union becomes the better shape and the
> overload disappears.
> h2. 2. The names are grammatically awkward
> {{isClassBelongsToPackages}} and {{isPackageBelongsToPackages}} read as "is
> class belongs to packages". Something like {{classBelongsToAnyPackage}} /
> {{packageMatchesAny}} would be clearer. The public one cannot be renamed
> before a major release.
> This item stands regardless of what WW-5675 does.
> h2. 3. Two public statics have no callers outside the class
> {{isClassBelongsToPackages(Class, Set)}} and {{toPackageName(Class)}} are
> both {{public static}} on a public class, but a repository-wide search finds
> no caller outside {{SecurityMemberAccess}} itself — not in core, not in any
> plugin, not in the tests. They appear to be public by accident rather than by
> design.
> Narrowing them to package-private would shrink the surface that has to be
> preserved. This _is_ source-breaking for anyone who happens to call them,
> hence 8.0.0. This item also stands regardless of WW-5675.
> h2. Why this matters now
> WW-4759 is drawing the {{struts2-api}} boundary for 8.0.0. Deciding what in
> {{SecurityMemberAccess}} is genuinely API is part of the same exercise, and
> it is cheaper to settle it while that work is open than to inherit these
> three methods into the new artefact unexamined.
> h2. Constraints
> {{SecurityMemberAccessPackageMatchingTest}} calls all three methods directly
> and will need updating. Any rename must not disturb the frozen
> {{legacyPrefixMatch}} and {{legacyToPackageName}} reference oracles in that
> test — they are verbatim copies of the pre-WW-5674 implementation kept as
> differential oracles, and they must continue to _not_ delegate to production
> code.
> Renaming or narrowing anything public needs a migration-guide entry.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
