vertx-web icon indicating copy to clipboard operation
vertx-web copied to clipboard

ChainAuthHandler does not invoke the post-authentication method

Open rzorzorzo opened this issue 2 years ago • 6 comments

Which version(s) did you encounter this bug ? 4.4.0

Do you have a reproducer?

ChainAuthHandler authHandler = ChainAuthHandler.any(); authHandler.add(formHandler);

successful login returns http status 405

the problem is, that ChainAuthHandler.postAuthentication is invoked instead of FormLoginHandler.postAuthentication

I suppose that ChainAuthHandler.postAuthentication should delegate to FormLoginHandler.postAuthentication. Currently ctx.next() is invoked which results in 405.

rzorzorzo avatar Apr 01 '23 06:04 rzorzorzo

@rzorzorzo I've added this test locally to reproduce your case:

// file: ChainAuthHandlerTest.java

  @Test
  public void testWithFormAuth() throws Exception {
    router.clear();

    router.route()
      .handler(SessionHandler.create(LocalSessionStore.create(vertx)))
      .handler(BodyHandler.create())
      .handler(ChainAuthHandler.any()
        .add(FormLoginHandler.create(authProvider)))
      .handler(ctx -> ctx.response().end());

    testRequest(HttpMethod.POST, "/login", sendLoginRequestConsumer(), resp -> {
      // session will be upgraded
      String setCookie = resp.headers().get("set-cookie");
      assertNotNull(setCookie);
    }, 200, "OK", null);
  }

  private Consumer<HttpClientRequest> sendLoginRequestConsumer() {
    return req -> {
      String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
      Buffer buffer = Buffer.buffer();
      String str =
        "--" + boundary + "\r\n" +
          "Content-Disposition: form-data; name=\"" + FormLoginHandler.DEFAULT_USERNAME_PARAM + "\"\r\n\r\ntim\r\n" +
          "--" + boundary + "\r\n" +
          "Content-Disposition: form-data; name=\"" + FormLoginHandler.DEFAULT_PASSWORD_PARAM + "\"\r\n\r\ndelicious:sausages\r\n" +
          "--" + boundary + "--\r\n";
      buffer.appendString(str);
      req.putHeader("content-length", String.valueOf(buffer.length()));
      req.putHeader("content-type", "multipart/form-data; boundary=" + boundary);
      req.write(buffer);
    };
  }

It seems this isn't reproducible on master (5.0.0-SNAPSHOT) can you confirm that the test is correct?

pmlopes avatar Apr 04 '23 14:04 pmlopes

thanks for your reply. tested it with 5.0.0-snapshot which does not seem to resolve the issue for me. here a reproducer:


import io.vertx.core.AbstractVerticle;
import io.vertx.core.Future;
import io.vertx.core.Vertx;
import io.vertx.core.http.HttpServer;
import io.vertx.ext.auth.User;
import io.vertx.ext.auth.authentication.AuthenticationProvider;
import io.vertx.ext.auth.authentication.Credentials;
import io.vertx.ext.web.Router;
import io.vertx.ext.web.handler.BodyHandler;
import io.vertx.ext.web.handler.ChainAuthHandler;
import io.vertx.ext.web.handler.FormLoginHandler;
import io.vertx.ext.web.handler.RedirectAuthHandler;
import io.vertx.ext.web.handler.SessionHandler;
import io.vertx.ext.web.sstore.LocalSessionStore;

public class WebServer extends AbstractVerticle {

	private static final int KB = 1024;
	private static final int MB = 1024 * KB;

	@Override
	public void start() throws Exception {
		super.start();
		Router router = Router.router(vertx);
		
		router.errorHandler(500, ctx -> ctx.end());
		router.route().handler(BodyHandler.create().setBodyLimit(5 * MB));
		router.route().handler(SessionHandler.create(LocalSessionStore.create(vertx)));
		ChainAuthHandler chainAuth = ChainAuthHandler.any();
		AuthenticationProvider authProvider = new AuthenticationProvider() {
			
			@Override
			public Future<User> authenticate(Credentials credentials) {
				if (credentials.toJson().getString("username").equals("a"))
				return Future.succeededFuture(User.fromName("a"));
				else
				return Future.failedFuture("bad user");
			}
		};
		FormLoginHandler formLoginHandler = FormLoginHandler.create(authProvider);
		formLoginHandler.setDirectLoggedInOKURL("/welcome");
		chainAuth.add(formLoginHandler);
		router
		  .get("/welcome")
		  .handler(RedirectAuthHandler.create(authProvider));
		
		router.post("/login").handler(formLoginHandler); // login ok
		//router.post("/login").handler(chainAuth); // login nok
		router.get("/welcome").handler(ctx -> ctx.end("welcome"));
		HttpServer server = vertx.createHttpServer();
		server.requestHandler(router);
		server.listen(8099).onSuccess(s -> System.out.println("listening on port: "+s.actualPort())).onFailure(ex -> ex.printStackTrace());
	}
	
	public static void main(String[] args) {
		Vertx vertx = Vertx.vertx();
		vertx.deployVerticle(new WebServer()).onSuccess(txt  -> System.out.println("ready")).onFailure(ex -> ex.printStackTrace());
	}
}

rzorzorzo avatar Apr 15 '23 14:04 rzorzorzo

PS: here the login request:

curl "http://localhost:8099/login" -X POST -H "User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/111.0" -H "Accept: /" -H "Accept-Language: en-US,en;q=0.5" -H "Accept-Encoding: gzip, deflate" -H "content-type: application/x-www-form-urlencoded" -H "Origin: moz-extension://02d938a1-ba5b-450d-aa3a-be5f83bed098" -H "Connection: keep-alive" --data-raw "username=a&password=b"`

rzorzorzo avatar Apr 15 '23 14:04 rzorzorzo

Happy to find this here and to see that someone else stumbled upon this. 🙂

I've the same issue with the JWTAuthHandler wrapped in a ChainAuthHandler.

vert.x version: 4.4.1

Since ChainAuthHandlerImpl extends AuthenticationHandlerImpl (which implements AuthenticationHandlerInternal) but itself doesn't override postAuthentication and also doesn't invoke postAuthentication on any of the handlers in the chain, I assume it will always only call the default implementation in the AuthenticationHandlerInternal interface.

And as it's implemented right now I'm not sure how this could be changed to work as one would expect... Because if there's another handler in the chain, ChainAuthHandler::iterate has to be called -> this part here.

authHandler.postAuthentication(ctx) can't be invoked here, because that might invoke the default implementation ctx.next() - which would skip the next auth handler in the chain, right?

But authHandler.postAuthentication(ctx) should be invoked before the next handler in the chain, right? Because for example in the JWTAuthHandler it checks the OAuth scopes - which has to be done.

Or do I miss something important here?

mf81bln avatar May 09 '23 21:05 mf81bln

Just noticed that this behaviour also leads to multiple security definitions for the same operation not working, like in the following example:

paths:
  '/v1/users':
    get:
      tags:
        - users
      summary: fetch users
      operationId: fetchUsers
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/User'
      security:
        - oauth:
            - 'users:read'
        - oauth:
            - 'users:all'

Desired behaviour:

  • user/client with scope users:read has access to this specific endpoint
  • user/client with scope users:all has access to this endpoint and everything else

Because of the "multi auth" security here a ChainAuthHandler is invoked - and the scopes are no longer validated.

mf81bln avatar May 10 '23 18:05 mf81bln

Thanks @rzorzorzo for the report and @mf81bln for sharing your findings. A fix is being tested in #2582

tsegismont avatar Mar 14 '24 14:03 tsegismont

Fixed by f8565123f

tsegismont avatar Mar 20 '24 09:03 tsegismont

I have similar operation like in https://github.com/vert-x3/vertx-web/issues/2408#issuecomment-1542635585. Which resolves to a ChainAuthHandler having two JWTAuthHandlerImpl. But if first requirement fails with scopes, context is failed and 2nd one's scopes not checked. Because JWTAuthHandlerImpl at this line fails the context, ChainAuthHandler cannot handle chain logic. https://github.com/vert-x3/vertx-web/blob/a6d16eb0e7f885755527be51d976020a118ea933/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/JWTAuthHandlerImpl.java#L154

ayhanap avatar Apr 18 '24 03:04 ayhanap

@ayhanap do you mean the JWTAuthHandler does not play nice for inclusion in a ChainAuthHandler? In this case, can you please file a separate issue for requesting the enhancement?

tsegismont avatar May 24 '24 15:05 tsegismont

@tsegismont Yes, actually before this update ChainAuthHandler with two JWTAuthHandler wasn't checking scopes because JWTAuthHandler checks scopes at postAuthentication.

The situation now is a ChainAuthHandler.any() with two JWTAuthHandler:

  • If ChainAuthHandler is successfully authenticates first JWTAuthHandler but if scopes don't match for first, at postAuthenticate it fails the vertx context because scopes does not match. Although second JWTAuthHandler can also authenticate and it can also pass scope check too, ChainAuthHandler did only check first one because it was successful.

I will try to supply a reproducer test.

ayhanap avatar May 29 '24 15:05 ayhanap