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

Add support for using an AuthTokenManager bean to authenticate with Neo4j #36650

Conversation

michael-simons
Copy link
Contributor

Neo4j Java driver introduced support for an AuthTokenManager that can be used to define expiring tokens to be used as authentication with a database.

This commit adds a ObjectProvider<AuthTokenManager> authTokenManagers parameter to the corresponding auto configuration class. If the provider resolves to a unique object, that AuthTokenManager will have precedence over any static token.

The usecase for Neo4j users is to be able to define expiring tokens and the like. Without this addition, they must configure the driver bean completely on their own. The config customiser will not help them as the token manager is configured in parallel. This leads to a lot of unnecessary duplicated code and some things are not even obvious that they won't work anymore (such as the integration with Spring logging).

The AuthTokenManager interface is marked as preview as it will probably receive more methods, it't won't go away however the code does not depend on specifics.

I tried adding the tests into unit scope, but the least bloated way is just running them as an integration test. To avoid having to start multiple containers, I used two nested tests in Neo4jAutoConfigurationIntegrationTests.

…bean.

Neo4j Java driver introduced support for an `AuthTokenManager` that can be used to define expiring tokens to be used as authentication with a database.

This commit adds a  `ObjectProvider<AuthTokenManager> authTokenManagers` parameter to the corresponding auto configuration class. If the provider resolves to a unique object, that `AuthTokenManager` will have precedence over any static token.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 31, 2023
@wilkinsona wilkinsona changed the title Add support for detecting the precense of a Neo4j AuthTokenManager bean. Add support for detecting the precense of a Neo4j AuthTokenManager bean Aug 2, 2023
@wilkinsona wilkinsona added this to the 3.2.x milestone Aug 2, 2023
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 2, 2023
@wilkinsona wilkinsona self-assigned this Aug 2, 2023
@wilkinsona
Copy link
Member

Thanks for the PR, @michael-simons. I was just about to merge it when I realised that we may have a problem with service connections and Neo4jConnectionDetails.

Connection details are intended to provide everything that's needed to connect to Neo4j. In a situation where there's a user-defined AuthTokenManager, it should be possible for the connection details to override that token manager. For example, this would allow the credentials from a Testcontainers-managed container to be used in favor of an AuthTokenManager that's for a production database. Does that make sense?

Assuming that it does for a moment, I'm not totally sure how to do it. One approach would be to add a getAuthManager() method to Neo4jConnectionDetails that returns null by default. Ignoring the naming for now, PropertiesNeo4jConnectionDetails could then return any user-defined AuthTokenManager from this new method. A Testcontainers service connection would then define its own Neo4jConnectionDetails causing the AuthTokenManager to be ignored.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Aug 2, 2023
@michael-simons
Copy link
Contributor Author

Oh yes, I get what you're saying and it makes a lot of sense to me.

So an updated version could look like this:

  • Don't use context beans to determine the existence of an AuthTokenManager as otherwise there is no indication from any connection details implementation how to override it
  • Add a default method to the Neo4jConnectionDetails returning an empty optional (or null, whatever you prefer) so that the authTokenMethod will be used
  • Keep the service connection as, so that it always uses the token

And then I don't know enough about the connection details I realise. Would a user provide a custom bean then? Which wouldn't be a problem for our users I think.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 2, 2023
wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Aug 2, 2023
Neo4j Java driver introduced support for an `AuthTokenManager` that can
be used to define expiring tokens for authentication with a database.

This commit adds an `ObjectProvider<AuthTokenManager> authTokenManagers`
parameter to the corresponding auto configuration class. If the provider
resolves to a unique object, that `AuthTokenManager` will have precedence
over any static token.

See spring-projectsgh-36650
wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request Aug 2, 2023
@wilkinsona
Copy link
Member

wilkinsona commented Aug 2, 2023

Thanks, Michael. I've pushed a branch that hopefully shows more clearly what I was trying to describe earlier: https://github.com/wilkinsona/spring-boot/tree/gh-36650.

If a user wants to use an AuthTokenManager they would declare one as a @Bean. This bean will be used unless there's a custom Neo4jConnectionDetails bean, typically coming from Testcontainers or Docker Compose., at which point the AutoTokenManager, if any, from the custom connection details will be used instead.

@michael-simons
Copy link
Contributor Author

Wow, thanks for your effort, understandable and suits the use case.

Only curious about the the reason using getIfAvailable over getIfUnique?

@wilkinsona
Copy link
Member

Only curious about the the reason using getIfAvailable over getIfUnique?

No good reason at all. Thanks for catching that. It should be getIfUnique() as you had used.

@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.2.0-M2 Aug 2, 2023
wilkinsona pushed a commit that referenced this pull request Aug 2, 2023
Neo4j Java driver introduced support for an `AuthTokenManager` that can
be used to define expiring tokens for authentication with a database.

This commit adds an `ObjectProvider<AuthTokenManager> authTokenManagers`
parameter to the corresponding auto configuration class. If the provider
resolves to a unique object, that `AuthTokenManager` will have precedence
over any static token.

See gh-36650
@wilkinsona wilkinsona closed this in 11753c5 Aug 2, 2023
@michael-simons
Copy link
Contributor Author

Thanks for the super fast turnaround, I know a bunch of people approaching this addition. 🙇

@wilkinsona
Copy link
Member

My pleasure. Thanks for the PR!

@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Aug 2, 2023
@wilkinsona wilkinsona changed the title Add support for detecting the precense of a Neo4j AuthTokenManager bean Add support for using an AuthTokenManager bean to authenticate with Neo4j Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants