-
Notifications
You must be signed in to change notification settings - Fork 3
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
ENG-12107: Add missing validation to idp saml resource #421
Conversation
22ec445
to
53c0398
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wcmjunior Thanks for adding this validation! I left some comments that I think can help improve the code. Do you think we could do that before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, I have a few refactoring suggestions below that I think could be useful.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the suggestions @VictorGFM and @Yowgf. I only skipped the bigger function refactoring suggested by @Yowgf, but all the others made sense. Here are the results of the tests after refactoring with your suggestions:
|
Description of the change
A missing validation step caused the generic SAML integrations to error out during user log in.
Type of change
Checklists
Development
Code review
Testing