As I'm partially customising the SAML configuration, it would be very useful to be able to make my config also conditional on org.springframework.boot.autoconfigure.security.saml2.RegistrationConfiguredCondition
, rather than having to duplicate it.
Comment From: wilkinsona
We wouldn't make the condition itself public, but provide a public annotation that references the condition. That said, without significant demand, I'm not sure that it's something that we'd want to do and copy-pasting the code may be your best option. To help us to decide, can you please describe in a bit more detail what you're trying to do and the role that the condition would perform in that.
Comment From: OrangeDog
I'm trying to have service(s) that can optionally have SAML enabled via application properties.
I am building my own RelyingPartyRegistrationRepository
, so I cannot use @ConditionalOnBean
. There's only a single properly that triggers SAML configuration, but it's a map hence why you have a RegistrationConfiguredCondition
in the first place instead of being able to use @ConditionalOnProperty
.
What I have done is define my own saml.enabled
boolean property to use, but that's not the best because Boot's autoconfiguration ignores it.
Comment From: philwebb
Thanks for the additional information @OrangeDog. I'm afraid I don't think we should make RegistrationConfiguredCondition
public in our own code. I'd rather keep that code internal.
Give that the class is only about 60 lines I would recommend taking the copy/paste approach that @wilkinsona suggested.
Comment From: OrangeDog
Fine I guess, but I still have to check and re-copy it every time you change it.
Comment From: philwebb
If it's any consolation, the class hasn't had any functional changes since it was written 5 years ago.
Comment From: gbaso
I found this issue looking for the same problem. While I understand the decision to keep the condition class, it's not really satisfying since the corresponding oauth2 condition is indeed public: ClientsConfiguredCondition.
Would it be possible to reconsider the decision? Or, alternatively, add public annotations that reference the conditions?
Comment From: philwebb
ClientsConfiguredCondition
is public because it's used by other packages so we couldn't make it package-private. Having said that, the inconsistency is a bit annoying. I'm going to reopen this one to discuss further if we should make the class public or add an annotation.
Comment From: philwebb
We discussed this today and we'd like to hide the ClientsConfiguredCondition
behind a @ConditionalOnClientsConfigured
annotation which we'll make public. We'd actually like to go further and make sure that all *Condition
classes are package-private and have an annotation.
Comment From: wilkinsona
A first pass at this can be found in this branch. I'd like to get some feedback from the team on the names of the new annotations:
@ConditionalOnEnabledDevTools
@ConditionalOnIssuerLocationJwtDecoder
@ConditionalOnOAuth2ClientRegistrationConfigured
@ConditionalOnPublicKeyJwtDecoder
We should also consider if we want to list the new annotations in the relevant section of the docs.
Comment From: OrangeDog
My 2¢ are that the words Enabled
and Configured
are probably not needed in the names.
Comment From: wilkinsona
We're going to keep @ConditionalOnEnabledDevTools
as we feel that the enabled part if important to distinguish between DevTools just being on the classpath. We're going to rename @ConditionalOnOAuth2ClientRegistrationConfigured
to @ConditionalOnOAuth2ClientRegistrationProperties
to make it clear that it's properties that have to be set for the condition to match.
Comment From: gbaso
Reposting in this closed issue because according to
We'd actually like to go further and make sure that all *Condition classes are package-private and have an annotation.
I expected to find an @Conditional
for SAML2 properties (as per the original request of this issue) but I couldn't find it in the latest 3.5.x. Was it excluded for a particular reason or was it just an oversight?