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

Feedback: More strongly typed errors as means to provide better diagnostics #250

Open
ricardofandrade opened this issue Jan 17, 2020 · 1 comment

Comments

@ricardofandrade
Copy link
Contributor

ricardofandrade commented Jan 17, 2020

Not always the error message coming from SAML makes sense in a broader context.
That's where our application sometimes struggles to offer a better diagnostic for users.

One example is the error that occurs when the configured signing certificate (in the IdP metadata) differs from the one in the received assertion:
cannot validate signature on Response: Could not verify certificate against trusted certs

The message seems sufficient in the context of SAML but we'd love to offer more insights for a quicker resolution, for example stating that metadata may need to be updated.

There's always room for improvement in error messages but we have no expectation that this package be responsible for provide such high level diagnostic in all cases. However, providing means for applications to handle errors selectively seems something within the reach of the package.

Currently to achieve this goal, we would have to watch for a particular error message with a logic based on string matching, which tends to be fragile. Instead, we'd rather match a particular error type.

In the last few years, the Go community has been moving in that direction:
https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully
https://golang.org/doc/go1.13#error_wrapping
https://github.com/golang/go/wiki/ErrorValueFAQ

There are few examples of this pattern in the existing code:
https://github.com/crewjam/saml/blob/master/service_provider.go#L421
https://github.com/crewjam/saml/blob/master/service_provider.go#L405

We're looking forward to see more of those.

P.S: I made the same suggestion to russellhaering/goxmldsig#53 which originates the error message mentioned above.

@crewjam
Copy link
Owner

crewjam commented Jan 18, 2020

This sounds great! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants