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

Adding a SAML Validation error when there is no registered certs #11609

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

Sgtpluck
Copy link
Member

@Sgtpluck Sgtpluck commented Dec 6, 2024

🎫 Ticket

Add "No Certs" Error

🛠 Summary of changes

Slack conversation here
Ursula bug ticket here

We have validations in the identity-idp-config repo to prevent partners from deploying non-pkce integrations without certificates to prod. However, we do not have those validations in the Partner Portal (because sometimes a partner doesn't have all the finalized details before creating an integration.)

This means that a partner could attempt to test an integration without a certificate, which currently blows up when we attempt to encrypt the response. This change adds a specific validation to ensure that a requesting service provider has registered a certificate, and if it has not, it returns an error.

@Sgtpluck Sgtpluck requested a review from a team December 6, 2024 18:18
@@ -776,6 +776,7 @@ errors.messages.invalid_recaptcha_token: Lo sentimos, pero es posible que tu com
errors.messages.invalid_sms_number: El número de teléfono ingresado no admite mensajes de texto. Intente la opción de llamada telefónica.
errors.messages.invalid_voice_number: Número de teléfono no válido. Verifique haber ingresado el código de país o de área correcto.
errors.messages.missing_field: Llene este campo.
errors.messages.no_cert_registered: No podemos detectar un certificado en su solicitud.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the non-english versions of this error are just the blank_cert_element_req errors. since these will not be seen by users in prod, i think this should be fine (the errors won't make much sense to users in either case) -- but i am asking the UX team if they feel strongly about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they gave me the all-clear!

@h-m-m
Copy link
Contributor

h-m-m commented Dec 6, 2024

I'm satisfied that this checks for the condition we were concerned about.

As someone who's not too up on IDP, where could I expect to see this error after it happens?

@Sgtpluck
Copy link
Member Author

Sgtpluck commented Dec 6, 2024

@h-m-m the error happens in the same error page where you'd see all our current SAML integration errors. if you'd like to test it out:

  • check out my branch
  • run your local IdP and saml sample application
  • go into the rails console and remove the existing cert
sp = ServiceProvider.find_by(issuer: 'urn:gov:gsa:SAML:2.0.profiles:sp:sso:localhost')
certs = sp.certs
sp.update(certs: nil)
# don't exit out! you can re-add the certs after you test it
  • try to log in from your local saml app.

cleanup, in the rails console:

sp.update(certs: certs)

@Sgtpluck Sgtpluck merged commit 31ed8e2 into main Dec 6, 2024
2 checks passed
@Sgtpluck Sgtpluck deleted the dmm/no-cert-error branch December 6, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants