[GitHub] [kafka] SamBarker commented on a diff in pull request #12179: [KAFKA-13848] Clients remain connected after SASL re-authentication f…

[GitBox <gi...@apache.org>]

SamBarker commented on code in PR #12179:
URL: https://github.com/apache/kafka/pull/12179#discussion_r886228291


##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##########
@@ -679,10 +679,11 @@ private long calcCompletionTimesAndReturnSessionLifetimeMs() {
                 else if (connectionsMaxReauthMs == null)
                     retvalSessionLifetimeMs = zeroIfNegative(credentialExpirationMs - authenticationEndMs);
                 else
-                    retvalSessionLifetimeMs = zeroIfNegative(
-                            Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));
+                    retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));
 
-                sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs;
+                if (connectionsMaxReauthMs != null) {

Review Comment:
   >when reauth is disabled (when max reauth ms is NOT set), leave ReauthInfo#sessionExpirationTimeNanos as null but return millis until the token expires in SaslAuthenticateResponse's sessionLifetimeMs
   
   I think I have a different understanding. My mental model was that if either of `credentialExpirationMs != null || connectionsMaxReauthMs != null` was `true` we have re-auth enabled. The re-auth disabled case I was thinking of was when both were `false`. Now that I think about that I'm not sure that is a valid scenario.
   
   I think your original change to make setting `sessionExpirationTimeNanos` un-conditional was the right one, my `>=0` test is effectively the same as making it un-conditional. Therefore my suggestion of delayed initialisation becomes moot as we don't need to worry about excluding the default value from the test.
   I was wondering if the property change was what was actually causing the test failures that you found rather than making it un-conditional. 



##########
clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticatorTest.java:
##########
@@ -126,40 +135,94 @@ public void testUnexpectedRequestType() throws IOException {
     @Test
     public void testOldestApiVersionsRequest() throws IOException {
         testApiVersionsRequest(ApiKeys.API_VERSIONS.oldestVersion(),
-            ClientInformation.UNKNOWN_NAME_OR_VERSION, ClientInformation.UNKNOWN_NAME_OR_VERSION);
+                ClientInformation.UNKNOWN_NAME_OR_VERSION, ClientInformation.UNKNOWN_NAME_OR_VERSION);
     }
 
     @Test
     public void testLatestApiVersionsRequest() throws IOException {
         testApiVersionsRequest(ApiKeys.API_VERSIONS.latestVersion(),
-            "apache-kafka-java", AppInfoParser.getVersion());
+                "apache-kafka-java", AppInfoParser.getVersion());
     }
 
     @Test
-    public void testExpiredCredentialLifetime() throws IOException {
+    public void testSessionExpirationLeftAsNullButLifetimeReturnedToTheClientWhenReauthDisabled() throws IOException {
         String mechanism = OAuthBearerLoginModule.OAUTHBEARER_MECHANISM;
+        Duration tokenExpirationDuration = Duration.ofSeconds(100);
         SaslServer saslServer = mock(SaslServer.class);
-        when(saslServer.getMechanismName()).thenReturn(mechanism);
-        when(saslServer.evaluateResponse(any())).thenReturn(new byte[]{});
-        when(saslServer.getNegotiatedProperty(eq(SaslInternalConfigs.CREDENTIAL_LIFETIME_MS_SASL_NEGOTIATED_PROPERTY_KEY))).thenReturn(1L);
-        KafkaPrincipalBuilder kafkaPrincipalBuilder = mock(KafkaPrincipalBuilder.class);
-        when(kafkaPrincipalBuilder.build(any())).thenReturn(new KafkaPrincipal("[principal-type]", "[principal-name"));
+
+        MockTime time = new MockTime();

Review Comment:
   I'm wondering if its worth extracting a new test class `SaslServerAuthenticatorSessionExpiryTest` or similar and that would allow  some of the common setup code to actually be extracted to a `@BeforeEach` and thus make it easier to reason about what's happening in the tests.



##########
clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslServerAuthenticator.java:
##########
@@ -679,10 +679,11 @@ private long calcCompletionTimesAndReturnSessionLifetimeMs() {
                 else if (connectionsMaxReauthMs == null)
                     retvalSessionLifetimeMs = zeroIfNegative(credentialExpirationMs - authenticationEndMs);
                 else
-                    retvalSessionLifetimeMs = zeroIfNegative(
-                            Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));
+                    retvalSessionLifetimeMs = zeroIfNegative(Math.min(credentialExpirationMs - authenticationEndMs, connectionsMaxReauthMs));
 
-                sessionExpirationTimeNanos = authenticationEndNanos + 1000 * 1000 * retvalSessionLifetimeMs;
+                if (connectionsMaxReauthMs != null) {

Review Comment:
   @showuon what is your thinking on this?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org