ADR 019 password rotation #725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndyHeap-NeoTech
approved these changes
Aug 15, 2023
| // The token has changed or the connection needs to re-authenticate but can use the same credentials. | ||
| if (AuthorizationStatus == AuthorizationStatus.AuthorizationExpired || !token.Equals(AuthToken)) | ||
| var authExpired = AuthorizationStatus == AuthorizationStatus.AuthorizationExpired; | ||
| if (authExpired || !token.Equals(AuthToken)) |
There was a problem hiding this comment.
Is there a situation here where the code would need to handle a null token being supplied from the AuthTokenManager. Previous code threw an exception in that case (see line 354 for conditional on null check).
There was a problem hiding this comment.
Added a null check.
Replace Task with ValueTask for auth manager APIs.
| } | ||
|
|
||
| if (exception is AuthorizationException) | ||
| { | ||
| // if an auth exception occured the pool member, all connections in the pool that are using that token | ||
| // should be closed. This is because the token is now invalid and all connections using it will fail. | ||
| foreach (var conn in _inUseConnections) |
There was a problem hiding this comment.
do we care about setting it for idle connections, _inUseConnections are connections that another session or transaction currently has borrowed.
There was a problem hiding this comment.
I think it's right to let those connections fail the next time they're attempted to be used?
There was a problem hiding this comment.
why do we want idle connections to lazily discover the auth token is expired but set in use connections as expired?
There was a problem hiding this comment.
Yeah, thinking about it, you're right, let me alter that.
thelonelyvulpes
approved these changes
Aug 22, 2023
robsdedude
reviewed
Aug 22, 2023
RichardIrons-neo4j
added a commit
to RichardIrons-neo4j/neo4j-dotnet-driver
that referenced
this pull request
Sep 25, 2023
commit 7b8ef91 Author: Richard Irons <[email protected]> Date: Thu Aug 31 11:58:24 2023 +0100 Version bump (neo4j#727) commit 6f36b08 Author: Richard Irons <[email protected]> Date: Tue Aug 22 12:31:10 2023 +0100 ADR 019 password rotation (neo4j#725) This PR updates the preview feature "re-auth" significantly. The changes allow for catering to a wider range of use cases including simple password rotation. As part of this PR, all auth-related namespaces have been moved to preview - previously some did not have this, although the classes therein would not have been usable. Since this is a preview feature all changes here are breaking changes. The OnTokenExpiredAsync method in the IAuthTokenManager interface was removed, and a new HandleSecurityExceptionAsync method was added in its place. The ExpirationBased method in AuthTokenManagers was renamed to Bearer, and a new Basic method was added to cater for password rotation. --------- Co-authored-by: grant lodge <[email protected]> commit 7c93291 Author: grant lodge <[email protected]> Date: Fri Aug 18 15:49:18 2023 +0100 Implement pipelined begin for executable query (neo4j#724) * Implement pipelined begin * Add unit tests commit d024d70 Author: grant lodge <[email protected]> Date: Wed Jul 26 14:53:47 2023 +0100 [5.11] Zoned date time fixes (neo4j#715) Fixes for ZonedDateTime. * Introduce Ambiguous bool property, Creating ZonedDateTime with out a datetime kind, or using a datetime kind local with a named Zone:Zone.of(string), meant we could create an ambiguous date time. * Introduce ZonedDateTime.AmbiguityReason enum flags type. * Introduce Reason AmbiguityReason property for ZonedDateTimes, to allow users to see why a value is considered ambiguous. * Introduce UtcSeconds long property which is the understood monotonic timestamp from unix epoch in UTC. * Fix ZonedDateTime lazily parsing when returned from a query, now it is parsed immediately. * Fix ZonedDateTime not supporting values outside of range of BCL Date Types * Add EpochTicks Property, and constructor which can be used for easy interop with nodatime's Instant
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the preview feature "re-auth" significantly. The changes allow for catering to a wider range of use cases including simple password rotation.
As part of this PR, all auth-related namespaces have been moved to preview - previously some did not have this, although the classes therein would not have been usable.
Since this is a preview feature all changes here are breaking changes.
The
OnTokenExpiredAsyncmethod in theIAuthTokenManagerinterface was removed, and a newHandleSecurityExceptionAsyncmethod was added in its place.The
ExpirationBasedmethod inAuthTokenManagerswas renamed toBearer, and a newBasicmethod was added to cater for password rotation.