OpenSaml support should preserve encrypted elements for further analysis

[kimgoetzke]

Describe the bug
In an application where spring-security-saml2-service-provider was upgraded from 6.3.x to 6.4.1, I am experiencing a change in behaviour where I'm struggling to access the list of encrypted assertions in a validator class. I think this may be a bug.

I'm configuring a response validator in my configuration as Spring intends by doing this:

var authProvider = new OpenSaml4AuthenticationProvider();
authProvider.setResponseValidator(responseValidator);

Having received a SAML Response with a single encrypted assertion, in said responseValidator, I'd be able to do something like this:

class SamlResponseValidator implements Converter<ResponseToken, Saml2ResponseValidatorResult> {

  @Override
  public Saml2ResponseValidatorResult convert(ResponseToken responseToken) {
    // Without any other overrides, the below gives you the Spring-decrypted assertion:
    var assertions = responseToken.getResponse().getAssertions();
    
    // In 6.3, the below would _also_ contain the still-encrypted assertions while
    // in 6.4, you'll always just get an empty list, it seems:
    var encryptedAssertions = responseToken.getResponse().getEncryptedAssertions();

    // (...)
  }
}

I can see that this change comes from what is now OpenSaml4Template.OpenSaml4DecryptionConfigurer.decryptResponse(Response response) which will do this:

// ...

int count = 0;
int size = response.getEncryptedAssertions().size();
for (EncryptedAssertion encrypted : response.getEncryptedAssertions()) {
   logger.trace(String.format("Decrypting EncryptedAssertion (%d/%d) in Response [%s]", count, size, response.getID()));
   try {
      Assertion decrypted = this.decrypter.decrypt(encrypted);
      if (decrypted != null) {
         encrypteds.add(encrypted);
         decrypteds.add(decrypted);
      }
      count++;
   } catch (DecryptionException ex) {
      throw new Saml2Exception(ex);
   }
}

response.getEncryptedAssertions().removeAll(encrypteds); // <-- The cause of what I'm seeing!
response.getAssertions().addAll(decrypteds);

// ...

The Javadoc on the method states that:

The methods that follow are adapted from OpenSAML's [classes].

Previously, response.getEncryptedAssertions().removeAll(encrypteds) did not exist.

As a result, I believe there to be two bugs:

1. responseToken.getResponse().getEncryptedAssertions() no longer contains encrypted assertions when Spring decrypted them for you
2. int count = 0 in OpenSaml4Template.OpenSaml4DecryptionConfigurer.decryptResponse(Response response) results in incorrect log statements such as Decrypting EncryptedAssertion (0/1) in Response [{id}] when there's only one assertion

To Reproduce

1. Create a basic application that uses spring-security-saml2-service-provider in which you provide your own response validator with:

var authProvider = new OpenSaml4AuthenticationProvider();
authProvider.setResponseValidator(responseValidator);

2. Check assertions in the response validator:

class SamlResponseValidator implements Converter<ResponseToken, Saml2ResponseValidatorResult> {

  @Override
  public Saml2ResponseValidatorResult convert(ResponseToken responseToken) {
    var encryptedAssertions = responseToken.getResponse().getEncryptedAssertions();

    // (...)
  }
}

3. Execute the SAML flow with a SAML response containing an encrypted assertion and assert on/break point on encryptedAssertions
4. Notice that the value will be 0 instead of 1

Expected behavior

1. responseToken.getResponse().getEncryptedAssertions() should contain encrypted assertions
2. The log statement Decrypting EncryptedAssertion (%d/%d) in Response [%s] should log the correct count i.e. 1/1 if one encrypted assertion was found (instead of 0/1)

Sample
Because of 1) how basic this is and 2) this possibly being intentional, I have not included one yet but happy to add it if required.

Before creating this issue, I asked this as a Stack Overflow question but didn't get an answer.

Please tell me if I'm wrong or missing something! If you do agree, I don't mind creating a PR.

[jzheaux]

Thanks, @kimgoetzke, I agree that it would be helpful to add this back in, at least to maintain backward compatibility. We'll target the next point release for this.