Copilot commented on code in PR #14345:
URL: https://github.com/apache/cloudstack/pull/14345#discussion_r4347282874
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLTokenDaoImpl.java:
##########
@@ -16,18 +16,24 @@
// under the License.
package org.apache.cloudstack.saml;
+import com.cloud.utils.DateUtil;
import com.cloud.utils.db.DB;
import com.cloud.utils.db.GenericDaoBase;
import com.cloud.utils.db.TransactionLegacy;
import com.cloud.utils.exception.CloudRuntimeException;
import org.springframework.stereotype.Component;
import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.concurrent.TimeUnit;
@DB
@Component
public class SAMLTokenDaoImpl extends GenericDaoBase<SAMLTokenVO, Long>
implements SAMLTokenDao {
+ protected static final String EXPIRE_TOKENS_SQL = "DELETE FROM
`saml_token` WHERE `created` < ?";
+ protected static final long TOKEN_LIFETIME_MILLIS =
TimeUnit.HOURS.toMillis(1);
Review Comment:
These constants don’t appear to be intended as part of a subclass API, but
`protected` exposes them for inheritance/override. Consider reducing visibility
(e.g., `private static final` for the SQL, and package-private or `private
static final` for the lifetime) to avoid expanding the class’s extension
surface area unnecessarily.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/saml/SAMLTokenDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.saml;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.TimeZone;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SAMLTokenDaoImplTest {
+
+ @Mock
+ private TransactionLegacy transactionMock;
+
+ @Mock
+ private PreparedStatement preparedStatementMock;
+
+ private final SAMLTokenDaoImpl samlTokenDao = new SAMLTokenDaoImpl();
+
+ @Test
+ public void testExpireTokensUsesBoundGmtCutOff() throws Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ // a JVM/DB local zone ahead of UTC is where NOW() deleted fresh tokens
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Amsterdam"));
+ try (MockedStatic<TransactionLegacy> ignored =
Mockito.mockStatic(TransactionLegacy.class)) {
+
ignored.when(TransactionLegacy::currentTxn).thenReturn(transactionMock);
+
when(transactionMock.prepareAutoCloseStatement(anyString())).thenReturn(preparedStatementMock);
+
+ long before = System.currentTimeMillis();
+ samlTokenDao.expireTokens();
+ long after = System.currentTimeMillis();
+
+ ArgumentCaptor<String> sqlCaptor =
ArgumentCaptor.forClass(String.class);
+
verify(transactionMock).prepareAutoCloseStatement(sqlCaptor.capture());
+ assertFalse("cut-off must not depend on the DB session time zone",
+ sqlCaptor.getValue().toUpperCase().contains("NOW()"));
+
+ ArgumentCaptor<Timestamp> cutOffCaptor =
ArgumentCaptor.forClass(Timestamp.class);
+ ArgumentCaptor<Calendar> calendarCaptor =
ArgumentCaptor.forClass(Calendar.class);
+ verify(preparedStatementMock).setTimestamp(eq(1),
cutOffCaptor.capture(), calendarCaptor.capture());
+ verify(preparedStatementMock).executeUpdate();
+ verify(transactionMock).commit();
+
+ long cutOff = cutOffCaptor.getValue().getTime();
+ assertTrue(cutOff >= before -
SAMLTokenDaoImpl.TOKEN_LIFETIME_MILLIS);
+ assertTrue(cutOff <= after -
SAMLTokenDaoImpl.TOKEN_LIFETIME_MILLIS);
+ assertEquals(0,
calendarCaptor.getValue().getTimeZone().getRawOffset());
+ } finally {
+ TimeZone.setDefault(defaultTimeZone);
+ }
Review Comment:
This test mutates the JVM global default timezone, which can create race
conditions and flakiness if the test suite executes tests concurrently.
Consider guarding the timezone change with a global lock (e.g., synchronizing
on a shared lock object) or using test framework facilities/configuration to
ensure this test cannot run in parallel with others that may depend on the
default timezone.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/saml/SAMLTokenDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.saml;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.TimeZone;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SAMLTokenDaoImplTest {
+
+ @Mock
+ private TransactionLegacy transactionMock;
+
+ @Mock
+ private PreparedStatement preparedStatementMock;
+
+ private final SAMLTokenDaoImpl samlTokenDao = new SAMLTokenDaoImpl();
+
+ @Test
+ public void testExpireTokensUsesBoundGmtCutOff() throws Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ // a JVM/DB local zone ahead of UTC is where NOW() deleted fresh tokens
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Amsterdam"));
+ try (MockedStatic<TransactionLegacy> ignored =
Mockito.mockStatic(TransactionLegacy.class)) {
+
ignored.when(TransactionLegacy::currentTxn).thenReturn(transactionMock);
+
when(transactionMock.prepareAutoCloseStatement(anyString())).thenReturn(preparedStatementMock);
+
+ long before = System.currentTimeMillis();
+ samlTokenDao.expireTokens();
+ long after = System.currentTimeMillis();
Review Comment:
The time-window assertions can be flaky on slow/loaded CI hosts (GC pauses,
scheduler delays), because the tolerated window is only the execution time of
`expireTokens()`. Consider adding a small tolerance (e.g., a few seconds)
around the expected cutoff, or asserting against a single sampled `now` value
with an allowed delta to make the test robust under variable runtime conditions.
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLTokenDaoImpl.java:
##########
@@ -37,8 +43,9 @@ public void expireTokens() {
TransactionLegacy txn = TransactionLegacy.currentTxn();
try {
txn.start();
- String sql = "DELETE FROM `saml_token` WHERE `created` < (NOW() -
INTERVAL 1 HOUR)";
- PreparedStatement pstmt = txn.prepareAutoCloseStatement(sql);
+ Timestamp cutOff = new
Timestamp(DateUtil.currentGMTTime().getTime() - TOKEN_LIFETIME_MILLIS);
Review Comment:
Using `DateUtil.currentGMTTime().getTime()` is functionally equivalent to
`System.currentTimeMillis()` for epoch millis. Since the goal is
timezone-independent cutoff calculation, consider using
`System.currentTimeMillis()` (or `Instant.now()`) to remove the extra
dependency and potential confusion about timezone affecting epoch time.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/saml/SAMLTokenDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.saml;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.TimeZone;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SAMLTokenDaoImplTest {
+
+ @Mock
+ private TransactionLegacy transactionMock;
+
+ @Mock
+ private PreparedStatement preparedStatementMock;
+
+ private final SAMLTokenDaoImpl samlTokenDao = new SAMLTokenDaoImpl();
+
+ @Test
+ public void testExpireTokensUsesBoundGmtCutOff() throws Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ // a JVM/DB local zone ahead of UTC is where NOW() deleted fresh tokens
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Amsterdam"));
+ try (MockedStatic<TransactionLegacy> ignored =
Mockito.mockStatic(TransactionLegacy.class)) {
+
ignored.when(TransactionLegacy::currentTxn).thenReturn(transactionMock);
+
when(transactionMock.prepareAutoCloseStatement(anyString())).thenReturn(preparedStatementMock);
+
+ long before = System.currentTimeMillis();
+ samlTokenDao.expireTokens();
+ long after = System.currentTimeMillis();
+
+ ArgumentCaptor<String> sqlCaptor =
ArgumentCaptor.forClass(String.class);
+
verify(transactionMock).prepareAutoCloseStatement(sqlCaptor.capture());
+ assertFalse("cut-off must not depend on the DB session time zone",
+ sqlCaptor.getValue().toUpperCase().contains("NOW()"));
+
+ ArgumentCaptor<Timestamp> cutOffCaptor =
ArgumentCaptor.forClass(Timestamp.class);
+ ArgumentCaptor<Calendar> calendarCaptor =
ArgumentCaptor.forClass(Calendar.class);
+ verify(preparedStatementMock).setTimestamp(eq(1),
cutOffCaptor.capture(), calendarCaptor.capture());
+ verify(preparedStatementMock).executeUpdate();
+ verify(transactionMock).commit();
+
+ long cutOff = cutOffCaptor.getValue().getTime();
+ assertTrue(cutOff >= before -
SAMLTokenDaoImpl.TOKEN_LIFETIME_MILLIS);
+ assertTrue(cutOff <= after -
SAMLTokenDaoImpl.TOKEN_LIFETIME_MILLIS);
Review Comment:
The time-window assertions can be flaky on slow/loaded CI hosts (GC pauses,
scheduler delays), because the tolerated window is only the execution time of
`expireTokens()`. Consider adding a small tolerance (e.g., a few seconds)
around the expected cutoff, or asserting against a single sampled `now` value
with an allowed delta to make the test robust under variable runtime conditions.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/saml/SAMLTokenDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.saml;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.TimeZone;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SAMLTokenDaoImplTest {
+
+ @Mock
+ private TransactionLegacy transactionMock;
+
+ @Mock
+ private PreparedStatement preparedStatementMock;
+
+ private final SAMLTokenDaoImpl samlTokenDao = new SAMLTokenDaoImpl();
+
+ @Test
+ public void testExpireTokensUsesBoundGmtCutOff() throws Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ // a JVM/DB local zone ahead of UTC is where NOW() deleted fresh tokens
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Amsterdam"));
+ try (MockedStatic<TransactionLegacy> ignored =
Mockito.mockStatic(TransactionLegacy.class)) {
+
ignored.when(TransactionLegacy::currentTxn).thenReturn(transactionMock);
Review Comment:
The variable name `ignored` is misleading since it’s actively used to stub
`TransactionLegacy.currentTxn()`. Consider renaming it to something descriptive
like `transactionLegacyStaticMock` (or similar) to match its purpose and
improve readability.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/saml/SAMLTokenDaoImplTest.java:
##########
@@ -0,0 +1,85 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.saml;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.sql.PreparedStatement;
+import java.sql.Timestamp;
+import java.util.Calendar;
+import java.util.TimeZone;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.utils.db.TransactionLegacy;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SAMLTokenDaoImplTest {
+
+ @Mock
+ private TransactionLegacy transactionMock;
+
+ @Mock
+ private PreparedStatement preparedStatementMock;
+
+ private final SAMLTokenDaoImpl samlTokenDao = new SAMLTokenDaoImpl();
+
+ @Test
+ public void testExpireTokensUsesBoundGmtCutOff() throws Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ // a JVM/DB local zone ahead of UTC is where NOW() deleted fresh tokens
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Amsterdam"));
Review Comment:
This test mutates the JVM global default timezone, which can create race
conditions and flakiness if the test suite executes tests concurrently.
Consider guarding the timezone change with a global lock (e.g., synchronizing
on a shared lock object) or using test framework facilities/configuration to
ensure this test cannot run in parallel with others that may depend on the
default timezone.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
