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

Expose validation exception in SamlResponse #253

Merged

Conversation

petenattress
Copy link
Contributor

Hi, we have a requirement in our product to take different actions based on the exact error with the SAML response (in our case, show the user different documentation pages on how they can fix the problem).

The best way I found to do this which doesn't rely on string matching was to expose the actual exception as in this PR, then use ValidationException#getErrorCode to drive the logic.

I have not updated any tests yet, but am happy to do that if you agree this is an acceptable change.

@coveralls
Copy link

coveralls commented Feb 19, 2020

Coverage Status

Coverage increased (+0.02%) to 94.581% when pulling 56cea3f on petenattress:expose-validation-exception into bed44f3 on onelogin:master.

}
return null;
}

/**
* After execute a validation process, if fails this method returns the cause
Copy link
Contributor

@pitbulk pitbulk Feb 20, 2020

Choose a reason for hiding this comment

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

After execute a validation process, if fails this method returns the Exception object

@pitbulk
Copy link
Contributor

pitbulk commented Feb 20, 2020

It's reasonable.

You may update LogoutRequest and LogoutResponse as well

And you should provide a getter to the Auth class in order to provide a similar method than getLastErrorReason , the method that returns the errorReason. You will see it updated on processResponse and processSLO -> 1 and 2

Test coverage for the new getValidationException methods should be provided as well

@petenattress
Copy link
Contributor Author

@pitbulk thank you for the feedback, I will make those changes as soon as I have some spare time. For the test coverage, will it be sufficient to just have one test for each new getValidationException method, or would you like me to add an assertion matching each call to getError, e.g.

assertEquals("SAML Response must contain 1 Assertion.", samlResponse.getError());
assertEquals(ValidationError.WRONG_NUMBER_OF_ASSERTIONS, ((ValidationError) samlResponse.getValidationException()).getErrorCode());

@pitbulk
Copy link
Contributor

pitbulk commented Feb 21, 2020

I understand 1 getValidationException per getError appearance is ideal, but you can simply cover the method.

Also add unit tests for validation exceptions
@petenattress
Copy link
Contributor Author

@pitbulk updated with tests and additional exception getters as you described.

@pitbulk pitbulk merged commit 26a29da into SAML-Toolkits:master Feb 24, 2020
@pitbulk
Copy link
Contributor

pitbulk commented Feb 24, 2020

Thanks for contributing

@petenattress
Copy link
Contributor Author

@pitbulk no problem, thanks for the useful library :)

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