Skip to content

Commit

Permalink
Issue #10295 - use ServletChannel states for the security handler dis…
Browse files Browse the repository at this point in the history
…patch

Signed-off-by: Lachlan Roberts <[email protected]>
  • Loading branch information
lachlan-roberts committed Aug 15, 2023
1 parent e9ab749 commit 8739610
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,18 @@ public String toString()
}
};

/**
* Authentication should be deferred, and request allowed to bypass security constraint.
*/
AuthenticationState DEFER = new AuthenticationState()
{
@Override
public String toString()
{
return "DEFER";
}
};

This comment has been minimized.

Copy link
@janbartel

janbartel Aug 18, 2023

Contributor

Its very confusing all these AuthenticationStates to do with deferment. What's the difference between this one and the Deferred extends AuthenticationState a bit further below?

This comment has been minimized.

Copy link
@lachlan-roberts

lachlan-roberts Aug 18, 2023

Author Contributor

I think this one should have been removed.
I will cleanup the unnecessary elements in this PR now.

This comment has been minimized.

Copy link
@lachlan-roberts

lachlan-roberts Aug 18, 2023

Author Contributor

It was already gone, this is an old commit.
But I have made some additional cleanups.

static Deferred defer(LoginAuthenticator loginAuthenticator)
{
return new DeferredAuthenticationState(loginAuthenticator);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -495,15 +495,14 @@ public boolean handle(Request request, Response response, Callback callback) thr
return true;
}

if (authenticationState == null)
if (authenticationState == null || authenticationState == AuthenticationState.DEFER)
authenticationState = _deferred;

AuthenticationState.setAuthenticationState(request, authenticationState);
IdentityService.Association association =
(authenticationState instanceof AuthenticationState.Succeeded user)
? _identityService.associate(user.getUserIdentity(), null) : null;


try
{
//process the request by other handlers
Expand Down Expand Up @@ -556,8 +555,10 @@ protected void redirectToSecure(Request request, Response response, Callback cal

protected boolean isNotAuthorized(Constraint constraint, AuthenticationState authenticationState)
{
UserIdentity userIdentity = authenticationState instanceof AuthenticationState.Succeeded user ? user.getUserIdentity() : null;
if (authenticationState == AuthenticationState.DEFER)
return false;

UserIdentity userIdentity = authenticationState instanceof AuthenticationState.Succeeded user ? user.getUserIdentity() : null;
return switch (constraint.getAuthorization())
{
case FORBIDDEN, ALLOWED, INHERIT -> false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@
/**
* FORM Authenticator.
*
* <p>This authenticator implements form authentication will use dispatchers to
* the login page if the {@link #__FORM_DISPATCH} init parameter is set to true.
* Otherwise it will redirect.</p>
*
* <p>The form authenticator redirects unauthenticated requests to a log page
* which should use a form to gather username/password from the user and send them
* to the /j_security_check URI within the context. FormAuthentication uses
Expand All @@ -56,7 +52,6 @@ public class FormAuthenticator extends LoginAuthenticator

public static final String __FORM_LOGIN_PAGE = "org.eclipse.jetty.security.form_login_page";
public static final String __FORM_ERROR_PAGE = "org.eclipse.jetty.security.form_error_page";
public static final String __FORM_DISPATCH = "org.eclipse.jetty.security.dispatch";
public static final String __J_URI = "org.eclipse.jetty.security.form_URI";
public static final String __J_POST = "org.eclipse.jetty.security.form_POST";
public static final String __J_METHOD = "org.eclipse.jetty.security.form_METHOD";
Expand Down Expand Up @@ -300,8 +295,7 @@ public AuthenticationState validateRequest(Request request, Response response, C
// not authenticated
if (LOG.isDebugEnabled())
LOG.debug("auth failed {}=={}", username, _formErrorPage);
sendError(request, response, callback);
return AuthenticationState.SEND_FAILURE;
return sendError(request, response, callback);
}

// Look for cached authentication
Expand Down Expand Up @@ -364,21 +358,22 @@ public AuthenticationState validateRequest(Request request, Response response, C
// send the challenge
if (LOG.isDebugEnabled())
LOG.debug("challenge {}->{}", session.getId(), _formLoginPage);
sendChallenge(request, response, callback);
return AuthenticationState.CHALLENGE;
return sendChallenge(request, response, callback);
}

protected void sendError(Request request, Response response, Callback callback)
protected AuthenticationState sendError(Request request, Response response, Callback callback)
{
if (_formErrorPage == null)
Response.writeError(request, response, callback, HttpStatus.FORBIDDEN_403);
else
Response.sendRedirect(request, response, callback, encodeURL(URIUtil.addPaths(request.getContext().getContextPath(), _formErrorPage), request), true);
return AuthenticationState.SEND_FAILURE;
}

protected void sendChallenge(Request request, Response response, Callback callback)
protected AuthenticationState sendChallenge(Request request, Response response, Callback callback)
{
Response.sendRedirect(request, response, callback, encodeURL(URIUtil.addPaths(request.getContext().getContextPath(), _formLoginPage), request), true);
return AuthenticationState.SEND_FAILURE;
}

public boolean isJSecurityCheck(String uri)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import java.util.concurrent.atomic.AtomicLong;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import org.eclipse.jetty.ee10.servlet.ServletRequestState.Action;
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpFields;
Expand Down Expand Up @@ -71,6 +73,9 @@
public class ServletChannel
{
private static final Logger LOG = LoggerFactory.getLogger(ServletChannel.class);
public static final String INITIAL_DISPATCH_REQUEST = "jetty.initial.dispatch.request";
public static final String INITIAL_DISPATCH_RESPONSE = "jetty.initial.dispatch.response";
public static final String INITIAL_DISPATCH_PATH = "jetty.initial.dispatch.path";

private final ServletRequestState _state;
private final ServletContextHandler.ServletScopedContext _context;
Expand Down Expand Up @@ -445,18 +450,18 @@ private void recycle()
_written = 0;
}


public void dispatched(Dispatchable dispatchable) throws Exception
/**
* <p>When this is called the initial dispatch will use the {@link ServletChannel#INITIAL_DISPATCH_PATH},
* {@link ServletChannel#INITIAL_DISPATCH_REQUEST} and {@link ServletChannel#INITIAL_DISPATCH_RESPONSE} attributes
* to do a {@link jakarta.servlet.DispatcherType#FORWARD} dispatch instead of the initial
* {@link jakarta.servlet.DispatcherType#REQUEST} dispatch.</p>
*
* <p>This must only be called before {@link ServletChannel#handle()} is first invoked.
* This can be used to dispatch to a different target before the initial request has been dispatched.</p>
*/
public void initialDispatch()
{
if (LOG.isDebugEnabled())
LOG.debug("handle {} {} ", _servletContextRequest.getHttpURI(), this);

Action action = _state.handling();
if (action != Action.DISPATCH)
throw new IllegalStateException(action.name());

dispatchable.dispatch();

_state.initialDispatch();
}

/**
Expand Down Expand Up @@ -498,6 +503,19 @@ public boolean handle()
break;
}

case INITIAL_DISPATCH:
{
HttpServletRequest request = (HttpServletRequest)_request.removeAttribute(INITIAL_DISPATCH_REQUEST);
HttpServletResponse response = (HttpServletResponse)_request.removeAttribute(INITIAL_DISPATCH_RESPONSE);
String path = (String)_request.removeAttribute(INITIAL_DISPATCH_PATH);

// TODO: Use listeners etc...
RequestDispatcher requestDispatcher = request.getRequestDispatcher(path);
requestDispatcher.forward(request, response);

break;
}

case ASYNC_DISPATCH:
{
dispatch(_asyncDispatchable);
Expand Down Expand Up @@ -903,7 +921,7 @@ public void abort(Throwable failure)
}
}

public interface Dispatchable
private interface Dispatchable
{
void dispatch() throws Exception;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ public class ServletRequestState
public enum State
{
IDLE, // Idle request
INITIAL_DISPATCH, // initialDispatch() has been called.
HANDLING, // Request dispatched to filter/servlet or Async IO callback
WAITING, // Suspended and waiting
WOKEN, // Dispatch to handle from ASYNC_WAIT
Expand Down Expand Up @@ -124,6 +125,7 @@ private enum OutputState
public enum Action
{
DISPATCH, // handle a normal request dispatch
INITIAL_DISPATCH, // initial request will be forwarded
ASYNC_DISPATCH, // handle an async request dispatch
SEND_ERROR, // Generate an error page or error dispatch
ASYNC_ERROR, // handle an async error
Expand Down Expand Up @@ -322,6 +324,21 @@ public boolean abortResponse()
}
}

public void initialDispatch()
{
try (AutoLock ignored = lock())
{
switch (_state)
{
case IDLE:
_state = State.INITIAL_DISPATCH;
break;
default:
throw new IllegalStateException(getStatusStringLocked());
}
}
}

/**
* @return Next handling of the request should proceed
*/
Expand All @@ -341,6 +358,13 @@ public Action handling()
_state = State.HANDLING;
return Action.DISPATCH;

case INITIAL_DISPATCH:
if (_requestState != RequestState.BLOCKING)
throw new IllegalStateException(getStatusStringLocked());
_initial = true;
_state = State.HANDLING;
return Action.INITIAL_DISPATCH;

case WOKEN:
if (_event != null && _event.getThrowable() != null && !_sendError)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.util.Enumeration;
import java.util.Locale;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletRequestWrapper;
import jakarta.servlet.http.HttpServletResponse;
Expand All @@ -26,6 +25,7 @@
import org.eclipse.jetty.ee10.servlet.ServletContextRequest;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpHeaderValue;
import org.eclipse.jetty.security.AuthenticationState;
import org.eclipse.jetty.security.authentication.SessionAuthentication;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Response;
Expand Down Expand Up @@ -70,44 +70,42 @@ public void setConfiguration(Configuration configuration)
}

@Override
protected void sendError(Request request, Response response, Callback callback)
protected AuthenticationState sendError(Request request, Response response, Callback callback)
{
if (_dispatch && getErrorPage() != null)
dispatch(getErrorPage(), request, response, callback);
return dispatch(getErrorPage(), request, response, callback);
else
super.sendError(request, response, callback);
return super.sendError(request, response, callback);
}

@Override
protected void sendChallenge(Request request, Response response, Callback callback)
protected AuthenticationState sendChallenge(Request request, Response response, Callback callback)
{
if (_dispatch)
dispatch(getLoginPage(), request, response, callback);
return dispatch(getLoginPage(), request, response, callback);
else
super.sendChallenge(request, response, callback);
return super.sendChallenge(request, response, callback);
}

private void dispatch(String path, Request request, Response response, Callback callback)
private AuthenticationState dispatch(String path, Request request, Response response, Callback callback)
{
try
{
// We are before the ServletHandler, so we must do the association.
ServletChannel servletChannel = Request.get(request, ServletContextRequest.class, ServletContextRequest::getServletChannel);
servletChannel.associate(request, response, callback);

response.getHeaders().put(HttpHeader.CACHE_CONTROL.asString(), HttpHeaderValue.NO_CACHE.asString());
response.getHeaders().putDate(HttpHeader.EXPIRES.asString(), 1);

ServletContextRequest contextRequest = Request.as(request, ServletContextRequest.class);
RequestDispatcher dispatcher = contextRequest.getServletApiRequest().getRequestDispatcher(path);
dispatcher.forward(new FormRequest(contextRequest.getServletApiRequest()), new FormResponse(contextRequest.getHttpServletResponse()));
contextRequest.setAttribute(ServletChannel.INITIAL_DISPATCH_PATH, path);
contextRequest.setAttribute(ServletChannel.INITIAL_DISPATCH_REQUEST, new FormRequest(contextRequest.getServletApiRequest()));
contextRequest.setAttribute(ServletChannel.INITIAL_DISPATCH_RESPONSE, new FormResponse(contextRequest.getHttpServletResponse()));
contextRequest.getServletChannel().initialDispatch();

// TODO: we need to run the ServletChannel.
callback.succeeded();
return AuthenticationState.DEFER;
}
catch (Throwable t)
{
Response.writeError(request, response, callback, t);
return AuthenticationState.SEND_FAILURE;
}
}

Expand Down

0 comments on commit 8739610

Please sign in to comment.