KNOX-3232: Handle pac4j cookies with "null" value

[bonampak]

KNOX-3232 - Handle pac4j cookies with "null" value

What changes were proposed in this pull request?

Just handle the case when our set-cookie header is setting explicit null values and KnoxSessionStore receives the cookie with "null" value.

How was this patch tested?

Manual test with CAS global logout url as described in the JIRA.
Added a unit test in KnoxSessionStoreTests.

[github-actions]

Test Results

7 tests   7 ✅  1s ⏱️
1 suites  0 💤
1 files    0 ❌

Results for commit c7fa377.

[hanicz]

So as I understand the culprit behind the "null" value is sb.append('=').append(value); in SetCookieHeader, right?
Wouldn't it be better to fix that by appending an empty value?
Also the SetCookieHeader class was introduced because with java 8 the sameSite attribute was missing from org.pac4j.core.context.Cookie. Is switching back an option?

[bonampak]

So as I understand the culprit behind the "null" value is sb.append('=').append(value); in SetCookieHeader, right? Wouldn't it be better to fix that by appending an empty value? Also the SetCookieHeader class was introduced because with java 8 the sameSite attribute was missing from org.pac4j.core.context.Cookie. Is switching back an option?

We could switch back as now setSameSitePolicy() is available on org.pac4j.core.context.Cookie.

It would still generate name=null; if the cookie value is null.

        Cookie cookie;
        if (value == null) {
            cookie = new Cookie(PAC4J_SESSION_PREFIX + key, null);
        }
        ...
        if(sessionStoreConfigs != null && sessionStoreConfigs.containsKey(PAC4J_COOKIE_SAMESITE)) {
            cookie.setSameSitePolicy(sessionStoreConfigs.get(PAC4J_COOKIE_SAMESITE));
        }
        context.addResponseCookie(cookie);

https://github.com/pac4j/pac4j/blob/pac4j-parent-6.3.0/pac4j-javaee/src/main/java/org/pac4j/jee/context/JEEContext.java#L217

https://github.com/pac4j/pac4j/blob/pac4j-parent-6.3.0/pac4j-core/src/main/java/org/pac4j/core/context/WebContextHelper.java#L147

    public static String createCookieHeader(Cookie cookie) {
        var builder = new StringBuilder();
        builder.append(String.format("%s=%s;", cookie.getName(), cookie.getValue()));

there are some extra logic in createCookieHeader and we need to make sure that it's consistent with what we have now:

        var sameSitePolicy = cookie.getSameSitePolicy() == null ? "lax" : cookie.getSameSitePolicy().toLowerCase();
        switch (sameSitePolicy) {
            case "strict" -> builder.append(" SameSite=Strict;");
            case "none" -> builder.append(" SameSite=None;");
            default -> builder.append(" SameSite=Lax;");
        }
        if (cookie.isSecure() || "none".equals(sameSitePolicy)) {
            builder.append(" Secure;");
        }

For now, I would keep it as it is, and create another issue to switch back to org.pac4j.core.context.Cookie later (and set cookie value to empty string instead of null).

[hanicz]

Approved
The mentioned changes will be done in https://issues.apache.org/jira/browse/KNOX-3233