[MGNLSSO-265] Possibility to implement custom SsoConfig Created: 18/Apr/23 Updated: 20/Oct/23 Resolved: 13/Jun/23 |
|
| Status: | Closed |
| Project: | Single Sign On |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | saas, 3.1.5 |
| Type: | Improvement | Priority: | Major |
| Reporter: | Björn Eschle | Assignee: | Evzen Fochr |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 2d 4.5h | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||||||
| Template: |
|
||||||||||||||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||||||||||||||
| Task DoD: |
[X]*
Doc/release notes changes? Comment present?
[X]*
Downstream builds green?
[X]*
Solution information and context easily available?
[X]*
Tests
[X]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||||||||||||||||||
| Release notes required: |
Yes
|
||||||||||||||||||||||||||||
| Date of First Response: | |||||||||||||||||||||||||||||
| Epic Link: | SSO maintenance | ||||||||||||||||||||||||||||
| Team: | |||||||||||||||||||||||||||||
| Work Started: | |||||||||||||||||||||||||||||
| Approved: |
Yes
|
||||||||||||||||||||||||||||
| Description |
|
The default ssoConfig is defined in the config yaml. This has 2 main issues:
The possibility to define a custom SsoConfig implementation would enable us to load the config from wherever we like (e.g. magnolia properties, jcr, ...) Currently this can only be achieved by a hack: import info.magnolia.config.source.yaml.YamlReader; import info.magnolia.map2bean.Map2BeanTransformer; import info.magnolia.module.ModuleRegistry; import info.magnolia.resourceloader.ResourceOrigin; import info.magnolia.sso.config.SsoConfig; import info.magnolia.sso.config.SsoConfigYamlBridge; import javax.inject.Inject; public class CustomSsoConfigBridge extends SsoConfigYamlBridge { private final CustomSsoConfig ssoConfig; @Inject public GardenaSsoConfigBridge( final ModuleRegistry moduleRegistry, final ResourceOrigin resourceOrigin, final YamlReader yamlReader, final Map2BeanTransformer map2Bean, final CustomSsoConfig ssoConfig) { super(moduleRegistry, resourceOrigin, yamlReader, map2Bean); this.ssoConfig = ssoConfig; } @Override public SsoConfig get() { return ssoConfig; } } import info.magnolia.sso.config.SsoConfig; public class CustomSsoConfig implements SsoConfig { ... } Binding in module.xml
<component>
<type>info.magnolia.sso.config.SsoConfigYamlBridge</type>
<implementation>...CustomSsoConfigBridge</implementation>
</component>
Binding the SsoConfig directly would be the cleaner solution and less dependent on the further development of this module (breaking change in case the Bridge ever gets removed/changed). |
| Comments |
| Comment by Mikaël Geljić [ 24/May/23 ] |
|
Hi beschle, On secrets in plain-text first:
The other part about extending SsoConfig: if I understand your use case correctly, there is a version check (if magnolia core = 6.2) that forces to use the YamlBridge, thus ignoring your CustomSsoConfig class. We could probably implement a "config selection logic" with fallbacks for this, but first, do you still need your custom config in light of the above? Hope that helps! |
| Comment by Björn Eschle [ 25/May/23 ] |
|
Hey @Mika, thanks for your feedback. You mean this feature: https://docs.magnolia-cms.com/product-docs/6.2/Administration/Architecture/Configuration-management.html#_environment_variables? Your docu (https://docs.magnolia-cms.com/magnolia-sso/3.1.3/guides/hiding-the-client-credentials-from-the-configuration-file.html) states:
However, even with env variables supported, I think allowing the binding of custom SsoConfigs will improve flexibility a lot and should not be a big effort/change on your side (I've attached a sample implementation - of gardena - so that you can get a glance on how we are currently using it). The issue with the bridge is not with magnolia versioning, but that we are (miss-)using it to bind our custom SsoConfig. This is working well right now, but if you ever decide to change/or remove the bridge, we'll have to adapt. One solution could be to inject a SsoConfigProvider instead of the SsoConfigYamlBridge in Pac4jConfigProvider and bind said provider to the DefaultSsoConfigProvider by default. This way we could simply change the binding to a custom SsoConfigProvider: import info.magnolia.cms.beans.config.ServerConfiguration; import info.magnolia.resourceloader.ResourceOrigin; import info.magnolia.sso.config.SsoConfig; import info.magnolia.sso.config.SsoConfigYamlBridge; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.inject.Inject; import java.util.HashSet; import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; import static info.magnolia.resourceloader.ResourceOriginChange.Type.ADDED; import static info.magnolia.resourceloader.ResourceOriginChange.Type.MODIFIED; class Pac4jConfigProvider { private final ServerConfiguration serverConfiguration; private final SsoConfigProvider ssoConfigProvider; @Inject public Pac4jConfigProvider( final ServerConfiguration serverConfiguration, final SsoConfigProvider ssoConfigProvider ) { this.serverConfiguration = serverConfiguration; this.ssoConfigProvider = ssoConfigProvider; ssoConfigProvider.registerSsoConfigChangedListener(ssoConfig -> loadPac4jConfig() ); } SsoConfig getSsoConfig() { return ssoConfigProvider.get(); } ... } interface SsoConfigProvider { SsoConfig get(); void registerSsoConfigChangedListener(Consumer<SsoConfig> listener); void unregisterSsoConfigChangedListener(Consumer<SsoConfig> listener); } class DefaultSsoConfigProvider implements SsoConfigProvider { private static final Logger log = LoggerFactory.getLogger(DefaultSsoConfigProvider.class); private final Set<Consumer<SsoConfig>> listeners = new HashSet<>(); private final Supplier<SsoConfig> ssoConfigSupplier; private SsoConfig ssoConfig; public DefaultSsoConfigProvider( final SsoConfig ssoMpConfig, final SsoConfigYamlBridge yamlBridge, final ResourceOrigin resourceOrigin ) { // if we're on 6.2, disregard the injected SSO MicroProfile config, and load if from yaml/m2b instead this.ssoConfigSupplier = yamlBridge.shouldLoadSsoConfigByYamlReader() ? yamlBridge : () -> ssoMpConfig; if (yamlBridge.shouldLoadSsoConfigByYamlReader()) { log.info("Monitoring SSO YAML configuration changes on {}", SsoConfigYamlBridge.SSO_YAML_CONFIG_PATH); resourceOrigin.registerResourceChangeHandler(change -> { String relatedResourcePath = change.getRelatedResourcePath(); boolean isConfigFileModifiedOrAdded = MODIFIED.equals(change.getType()) || ADDED.equals(change.getType()); if (isConfigFileModifiedOrAdded && relatedResourcePath.matches(SsoConfigYamlBridge.SSO_YAML_CONFIG_PATH)) { log.info("Receive {} change on file {}, reload SSO config and Pac4j configuration...", change.getType(), relatedResourcePath); ssoConfig = ssoConfigSupplier.get(); notifyListeners(); } }); } } @Override public SsoConfig get() { if (ssoConfig == null) { ssoConfig = ssoConfigSupplier.get(); } return ssoConfig; } private void notifyListeners() { listeners.forEach(listener -> listener.accept(get())); } @Override public void registerSsoConfigChangedListener(Consumer<SsoConfig> listener) { listeners.add(listener); } @Override public void unregisterSsoConfigChangedListener(Consumer<SsoConfig> listener) { listeners.remove(listener); } } |
| Comment by Mikaël Geljić [ 02/Jun/23 ] |
|
Yes, understood, We're considering a similar refactoring to ensure SsoConfig binding can be overridden without having to care about the YamlBridge (nor MicroProfile config moving forward). |
| Comment by Nguyen Phung Chi [ 12/Jun/23 ] |
|
Hey guys, FYI, the documentation has been updated: https://docs.magnolia-cms.com/magnolia-sso/3.1.4/guides/hiding-the-client-credentials-from-the-configuration-file.html Also the ticket is in final stage, will be released in SSO v3.1.5. |
| Comment by Björn Eschle [ 14/Jun/23 ] |
|
Thanks for the fast implementation! Works like a charm |