eclipse / kapua

Eclipse Public License 2.0
222 stars 160 forks source link

:bug: RebuildSessionFilter incorrectly dirties the response #4059

Closed dseurotech closed 2 months ago

dseurotech commented 2 months ago

When calling the job engine from a privileged context (e.g.: from within KapuaSecurityUtils.doPrivileged context on the client side), a MalformedAccessTokenException is thrown within the KapuaTokenAuthenticationFilter. Currently the class RebuildSessionFilter (extending KapuaTokenAuthenticationFilter) relies on the onAccessDenied in order to analyze the request headers and rebuild the KapuaSession correctly (due to shiro’s implementation of AccessControlFilter:

public boolean onPreHandle(ServletRequest request, ServletResponse response, Object mappedValue) throws Exception {
    return isAccessAllowed(request, response, mappedValue) || onAccessDenied(request, response, mappedValue);
}

However by that point the response has already been modified in a 401 by KapuaTokenAuthenticationFilter.isAccessAllowed:

@Override
protected boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) {
    [...]
    try {
        return executeLogin(request, response);
    } catch (AuthenticationException ae) {
        return onLoginFailure(null, ae, request, response);
    } 
    [...]
}
protected boolean onLoginFailure(AuthenticationToken token, AuthenticationException e,
        ServletRequest request, ServletResponse response) {
    HttpServletResponse httpResponse = WebUtils.toHttp(response);
    httpResponse.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
    //now I set a dummy header to propagate the error message to the CORSResponseFilter Class, that eventually will send this error message if CORS filter passes
    httpResponse.setHeader("exceptionMessagePropagatedToCORS", handleAuthException(e));
    return false;
}

Because of this, any other filter in the chain would likely fail. This is the case, for example, if the CORSResponseFilter si declared AFTER the ShiroFilter in the chain - as it should be, considering the CORS filter needs the current scope to be defined in order to retrieve origin endpoints configured for that specific client.

It could be argued that CORS is not needed at all for the Job Engine, as this component is exposed only in the intranet, and therefore called only internally, however:

it’s still wrong to invoke a potentially expensive jwt parsing code, and throwing an exception just to ignore it afterwards (exception handling is, as always, very costly)

other filters might need to be added in the future, and could fail due to the dirty response

This PR moves the “trusted” header identification directly into the RebuildSessionFilter.isAccessAllowed method, overriding completely the underlying implementation if a trusted context is present.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes missing coverage. Please review.

Project coverage is 16.78%. Comparing base (8e8cbfd) to head (c27045d).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse/kapua/pull/4059/graphs/tree.svg?width=650&height=150&src=pr&token=1P4N4CApH8&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse)](https://app.codecov.io/gh/eclipse/kapua/pull/4059?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse) ```diff @@ Coverage Diff @@ ## develop #4059 +/- ## ========================================== Coverage 16.78% 16.78% Complexity 22 22 ========================================== Files 2006 2006 Lines 52093 52101 +8 Branches 4385 4386 +1 ========================================== + Hits 8745 8747 +2 - Misses 42953 42959 +6 Partials 395 395 ``` | [Files](https://app.codecov.io/gh/eclipse/kapua/pull/4059?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse) | Coverage Δ | | |---|---|---| | [...g/eclipse/kapua/commons/security/KapuaSession.java](https://app.codecov.io/gh/eclipse/kapua/pull/4059?src=pr&el=tree&filepath=commons%2Fsrc%2Fmain%2Fjava%2Forg%2Feclipse%2Fkapua%2Fcommons%2Fsecurity%2FKapuaSession.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-Y29tbW9ucy9zcmMvbWFpbi9qYXZhL29yZy9lY2xpcHNlL2thcHVhL2NvbW1vbnMvc2VjdXJpdHkvS2FwdWFTZXNzaW9uLmphdmE=) | `47.27% <100.00%> (-0.95%)` | :arrow_down: | | [.../api/core/auth/KapuaTokenAuthenticationFilter.java](https://app.codecov.io/gh/eclipse/kapua/pull/4059?src=pr&el=tree&filepath=rest-api%2Fcore%2Fsrc%2Fmain%2Fjava%2Forg%2Feclipse%2Fkapua%2Fapp%2Fapi%2Fcore%2Fauth%2FKapuaTokenAuthenticationFilter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-cmVzdC1hcGkvY29yZS9zcmMvbWFpbi9qYXZhL29yZy9lY2xpcHNlL2thcHVhL2FwcC9hcGkvY29yZS9hdXRoL0thcHVhVG9rZW5BdXRoZW50aWNhdGlvbkZpbHRlci5qYXZh) | `27.58% <100.00%> (+5.36%)` | :arrow_up: | | [...ua/job/engine/client/filter/SessionInfoFilter.java](https://app.codecov.io/gh/eclipse/kapua/pull/4059?src=pr&el=tree&filepath=job-engine%2Fclient%2Fsrc%2Fmain%2Fjava%2Forg%2Feclipse%2Fkapua%2Fjob%2Fengine%2Fclient%2Ffilter%2FSessionInfoFilter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-am9iLWVuZ2luZS9jbGllbnQvc3JjL21haW4vamF2YS9vcmcvZWNsaXBzZS9rYXB1YS9qb2IvZW5naW5lL2NsaWVudC9maWx0ZXIvU2Vzc2lvbkluZm9GaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | | | [...on/shiro/realm/AccessTokenAuthenticatingRealm.java](https://app.codecov.io/gh/eclipse/kapua/pull/4059?src=pr&el=tree&filepath=service%2Fsecurity%2Fshiro%2Fsrc%2Fmain%2Fjava%2Forg%2Feclipse%2Fkapua%2Fservice%2Fauthentication%2Fshiro%2Frealm%2FAccessTokenAuthenticatingRealm.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-c2VydmljZS9zZWN1cml0eS9zaGlyby9zcmMvbWFpbi9qYXZhL29yZy9lY2xpcHNlL2thcHVhL3NlcnZpY2UvYXV0aGVudGljYXRpb24vc2hpcm8vcmVhbG0vQWNjZXNzVG9rZW5BdXRoZW50aWNhdGluZ1JlYWxtLmphdmE=) | `14.66% <16.66%> (+0.38%)` | :arrow_up: |