Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jhanders34 opened this issue Nov 20, 2024 · 2 comments
Labels
challenge TCK challenge

Comments

@jhanders34
Copy link

jhanders34 commented Nov 20, 2024

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
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
challenge TCK challenge
Projects
None yet
Development

No branches or pull requests

2 participants