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

Increase Usage of Warnings Across Response Parsing #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jahidadam
Copy link

As I mentioned in #31, the InvalidTime warning was being thrown in response to one NotOnOrAfter problem, and an error was being thrown in response to another one. I think those two should be consistent, and I've done a slight refactor so that they can both throw the warning.

My particular use case, and why I think they should be warnings as opposed to errors, is that I'm using the assertion itself to store session at the SP. This means that I'd like to be able to choose when to respect those NotOnOrAfter times (when I first see an assertion) and when to ignore them (when they're stored as session, so I use SessionNotOnOrAfter instead).

In general, I think the design of all SAML-aware parsing functions being able to set warnings if they need to is an improvement, since it'll make future requests similar to this one much easier to implement.

Note that this change doesn't affect the basic use case of parsing an assertion with RetrieveAssertionInfo, since a responsible user would already be checking the warning if they cared about the time.

@russellhaering
Copy link
Owner

Thanks for tackling this refactor, and sorry for taking so (very, very) long to review it.

I'm generally positive on giving callers more flexibility to decide how to handle (or to ignore) certain errors, but the current WarningInfo API worries me; I think it's too easy for callers to fail to check warnings, and if we add additional warnings in the future existing callers will need to explicitly add code to check them (and likely won't know to do so). Given that, I'm nervous about relying even more heavily on it yet.

I've been contemplating some kind of "error mask" API, which permits callers to explicitly opt out of specific errors in favor of warnings. By default we would error out if NotInAudience or InvalidTime is true; I'm not yet sure the best way to handle OneTimeUse and ProxyRestrictions, which don't necessarily seem like warnings at all, more like some kind of restrictions on use.

I'm going to start a branch to experiment with the error mask idea, and I'll pull this PR in to that. The code looks good, and this approach (gathering warnings instead of erroring out early) is definitely an improvement, so again, thanks for taking the time to do this.

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.

2 participants