Skip to content

Add permissionsPolicy http header #9265

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 11, 2020
Merged

Add permissionsPolicy http header #9265

merged 1 commit into from
Dec 11, 2020

Conversation

kris2kris
Copy link
Contributor

Pull request for issue #9262

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 5, 2020
@eleftherias eleftherias self-assigned this Dec 7, 2020
@eleftherias eleftherias added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 7, 2020
eleftherias
eleftherias suggested changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kris2kris!

I have left some feedback inline, mostly around the lambda DSL.

Please reach out if you have any questions.

@kris2kris
Copy link
Contributor Author

Hi @eleftherias I added a separate commit after your review.
If it is ok, I will squash the commits

eleftherias
eleftherias suggested changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

I have left some more comments inline.

Please do squash the commits.

@kris2kris
Copy link
Contributor Author

kris2kris commented Dec 10, 2020 •
edited
Loading

Hi @eleftherias,

I did the requested changes.

eleftherias
eleftherias suggested changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks @kris2kris!
I have left a couple more comments.

config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
private PermissionsPolicySpec() {
}

private PermissionsPolicySpec(String permissionsPolicy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this constructor is unused.

...main/java/org/springframework/security/web/header/writers/PermissionsPolicyHeaderWriter.java Outdated
* Create a new instance of {@link PermissionsPolicyHeaderWriter}.
*/
public PermissionsPolicyHeaderWriter() {
this("");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should set the policy to "".
We want to ensure that users are configuring the policy if they are using the PermissionsPolicyHeaderWriter.
This will allow them to create a PermissionsPolicyHeaderWriter without specifying the policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so we should have two ways to set the Policy ? With String and with Lambda ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

http
	.headers()
		.permissionsPolicy()
			.policy("geolocation 'self'");

and

http
	.headers(headers -> headers
		.permissionsPolicy(permissions -> permissions
			.policy("geolocation 'self'")
		)
	);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I change constructor and add some tests to be sure everything is ok

eleftherias
eleftherias suggested changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @kris2kris!
I just have one more comment.

...n/java/org/springframework/security/config/annotation/web/configurers/HeadersConfigurer.java Outdated
Comment on lines 520 to 523
public PermissionsPolicyConfig permissionsPolicy(String policy) {
this.permissionsPolicy.writer = new PermissionsPolicyHeaderWriter(policy);
return this.permissionsPolicy;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I confused you with my previous comment. This was correct before.

Suggested change
public PermissionsPolicyConfig permissionsPolicy(String policy) {
this.permissionsPolicy.writer = new PermissionsPolicyHeaderWriter(policy);
return this.permissionsPolicy;
}
public PermissionsPolicyConfig permissionsPolicy() {
this.permissionsPolicy.writer = new PermissionsPolicyHeaderWriter();
return this.permissionsPolicy;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, it's my bad... I revert the change and update the tests

@eleftherias eleftherias added this to the 5.5.0-M2 milestone Dec 11, 2020
@eleftherias eleftherias added the status: duplicate A duplicate of another issue label Dec 11, 2020
@eleftherias eleftherias merged commit 54d3839 into spring-projects:master Dec 11, 2020
@eleftherias
Copy link
Contributor

Thanks for all of your work @kris2kris! This is now merged into master.

@eleftherias eleftherias mentioned this pull request Dec 11, 2020
Closed
php-coder
php-coder reviewed Dec 15, 2020
View reviewed changes
...src/main/kotlin/org/springframework/security/config/web/server/ServerPermissionsPolicyDsl.kt

package org.springframework.security.config.web.server

import org.springframework.security.web.server.header.ReferrerPolicyServerHttpHeadersWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it looks like an unused import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder left review comments

@eleftherias eleftherias eleftherias requested changes

Assignees

@eleftherias eleftherias

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.5.0-M2
Development

Successfully merging this pull request may close these issues.
4 participants
@kris2kris @eleftherias @php-coder @spring-projects-issues