Skip to content
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

WW-5400 Extend default configuration options for the CSP interceptor. #913

Merged
merged 3 commits into from
May 11, 2024
Merged

Conversation

eschulma
Copy link
Contributor

@eschulma eschulma commented Apr 10, 2024

Previously, it was impossible to set global options for the CSP interceptor. The only option was to have every action individually implement CspSettingsAware.

To fix this, we add an interceptor parameter of defaultCspSettingsClassName. Values from this class will be used in the CSP header instead of DefaultCspSettings. Users may define their own custom class which implements CspSettings, and that will be the default for all actions that do not implement the CspSettingsAware interface. It is now possible to create this custom class by simply extending DefaultCspSettings.

I have fixed a spelling error in DefaultCspSettings.java -- cratePolicyFormat renamed to createPolicyFormat.

Closes WW-5400

Previously, it was impossible to set global options for the CSP interceptor. The only options was to have every action individually implement CspSettingsAware.

To fix this, we add an interceptor parameter of defaultCspSettingsClassName. Values from this class will be used in the CSP header instead of DefaultCspSettings. Users may define their own custom class which implements CspSettings, and that will be the default for all actions that do not implement the CspSettingsAware interface. It is now possible to create this custom class by simply extending DefaultCspSettings.

I have fixed a spelling error in DefaultCspSettings.java -- cratePolicyFormat renamed to createPolicyFormat.
@eschulma
Copy link
Contributor Author

@lukaszlenart submitted per your request

@eschulma
Copy link
Contributor Author

Hold off a bit, I need to check something (this is what I get for implementing my own separate solution)

@eschulma
Copy link
Contributor Author

Ok all good.

Comment on lines +160 to +162
public void setDefaultCspSettingsClassName(String defaultCspSettingsClassName) {
this.defaultCspSettingsClassName = defaultCspSettingsClassName;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Struts inject mechanism instead of using raw class and creating the instance by yourself. It's all about defining a <bean name="customCspSettings" class="..."/> and then annotating the setter with @Inject("customCspSettings").

I assume you never played with Struts @Inject, so let's leave it as is and I will change that in the next PR.

Comment on lines +60 to +77
LOG.trace("Using {} with action: {}", defaultCspSettingsClassName, action);

// if the defaultCspSettingsClassName is not a real class, throw an exception
try {
Class.forName(defaultCspSettingsClassName, false, Thread.currentThread().getContextClassLoader());
}
catch (ClassNotFoundException e) {
throw new IllegalArgumentException("The defaultCspSettingsClassName must be a real class.");
}

// if defaultCspSettingsClassName does not implement CspSettings, throw an exception
if (!CspSettings.class.isAssignableFrom(Class.forName(defaultCspSettingsClassName))) {
throw new IllegalArgumentException("The defaultCspSettingsClassName must implement CspSettings.");
}

CspSettings cspSettings = (CspSettings) Class.forName(defaultCspSettingsClassName)
.getDeclaredConstructor().newInstance();
applySettings(invocation, cspSettings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can move this code into init() method of the interceptor as right now a new instance is created per each invocation

Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and I will try to refactor it afterwards

@lukaszlenart lukaszlenart merged commit 649760d into apache:master May 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants