jakartaee / authentication

Jakarta Authentication
https://eclipse.org/ee4j/jaspic
Other
24 stars 33 forks source link

ServletProfileSPITest#VerifyRequestDispatchedProperly fails due to Authorization header not being set #227

Open jhanders34 opened 1 week ago

jhanders34 commented 1 week ago

Challenged tests ee.jakarta.tck.authentication.test.basic.ServletProfileSPITest#VerifyRequestDispatchedProperly

TCK Version Jakarta Authentication 3.1.0

Description This test is one of them that was converted from the old tck format. In the old format when sending a request with credentials, the Authorization header was set as seen here. In the updated framework, a CredentialsProvider is set instead of setting the Authorization header when using the responseFromServerWithCredentials method as seen here. The way that CredentialsProvider works with Apache HttpClient is that it only sets the credential if a 401 is returned. The test is not expecting a 401 to be returned to request the credential to be provided.

Additional context To resolve the issue, ArquillianBase can be updated as such:

   protected WebResponse responseFromServerWithCredentials(String path, String username, String password) {
        getWebClient().addRequestHeader(
            "Authorization",
            "Basic " + Base64.getEncoder()
                             .encodeToString((username + ":" + password).getBytes()));

        return responseFromServer(path);
    }

Similarly I think it is prudent to also update readFromServerWithCredentials as such

    protected String readFromServerWithCredentials(String path, String username, String password) {
        getWebClient().addRequestHeader(
            "Authorization",
            "Basic " + Base64.getEncoder()
                             .encodeToString((username + ":" + password).getBytes()));

        return readFromServer(path);
    }

A pull request is associated with this issue that has the above change in it that can be used if this challenge is accepted.

arjantijms commented 1 week ago

I can accept the challenge, and the proposed change seems very reasonable.

I just wonder, why did the migrated test currently pass on the ratifying compatible implementation (GlassFish)? Is it a case of the 401 being sent transparently in GlassFish, but not in Liberty? Or is there something else going on?

Maybe in a next major version of the TCK we need to be more explicit when testing pre-emptive authentication vs authentication after a challenge?

jhanders34 commented 1 week ago

I just wonder, why did the migrated test currently pass on the ratifying compatible implementation (GlassFish)? Is it a case of the 401 being sent transparently in GlassFish, but not in Liberty? Or is there something else going on?

Liberty has a registry as well that things are validated against beyond the Jakarta authentication check. With the way that the TCK works, if there is not a Principal in a Subject, it still returns a user name of empty string instead of null in ServerCallbackSupport. This empty string is passed to CallerPrincipalCallback to be a user name of empty string. Liberty does not like a user name of empty string. That is as far as I went to debug what is going on with this test.

Otherwise if Glassfish is just validating using authentication plug-ins and not doing any other validation on a user name, that could explain why they are not seeing the problem. Also if an empty string is treated special on glassfish that could also explain it. I do not know any of the glassfish code, so I can only speculate.