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

[Broker] Only store authentication data after authentication is complete #12077

Conversation

michaeljmarshall
Copy link
Member

Motivation

The ServerCnx class stores the most recent AuthenticationDataSource for a connection in the authenticationData field. This should not be stored until the authentication state isComplete has returned true. Since the ServerCnx state does not go to Connected until after the auth state has completed, this is really just a cosmetic change.

For historical context, it looks like this line was added a while ago here: #5462. At that time, the doAuthentication method did not set the authenticationState field. The method now sets that field, so I think we should remove this line.

if (authState.isComplete()) {
// Authentication has completed. It was either:
// 1. the 1st time the authentication process was done, in which case we'll send
// a `CommandConnected` response
// 2. an authentication refresh, in which case we need to refresh authenticationData
String newAuthRole = authState.getAuthRole();
// Refresh the auth data.
this.authenticationData = authState.getAuthDataSource();

Modifications

  • Remove the premature setting of authentication data in ServerCnx

Verifying this change

This change is mostly cosmetic. The authenticationData field is only used when the class's state is Connected, and that only happens after the authentication state has returned true for isComplete. As such, I think tests passing will be enough to ensure correctness.

Does this pull request potentially affect one of the following parts:

This change is not breaking.

Documentation

No docs required.

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Sep 18, 2021
@lhotari lhotari added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label Nov 8, 2021
@lhotari lhotari added this to the 2.10.0 milestone Nov 8, 2021
@lhotari
Copy link
Member

lhotari commented Nov 8, 2021

Closing and re-opening to trigger a new fresh test run.

@lhotari lhotari closed this Nov 8, 2021
@lhotari lhotari reopened this Nov 8, 2021
@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

@michaeljmarshall michaeljmarshall force-pushed the only-set-auth-data-upon-completion branch from 056563e to 2d699c7 Compare November 24, 2021 05:44
@michaeljmarshall
Copy link
Member Author

Just rebased. The same line is the only change.

@codelipenghui codelipenghui merged commit fbec699 into apache:master Jan 18, 2022
@michaeljmarshall michaeljmarshall deleted the only-set-auth-data-upon-completion branch January 18, 2022 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants