Refactor messageContext initialization for enhanced scope availability in SamlAssertionConsumerFunction

[yeojin-dev]

Motivation:

This PR addresses a code enhancement in the SamlAssertionConsumerFunction by initializing the messageContext variable at the beginning of the method. This change is intended to streamline the assignment and handling of messageContext, ensuring that it can be consistently used throughout the method, particularly in exception handling scenarios.

Modifications:

Declared messageContext at the method start to allow its use across the entire method scope, including within try and catch blocks.

Result:

• Closes Provide debug hook point to trace original saml callback response #5401
• The refactor ensures messageContext is available for error handling.

[github-actions]

🔍 Build Scan® (commit: 545f515)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/2ymw3n2j3wdzi
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/acv7pjpp3erye
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/6pokl2pyiwz6i
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/dcxmev2t7lthu
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/3kxosxbdgf2yw
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/m23ectquxlwwe
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/rqxrlywphbrgu
build-macos-12-jdk-21	❌ (failure)	https://ge.armeria.dev/s/emjoksw4mkxdi

[jrhee17]

Left one nit but looks solid. Thanks @yeojin-dev ! 🙇 👍 🙇

[jrhee17]

I'm guessing this is a mistake since CredentialResolver doesn't exist

Suggested change

	CredentialResolver credentialResolver =
	KeyStoreCredentialResolver credentialResolver =

[yeojin-dev]

Then I think we need to change this one too. :)

armeria/examples/saml-service-provider/src/main/java/example/armeria/server/saml/sp/Main.java

Line 55 in 3d800a2

final CredentialResolver credentialResolver =

[jrhee17]

Oops nevermind, I was looking at the wrong part of the code. The fix looks good 👍

[jrhee17]

👍 👍 👍

[ikhoon]

Thanks, @yeojin-dev!

[minwoox]

Could you add this messageContext to the log message?

armeria/saml/src/main/java/com/linecorp/armeria/server/saml/SamlServiceProviderBuilder.java

Line 129 in b2aa9f4

logger.warn("{} SAML SSO failed", ctx, cause);

Could you also add a simple test for this?

[yeojin-dev]

OK, I'll. 🚀

[yeojin-dev]

Creating test cases for the use of messageContext is challenging due to the internal use of static methods (HttpRedirectBindingUtil and HttpPostBindingUtil) in the serve() function. While mocking static functions is possible, it is not a best practice and is not utilized by Armeria. To improve the design for easier testing, I propose the creation of SamlBindingProcessorFactory and SamlBindingProcessor interfaces. This structure allows for more flexible and maintainable code. Below is a brief example of how these interfaces could be implemented:

public interface SamlBindingProcessor {
    MessageContext<Response> toSamlObject(AggregatedHttpRequest request, String responseType);
}

public interface SamlBindingProcessorFactory {
    SamlBindingProcessor create(SamlBindingProtocol bindingProtocol);
}

public class DefaultSamlBindingProcessorFactory implements SamlBindingProcessorFactory {
    @Override
    public SamlBindingProcessor create(SamlBindingProtocol bindingProtocol) {
        switch (bindingProtocol) {
            case HTTP_REDIRECT:
                return new HttpRedirectBindingProcessor();
            case HTTP_POST:
                return new HttpPostBindingProcessor();
            default:
                throw new IllegalArgumentException("Unsupported binding protocol");
        }
    }
}

public class SamlAssertionConsumerFunction {
    private SamlBindingProcessor processor;

    public SamlAssertionConsumerFunction(SamlAssertionConsumerConfig cfg, String entityId,
                                         SamlBindingProcessorFactory processorFactory) {
        this.processor = processorFactory.create(cfg.endpoint().bindingProtocol());
        // Use the processor like this:
        // MessageContext<Response> messageContext = processor.toSamlObject(req, SAML_RESPONSE);
    }
}

These changes necessitate more extensive modifications than what is currently proposed in the PR. Please review and let me know if there are any misunderstandings or further adjustments needed. :)

[minwoox]

Oops, I thought we could just use the existing test.
For example, we could probably capture the messageContext here and use it in the test where the login fails.
https://github.com/minwoox/armeria/blob/main/saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java#L274

SamlBindingProcessorFactory and SamlBindingProcessor seems well-designed, but I'm uncertain whether customization is necessary. 🤔

[minwoox]

@yeojin-dev, I've added a test. 😉
8f87ec2

[trustin]

LGTM once @minwoox's comment (add tests) is addressed. Thanks, @yeojin-dev!

[minwoox]

Thanks a lot, @yeojin-dev! 😄