Repository navigation
Introduce ValidationPackageOpener to relay open directives to validation providers (JPMS package opens -> validation provider) #340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| * Jakarta Validation API | ||
| * | ||
| * License: Apache License, Version 2.0 | ||
| * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>. | ||
| */ | ||
| package jakarta.validation.internal; | ||
|
|
||
| import java.lang.invoke.MethodHandles; | ||
| import java.util.Objects; | ||
|
|
||
| import jakarta.validation.spi.PackageAccessException; | ||
| import jakarta.validation.spi.ValidationPackageOpener; | ||
|
|
||
| /** | ||
| * Implementation of {@link ValidationPackageOpener} bound to a specific provider module. | ||
| * <p> | ||
| * This class resides in a non-exported package so that it cannot be accessed, | ||
| * subclassed, or instantiated from outside the {@code jakarta.validation} module. | ||
| * <p> | ||
| * The {@link Module#addOpens(String, Module)} call is {@code @CallerSensitive}; | ||
| * because this class belongs to the {@code jakarta.validation} module, the JVM | ||
| * recognizes it as a valid caller when the user has opened a package to | ||
| * {@code jakarta.validation}. | ||
| * | ||
| * @since 4.0 | ||
| */ | ||
| public final class ValidationPackageOpenerImpl implements ValidationPackageOpener { | ||
|
|
||
| private final Module providerModule; | ||
|
|
||
| /** | ||
| * @param providerModule the module of the validation provider discovered | ||
| * during bootstrap; this opener will only open packages to this module | ||
| */ | ||
| public ValidationPackageOpenerImpl(Module providerModule) { | ||
| this.providerModule = Objects.requireNonNull( providerModule ); | ||
| } | ||
|
|
||
| @Override | ||
| public void openPackage(MethodHandles.Lookup providerLookup, Module targetModule, String packageName) { | ||
| Objects.requireNonNull( providerLookup ); | ||
| Objects.requireNonNull( targetModule ); | ||
| Objects.requireNonNull( packageName ); | ||
|
|
||
| if ( !providerLookup.hasFullPrivilegeAccess() ) { | ||
| throw new PackageAccessException( | ||
| "ValidationPackageOpener requires a full-privilege Lookup (obtained via MethodHandles.lookup())" | ||
| ); | ||
| } | ||
|
|
||
| if ( !providerLookup.lookupClass().getModule().equals( providerModule ) ) { | ||
| throw new PackageAccessException( | ||
| "Lookup class module " + providerLookup.lookupClass().getModule().getName() | ||
| + " does not match the bound provider module " + providerModule.getName() | ||
| ); | ||
| } | ||
|
|
||
| if ( !targetModule.isNamed() ) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if ( !targetModule.isOpen( packageName, providerModule ) ) { | ||
| targetModule.addOpens( packageName, providerModule ); | ||
| } | ||
| } | ||
| catch (IllegalCallerException e) { | ||
| throw new PackageAccessException( | ||
| "Cannot open package " + packageName + " in module " + targetModule.getName() | ||
| + " to provider module " + providerModule.getName() | ||
| + ". Ensure the module has 'opens " + packageName + " to jakarta.validation'", | ||
| e | ||
| ); | ||
| } | ||
|
Comment on lines
+46
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @mkouba @Sanne, you expressed concerns about the security aspects of https://in.relation.to/2018/03/21/spec-api-modularity-patterns/ a (long) while ago. I believe @marko-bekhta successfully addressed these concerns here by making sure the package opener can only ever be used to open a package to the provider (in this case, Hibernate Validator) and not to a random malicious module. Do you see any obvious flaw with this solution? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been too long ;-). But @manovotn might have some good observations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am by no means a JPMS expert but we are just now having a similar conversation over on the CDI specification (jakartaee/cdi#1015). I wonder - and this is likely because of my utter ignorance of validation so sorry for that - what scenarios are you covering with this? Because for CDI, we have this conversation mainly because of CDI SE - in Jakarta EE, existing servers either have their own approximation of modular approach already or they can introduce a module layer and sidestep the whole problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right sorry it's been too long for me as well. At the time I was working actively with various POCs of modules and trying out things... JPA was nowhere ready for it and I remember suggesting changes but it took eons to get them integrated. It's now 8 years later and I'm just very rusty with it all - I barely remember what I worked on last month :D Perhaps @gunnarmorling would remember some details and would want to chime in? |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /* | ||
| * Jakarta Validation API | ||
| * | ||
| * License: Apache License, Version 2.0 | ||
| * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>. | ||
| */ | ||
| /** | ||
| * Internal implementation package -- not exported and not part of the public API. | ||
| */ | ||
| package jakarta.validation.internal; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * Jakarta Validation API | ||
| * | ||
| * License: Apache License, Version 2.0 | ||
| * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>. | ||
| */ | ||
| package jakarta.validation.spi; | ||
|
|
||
| import jakarta.validation.ValidationException; | ||
|
|
||
| import java.io.Serial; | ||
|
|
||
| /** | ||
| * Exception raised when a {@link jakarta.validation.spi.ValidationPackageOpener} | ||
| * cannot relay package access to the validation provider module. | ||
| * <p> | ||
| * Typical causes include a missing or invalid {@link java.lang.invoke.MethodHandles.Lookup} | ||
| * and a package that has not been opened to the {@code jakarta.validation} module. | ||
| * | ||
| * @since 4.0 | ||
| */ | ||
| public class PackageAccessException extends ValidationException { | ||
|
|
||
| @Serial | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| public PackageAccessException() { | ||
| super(); | ||
| } | ||
|
|
||
| public PackageAccessException(String message) { | ||
| super( message ); | ||
| } | ||
|
|
||
| public PackageAccessException(Throwable cause) { | ||
| super( cause ); | ||
| } | ||
|
|
||
| public PackageAccessException(String message, Throwable cause) { | ||
| super( message, cause ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| /* | ||
| * Jakarta Validation API | ||
| * | ||
| * License: Apache License, Version 2.0 | ||
| * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>. | ||
| */ | ||
| package jakarta.validation.spi; | ||
|
|
||
| import java.lang.invoke.MethodHandles; | ||
|
|
||
| /** | ||
| * Callback provided by the Jakarta Validation bootstrap to relay | ||
| * JPMS package-opens through the {@code jakarta.validation} module to a | ||
| * validation provider module. | ||
| * <p> | ||
| * When a user module opens a package to {@code jakarta.validation} | ||
| * via a qualified {@code opens ... to jakarta.validation} directive, | ||
| * this callback relays that access to the validation provider's module, | ||
| * so the user never needs to name the provider module directly. | ||
| * <p> | ||
| * The opener is bound to the provider module that was discovered during | ||
| * bootstrap. The {@link MethodHandles.Lookup} parameter serves as an | ||
| * unforgeable proof of caller identity: the implementation verifies | ||
| * that the lookup has {@linkplain MethodHandles.Lookup#hasFullPrivilegeAccess() | ||
| * full privilege access} and that its | ||
| * {@linkplain MethodHandles.Lookup#lookupClass() lookup class} belongs | ||
| * to the bound provider module. | ||
| * <p> | ||
| * <b>Usage by providers:</b> | ||
| * <pre> | ||
| * // During bootstrap, obtain the opener from BootstrapState | ||
| * ValidationPackageOpener opener = bootstrapState.getPackageOpener(); | ||
| * | ||
| * // Only relay when the user module has opened the package to jakarta.validation | ||
| * Module targetModule = beanClass.getModule(); | ||
| * String packageName = beanClass.getPackageName(); | ||
| * if (targetModule.isOpen(packageName, ValidationPackageOpener.class.getModule())) { | ||
| * // Create lookup at the call site — do not cache as a static field | ||
| * opener.openPackage(MethodHandles.lookup(), targetModule, packageName); | ||
| * } | ||
| * </pre> | ||
| * <p> | ||
| * In non-modular environments (classpath), invoking this callback | ||
| * is a safe no-op. | ||
| * <p> | ||
| * If the provider needs to further relay access to a third-party | ||
| * library module, it can call {@link Module#addOpens(String, Module)} | ||
| * from its own code after obtaining access through this callback. | ||
| * | ||
| * @since 4.0 | ||
| */ | ||
| @FunctionalInterface | ||
| public interface ValidationPackageOpener { | ||
|
|
||
| /** | ||
| * Opens {@code packageName} in {@code targetModule} to the bound | ||
| * provider module, provided that the package is open to the | ||
| * {@code jakarta.validation} module and not already open to the | ||
| * provider. | ||
| * | ||
| * @param providerLookup a full-privilege {@link MethodHandles.Lookup} | ||
| * obtained via {@code MethodHandles.lookup()} in the provider's | ||
| * code; used to verify the caller's identity | ||
| * @param targetModule the module containing the package to open | ||
| * (typically the user's module) | ||
| * @param packageName the fully qualified package name to open | ||
| * (e.g. {@code "com.example.beans"}) | ||
| * @throws PackageAccessException if the lookup does not | ||
| * have full privilege access, does not belong to the bound | ||
| * provider module, or the package could not be opened | ||
| */ | ||
| void openPackage(MethodHandles.Lookup providerLookup, Module targetModule, String packageName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to ensure this method is only ever called from the module of the provider?
We could use
StackWalkerto find all the stack frames until the provider module is reached and assert that there are no stack frames from other modules, which are not reflection related. That way, we'd ensure that only the provider can call this, as an additional safety net.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 , that definitely was the intention, yes! I thought that I got it addressed by:
the idea being that we pass the lookup created at a callsite
opener.openPackage(MethodHandles.lookup(), ...)and as this carries the info of who called the lookup + it cannot be "faked" we get that safty net ? Or did you mean to add the stack walker as an extra check to this one?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding the
StackWalkercode as an extra safety, but it seems like your proposed solution is good enough, assuming safe usage from the provider.