SEC-1788: AbstractPreAuthenticatedProcessingFilter: Unnecessary calls to getPreAuthenticatedPrincipal() from requiresAuthentication() #2023

Closed
spring-issuemaster opened this Issue Jul 21, 2011 · 1 comment

1 participant

@spring-issuemaster

Andrew Lamb (Migrated from SEC-1788) said:

The requiresAuthentication(...) method in the AbstractPreAuthenticatedProcessingFilter class creates a principal object before it is known whether it will be needed. If checkForPrincipalChanges is set to false, the getPreAuthenticatedPrincipal(...) call is unnecessary.

The current code:

    Object principal = getPreAuthenticatedPrincipal(request);
    if (checkForPrincipalChanges &&
        ! currentUser.getName().equals(principal)) {
        // Check session...
        return true;
    }
    return false;

Could be more efficiently written:

    if (checkForPrincipalChanges) {
        Object principal = getPreAuthenticatedPrincipal(request);
        if (! currentUser.getName().equals(principal)) {
           // Check session...
           return true;
        }
    }
    return false;

The unnecessary call to getPreAuthenticatedPrincipal(...) may be trivial for most implementations but I'm in a situation where I'm working with a corporate security service which saves the username of the preauthenticated user inside an encrypted cookie. Each call to getPreAuthenticatedPrincipal(...) requires that the cookie be decrypted and validated. Consequently this is occurs multiple times for each ServletRequest (once for each resource associated with the page: image, css, js, etc.)

This difficulty could be worked around by subclassing AbstractPreAuthenticatedProcessingFilter, but, alas, requiresAuthentication(...) is private: see SEC-1759.

@spring-issuemaster

Luke Taylor said:

I've modified the class to only make the call to getPreAuthenticationPrincipal if checkForPrincipalChanges is true.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.RC3 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment