Skip to content

Commit

Permalink
Refactor messageContext initialization for enhanced scope availabilit…
Browse files Browse the repository at this point in the history
…y in SamlAssertionConsumerFunction (line#5622)

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 line#5401 
- The refactor ensures messageContext is available for error handling.
  • Loading branch information
yeojin-dev authored and Dogacel committed Jun 8, 2024
1 parent 505ae9a commit 0592ef8
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,10 @@ final class SamlAssertionConsumerFunction implements SamlServiceFunction {
@Override
public HttpResponse serve(ServiceRequestContext ctx, AggregatedHttpRequest req,
String defaultHostname, SamlPortConfig portConfig) {
MessageContext<Response> messageContext = null;

try {
final SamlBindingProtocol bindingProtocol = cfg.endpoint().bindingProtocol();
final MessageContext<Response> messageContext;
if (bindingProtocol == SamlBindingProtocol.HTTP_REDIRECT) {
messageContext = HttpRedirectBindingUtil.toSamlObject(req, SAML_RESPONSE,
idpConfigs, defaultIdpConfig,
Expand All @@ -116,7 +117,7 @@ public HttpResponse serve(ServiceRequestContext ctx, AggregatedHttpRequest req,

return ssoHandler.loginSucceeded(ctx, req, messageContext, sessionIndex, relayState);
} catch (SamlException e) {
return ssoHandler.loginFailed(ctx, req, null, e);
return ssoHandler.loginFailed(ctx, req, messageContext, e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@
import java.util.Set;
import java.util.concurrent.CompletionStage;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Pattern;

import org.joda.time.DateTime;
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;
import org.jsoup.nodes.Element;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.opensaml.core.criterion.EntityIdCriterion;
Expand Down Expand Up @@ -115,6 +117,9 @@
@GenerateNativeImageTrace
class SamlServiceProviderTest {

private static final AtomicReference<MessageContext<Response>> messageContextHolder =
new AtomicReference<>();

private static final String signatureAlgorithm = SignatureConstants.ALGO_ID_SIGNATURE_RSA;

private static final String spHostname = "localhost";
Expand Down Expand Up @@ -289,6 +294,7 @@ public HttpResponse loginSucceeded(ServiceRequestContext ctx, AggregatedHttpRequ
@Override
public HttpResponse loginFailed(ServiceRequestContext ctx, AggregatedHttpRequest req,
@Nullable MessageContext<Response> message, Throwable cause) {
messageContextHolder.set(message);
// Handle as an error so that a test client can detect the failure.
return HttpResponse.of(HttpStatus.BAD_REQUEST);
}
Expand All @@ -315,6 +321,11 @@ public boolean validateId(String id) {

final WebClient client = WebClient.of(server.httpUri());

@BeforeEach
void setup() {
messageContextHolder.set(null);
}

@Test
void shouldRespondAuthnRequest_HttpRedirect() throws Exception {
final AggregatedHttpResponse resp = client.get("/redirect").aggregate().join();
Expand Down Expand Up @@ -436,6 +447,8 @@ void shouldNotConsumeAssertionWithInvalidSignature_HttpRedirect() throws Excepti
@Test
void shouldNotConsumeAssertionWithoutSignature_HttpPost() throws Exception {
shouldNotConsumeAssertion_HttpPost(null);
final MessageContext<Response> messageContext = messageContextHolder.get();
assertThat(messageContext.getMessage().getSignature()).isNull();
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions site/src/pages/docs/advanced-saml.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ attach it to your <type://Server>.
```java
// Specify information about your keystore.
// The keystore contains two key pairs, which are identified as 'signing' and 'encryption'.
KeyStoreCredentialResolver credentialResolver =
CredentialResolver credentialResolver =
new KeyStoreCredentialResolverBuilder(ClassLoader.getSystemClassLoader(),
"sample.jks")
.type("PKCS12")
.password("N5^X[hvG")
// You need to specify your key pair and its password here.
.addKeyPassword("signing", "N5^X[hvG")
.addKeyPassword("encryption", "N5^X[hvG")
.keyPassword("signing", "N5^X[hvG")
.keyPassword("encryption", "N5^X[hvG")
.build();

SamlServiceProvider ssp =
Expand Down

0 comments on commit 0592ef8

Please sign in to comment.