LG-12085 Returns 400 when blank certificate element is passed through assertion

[Sgtpluck]

🎫 Ticket

LG-12085

🛠 Summary of changes

Adds a validation for whether the assertion has a blank certificate element.

There's probably a better way to do this, but creating a new validation in the SamlRequestValidator felt the cleanest in terms of being able to create an expected error for the user and return an event that tracks what's happening. Suggestions on other ways to do it welcome! Thanks.

[aduth]

Couple minor comments, but LGTM overall 👍

[aduth]

Somewhat related to my previous comment and probably out of scope for what we want to address here, but would we want to extract the other "validations" occurring in validate_saml_request to happen within our validator class / "bad request" handling as well? Specifically the valid_saml_request? check and Nokogiri::XML::SyntaxError rescue in that method.

Imagining something like...

def saml_request_validator
  saml_request = Request.from_deflated_request(raw_saml_request, get_params: params)
  if saml_request.valid?
    SamlRequestValidator.new # ...
  else
    SamlRequestValidator.new # ...
  end
rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError
  SamlRequestValidator.new # ...
rescue Nokogiri::XML::SyntaxError
  SamlRequestValidator.new # ...
end

Raising here only in case it would affect how we'd want to approach this generally, such as the snippet above avoiding the before_action altogether.

[Sgtpluck]

yes, long-term we definitely want to extract other validation errors, as well as specific certificate signature errors. (you can see some code i wrote to address that here: 18F/saml_idp#86).

however, there are a lot of unknowns around saml assertion changes (and can really balloon quickly in complexity), and this ticket is fairly tightly scoped to address the 500s that have been occurring regularly, so i do think this suggestion is out of scope of this particular ticket. better errors/clearer code is something i am hoping to have added as part of the protocols team roadmap when it spins up!

[nprimak]

LGTM!