Spring-security: Add Clear Site Data to Log Out

[rwinch]

We should investigate adding Clear Site Data to Spring Security's LogoutHandler implementations. See https://w3c.github.io/webappsec-clear-site-data/

[jzheaux]

The Clear-Site-Data header is now supported by the majority of browsers with compatibility coming soon for others.

Spring Security could simplify Clear-Site-Data configuration via an implementation of HeaderWriter and an implementation of LogoutHandler:

public class ClearSiteDataHeaderWriter implements HeaderWriter {
    String directives;

    // ...
}

public class HeaderWriterLogoutHandler implements LogoutHandler {
    HeaderWriter headerWriter;

    // ...
}

We should consider both a HeaderWriter and a LogoutHandler since the spec lists numerous other circumstances as well as logout for which writing this header is handy.

We should also keep in mind that the spec requires a secure connection:

It is possible that an application could be put into an indeterminate state by clearing only one type of storage. We mitigate that to some extent by clearing all storage options as a block, and by requiring that the header be delivered over a secure connection.

SecureRequestMatcher could turn out to be handy for this.

[rhamedy]

Can I work on this issue?

@jzheaux thank you for your insightful comment. Is there anything else that I should know as a first time contributor?

Even though this task is not labeled as first-timers-only , I would love to work on it since I have previously used Spring Framework and (even did some debugging of Spring OAuth2 - obviously different).

[jzheaux]

@rhamedy yes, it's yours!

I'd recommend keeping the solution as minimal as possible - usually, the smallest public API is the easiest to comprehend its impact on the rest of the codebase.

I'd also recommend taking a look at the contribution guidelines. It's not a long read, but it's easy to forget to do.

[rhamedy]

Thanks. I will start working on this. I read the clear-site-data webpsec and understand its purpose. I will just how to figure out how the HeaderWriter and LoginHandler works and then I should be able to come up with a implementation and discuss it here before making a PR 👍

[rhamedy]

Hi @jzheaux ,

I wanted to share what I have done some far to get some feedback as well as ask the question that came up.

Questions

1. Should this header be included as a default in HeaderConfigurer.java?
2. Curious how does HeadersBeanDefinitionParser.java work?
3. In regards to implementation of LogoutHandler did you mean ClearSiteDataLogoutHandler or HeaderWriterLogoutHandler?
4. There is a web.x and web.server.x for both headers.writer and logout and I used the web.x package. I wanted to learn more about the difference? Is there a resource?
5. Should I write the integration test in Groovy? I remember an issue where the tests are in the process of being migrated to java. Running all the integration tests based on Contribution Guideline take a little long. Is there a way I can run a subset of integration tests?

Feedback
Here is what I trimmed snippets of what I have done so far

1) Added ClearSiteDataHeaderWriter in org.springframework.security.web.header.writer

private static final String CLEAR_SITE_DATA_HEADER = "Clear-Site-Data";
private final Log logger = LogFactory.getLog(getClass());

private boolean all;
private boolean cache;
private boolean cookies;
private boolean storage;
private boolean executionContexts;

private RequestMatcher requestMatcher;

private String headerValue;

2) Followed by a range of constructors that follows HstsHeaderWriter.java for consistency

private ClearSiteDataHeaderWriter(
            boolean all,
            boolean cache,
            boolean cookies,
            boolean storage,
            boolean executionContexts) {
        this.requestMatcher = new SecureRequestMatcher();
        this.all = all;
        this.cache = cache;
        this.cookies = cookies;
        this.storage = storage;
        this.executionContexts = executionContexts;
        this.updateHeaderValues();
    }
        // Default constructor assumes the wild card case i.e. Clear-Site-Data: "*" ??
    public ClearSiteDataHeaderWriter() {
        this(true, false, false, false, false);
    }
    public ClearSiteDataHeaderWriter(boolean cache) {
        this(false, cache, false, false, false);
    }
    public ClearSiteDataHeaderWriter(boolean cache, boolean cookies) {
        this(false, cache, cookies, false, false);
    }
    public ClearSiteDataHeaderWriter(boolean cache, boolean cookies, boolean storage) {
        this(false, cache, cookies, storage, false);
    }
    public ClearSiteDataHeaderWriter(boolean cache, boolean cookies, boolean storage,
            boolean executionContexts) {
        this(false, cache, cookies, storage, executionContexts);
    }

3) I am little confused with the logout handler. I came up with the following based on your comment above where you indicated that HeaderWriterLogoutHandler have HeaderWriter and placed it inside org.springframework.security.web.authentication.logout

public class ClearSiteDataLogoutHandler implements LogoutHandler {
    private ClearSiteDataHeaderWriter clearSiteDataHeaderWriter;

    public ClearSiteDataLogoutHandler(ClearSiteDataHeaderWriter clearSiteDataHeaderWriter) {
        Assert.notNull(clearSiteDataHeaderWriter, "Clear-Site-Data header cannot be null.");
        this.clearSiteDataHeaderWriter = clearSiteDataHeaderWriter;
    }

    @Override
    public void logout(HttpServletRequest request, HttpServletResponse response,
            Authentication authentication) {
        response.setHeader("Clear-Site-Data", clearSiteDataHeaderWriter.getHeaderValue());
    }
}

Depending on your feedback, I will need to

• Add unit tests
• Update the HeaderConfigurer & HeaderDefinitionBeanParser as well as
• Update the headers.adoc and namespace.adoc

Thanks for your help 👍🥇

[jzheaux]

Good questions, @rhamedy:

1. No, not at this point. The user can easily use the LogoutConfigurer to specify the logout handler to use
2. HeadersBeanDefinitionParser is part of XML config and doesn't need to be updated at this time, for the same reason that HeadersConfigurer doesn't need updating.
3. I think if we do HeaderWriterLogoutHandler, then users will be able to indicate any header writer on logout. So, then idea would be:

http
    .logout()
        .addLogoutHandler(new HeaderWriterLogoutHandler(new ClearSiteDataHeaderWriter(...)))

1. The difference is servlet vs reactive. If you are interested, we can take a look at the reactive equivalent support after this ticket. web.x is for servlet and web.server.x is for reactive.
2. No, thank you for asking. You are right, we are migrating to Java, so please write them in Java. As for running the test suite, you don't need to run the full suite each time, just do it before you submit your PR. For now, I'd recommend doing ../gradle clean build test from the web directory or simply running your test in your IDE.

Regarding what you've done so far, thank you for sharing early - it helps us to work off of the same page.

1 & 2. When there are several constructors that all take the same type, it can be tricky for the user to keep track of what he sent to where. Consider what this looks like to the user:

new ClearSiteDataHeaderWriter(true, false, true)

Also, we'd like these constructors to be as resilient to change as possible. If the clear site data header specification adds another value, we'd need to revisit these constructors again, wait for browser support, etc.

What if you did this instead:

public ClearSiteDataHeaderWriter(String... sources) {
    Assert.notEmpty(sources, "sources cannot be empty");
    this.headerValue = Stream.of(sources).map(this::quote).collect(Collectors.joining(","));
}

Or simply:

public ClearSiteData(String headerValue) {
    Assert.hasText(headerValue, "headerValue cannot be empty");
    this.headerValue = headerValue;
}

The nice thing about these is that the user can read it, and it reads similarly to the header itself:

new ClearSiteDataHeaderWriter("cache", "storage")

1. Yes, that's what I was thinking, though I'd probably do something more like:

public class HeaderWriterLogoutHandler implements LogoutHandler {
    private final HeaderWriter headerWriter;

    public HeaderWriterLogoutHandler(HeaderWriter headerWriter) {
        Assert.notNull(headerWriter, "headerWriter cannot be null");
        this.headerWriter = headerWriter;
    }

    public void logout(...) {
        this.headerWriter.writeHeaders(request, response);
    }
}

If we just call it HeaderWriterLogoutHandler then it can be used in other circumstances as well.

1. Bless you for adding some documentation! I'd recommend updating the logout section as opposed to the headers section.

[rhamedy]

Thank you @jzheaux for answering my questions and feedback.

I had initially thought of a constructor with String headerValue but, then dismissed that because of validation of headerValue but, your points about resilience to change as well as ease of read make sense. I will go ahead with the varargs version as it's more concise.

I will make the LogoutHandler implementation more generic as shown above 👍

Looks like the solution I have is not far from a PR! I will let you know once I have a PR. I agree let's finish off this one and I would be happy to work on the Reactive ticket for the same 😄

[rhamedy]

https://github.com/spring-projects/spring-security/pull/6550
@jzheaux FYI ^

Please let me know if you see the need for any changes to be made 😄

[jzheaux]

Thanks, @rhamedy! I've left some feedback inline in the PR.

[rhamedy]

Thank for the feedback @jzheaux

I have addressed all the code review comments except the switch to slf4j where I hit a roadblock. I have left a comment in the PR FYI.

[rhamedy]

During our discussions in this issues and in code reviews, we briefly mentioned

• Reactive support
• Logging strategy
  I would be happy to work on those tasks since I now know how things work.
  Please feel free to tag me in the issues once it's created and scoped :slightly_smiling_face:

I have addressed all the requested changes in the PR. Please let me know if there is any outstanding work left to do.