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

Fixed: [sc-26355] Attempt to de-escalate SAML login and logout errors #15277

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

uberbrady
Copy link
Collaborator

The SAML login and logout process are both pretty solid, but they do occasionally throw errors when there's some kind of misconfiguration. This ends up resulting in error 500 pages being returned to the user rather than a 4xx. Additionally, it adds a lot of 'noise' to our internal Rollbar tooling, and those errors aren't ones that we, ourselves can fix - only the customer can.

This change wraps the login and logout methods within a try/catch, and dumps a warning in the Laravel log (rather than an 'error').

Additionally, one of the SAML methods had an incorrect signature in its docblock that was making PHPStorm slightly annoyed, so I fixed that while I was there.

I was able to test the 'happy path' here and do a SAML login and logout on my local which worked OK.

Copy link

what-the-diff bot commented Aug 13, 2024

PR Summary

  • Improved Exception Handling in SamlController
    We've strengthened the method to handle unexpected situations in SamlController. Now, both processResponse and processSLO methods are equipped with exception handlers in the acs and sls functions respectively.
  • Added Warning Logging
    In the event of any unexpected behavior, the system will now record these occurrences in a log with a warning level, providing us with valuable information to trace and resolve any issues.
  • Updated logging severity
    We've also adjusted the severity of error logs from error to a warning level. This change will help prioritize critical issues while ensuring we capture and record less critical but still significant incidents for review.
  • Improve user redirection
    The user redirection route after an operation in our acs function has been updated. We've also revised the error message, enhancing our communication with users during unforeseen circumstances.
  • Refined Comment and Return Type in Saml's getSetting Method
    The getSetting method in Saml has been made clearer by adding an author's comment. It's easier now to understand the wring and intent of the coder. Furthermore, we've also refined the return type of the method from void to mixed, adding flexibility and adaptability to the method's response.

app/Services/Saml.php Outdated Show resolved Hide resolved
@snipe
Copy link
Owner

snipe commented Aug 13, 2024

Looks great, thank you!

@snipe snipe merged commit 72fd997 into snipe:develop Aug 13, 2024
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants