-
Notifications
You must be signed in to change notification settings - Fork 740
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
feat(saml): Update SAML to use Spring Security #1744
Conversation
This updates `gate-saml` to use the built-in SAML support in Spring Security which uses OpenSAML 4.x. Nearly all the previously supported properties for SAML are still supported, though a couple niche options no longer seem to be configurable. This also introduces `AuthenticationService`, a variation of `PermissionService` which can also return a user's granted authorities in one login call. It was also used for exception translation previously as retrofit exceptions are not serializable which would cause errors in Spring Security authentication failure error handlers, but the underlying exception being thrown has since been updated to avoid that problem.
dependencies{ | ||
dependencies { | ||
constraints { | ||
implementation 'org.opensaml:opensaml-core:4.1.0' |
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.
I'll note that I tried using a newer release of OpenSAML, but it didn't seem to work with the version of Spring Security I was testing against at the time. With a new enough version of Spring Security, we shouldn't need to override this dependency anymore (this older version of Spring Security supports both OpenSAML 3 and 4).
RelyingPartyRegistrations.fromMetadataLocation(properties.getMetadataUrl()) | ||
.registrationId(properties.getRegistrationId()) | ||
.entityId(properties.getIssuerId()) | ||
.assertionConsumerServiceLocation(properties.getAssertionConsumerServiceLocation()); |
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.
Minor, but we may want:
.assertingPartyDetails({ party -> party.wantAuthnRequestsSigned(false) })
I'd have to go back check on the requirements on this and whether this was ignored, still digging though. I recall hitting this. OR make it an option. This was comparing fairly low level DSL to look for potential SAML loading changes.
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.
Not really sure how that works; I always use encryption in SAML instead.
This updates
gate-saml
to use the built-in SAML support in Spring Security which uses OpenSAML 4.x. Nearly all the previously supported properties for SAML are still supported, though a couple niche options no longer seem to be configurable.This also introduces
AuthenticationService
, a variation ofPermissionService
which can also return a user's granted authorities in one login call. It was also used for exception translation previously as retrofit exceptions are not serializable which would cause errors in Spring Security authentication failure error handlers, but the underlying exception being thrown has since been updated to avoid that problem.