Skip to content

Conversation

@georgweiss
Copy link
Collaborator

Docs updated to describe the feature.

…henticated. Also some duplicate code removed
@georgweiss georgweiss requested a review from shroffk November 14, 2023 13:06
@shroffk
Copy link
Member

shroffk commented Nov 14, 2023

There are a few things here ( mainly spring things )

  • once we merge this Phoebus will need jdk 11.0.5 and newer ( we might have to fix documentation.. since up until now 11.0.2 was supported )
  • I am trying to disable the embedded server. currently if you include the follwoing dependency then the embedded ldap server is started on startup of the save restore services irrespective of weather it is used or now
		<dependency>
			<groupId>com.unboundid</groupId>
			<artifactId>unboundid-ldapsdk</artifactId>
		</dependency>

while this is not a new issue... if we have two spring services each with their own embedded server then one of the will fail to startup due to port conflicts. It is not ideal that the service should not startup cause of an unnecessary services having port conflicts.

@georgweiss
Copy link
Collaborator Author

Concluded the same: only way to suppress start of embedded server is to remove dependency...

@georgweiss
Copy link
Collaborator Author

georgweiss commented Nov 15, 2023

@shroffk, I have spent some time to figure if and how to suppress the embedded ldap server using other strategies besides removing the "unbound" dependency. Here is one (the only?) way to do it without removing the "unboundid" dependency:

Comment out/skip application property spring.ldap.embedded.base-dn. That suppresses the auto configuration of embedded ldap. External ldap auto config will then be enabled (do not know why). To also disable external ldap auto config, add to the command line:
-Dspring.autoconfigure.exclude=org.springframework.boot.autoconfigure.ldap.LdapAutoConfiguration

@shroffk
Copy link
Member

shroffk commented Nov 15, 2023

Ok,
So we have 2 solutions:

  1. above solution skipping the property and configuring spring to disable the autoconfiguring
  2. make the maven dependency optional and then inlcude/exclude it in your service product

I don't think either of these are great, we should just hope that future releases of spring make it easier to control the lifecycle of services/beans/autoconfigured pieces

@georgweiss
Copy link
Collaborator Author

georgweiss commented Nov 15, 2023

In my view 1 is preferable, though indeed not great. I'm wondering if we should comment out all spring.ldap.embedded.* properties to at least cover one case by default.

Maybe our use case is in fact "special". We provide a service where users can choose from a selection of mutually quite different authentication implementations. Again, we could consider creating WebSecurityConfig classes for each of these use cases, that would make it possible to more easily suppress what is not wanted.

@shroffk
Copy link
Member

shroffk commented Nov 15, 2023

I think for the time being we are fine... we have this "unique" situation with the security modules somewhat understood and documented here in PR's and Issues

Again, we could consider creating WebSecurityConfig classes for each of these use cases, that would make it possible to more easily suppress what is not wanted.

So instead of configurable beans which inject the authentication manager... we have the multiple WebSecurityConfig as a configurable bean? I think this would work for suppressing the external ldap bindings... but the embedded one would still need one of the 2 solutions above ( solution 1 being the preferred one )

Maybe we pause here and keep things as they are... I was reading that spring security 7 is going to have a lot of changes ( including removing some of the deprecated classes which started this fun journey ), so we can come back to the remaining cleanup then.... as of now things are broadly working as we need them to.

@georgweiss
Copy link
Collaborator Author

georgweiss commented Nov 15, 2023

Agree, let's stop now.
There is one thing that bothers me a bit... In the client things will not work unless credentials caching is enabled. After all, we do not have any credentials input fields in save&restore.
To me the easiest solution is to just make caching mandatory, using in-memory as default.

@shroffk
Copy link
Member

shroffk commented Nov 15, 2023

yes absolutely... even I was stumped for a bit and thought my Phoebus was not correctly configured.

@georgweiss
Copy link
Collaborator Author

georgweiss commented Nov 15, 2023

@shroffk, turns out my concerns about credentials caching were unfounded. Caching is kind of implied when logging in through the credentials manager application. Olog client is different as it offers credentials input, and there you can specify a caching policy.

@shroffk
Copy link
Member

shroffk commented Nov 28, 2023

Ready to merge but will wait till after the current release of Phoebus

@georgweiss
Copy link
Collaborator Author

@shroffk, that's fine, but the release is currently blocked by the VTable issue. Not sure when and how this can be resolved.

@georgweiss
Copy link
Collaborator Author

@shroffk, can we merge this?

@shroffk shroffk merged commit f6948e5 into master Jan 11, 2024
@georgweiss georgweiss deleted the CSSTUDIO-1684 branch January 11, 2024 16:13
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.

3 participants