Copilot commented on code in PR #13953:
URL: https://github.com/apache/cloudstack/pull/13953#discussion_r4334980491
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java:
##########
@@ -80,6 +97,39 @@ public void setUp() throws NoSuchFieldException,
IllegalAccessException {
Mockito.doReturn(true).when(saml2AuthManager).isSAMLPluginEnabled();
}
+ @After
+ public void tearDown() {
+ resetConfigKeyValue(SAML2AuthManager.EnableLoginAfterSAMLDisable);
+ }
+
+ private static <T> void setConfigKeyValue(ConfigKey<T> configKey, T value)
{
+ try {
+ Field valueField = ConfigKey.class.getDeclaredField("_value");
+ valueField.setAccessible(true);
+ valueField.set(configKey, value);
+
+ Field dynamicField =
ConfigKey.class.getDeclaredField("_isDynamic");
+ dynamicField.setAccessible(true);
+ dynamicField.setBoolean(configKey, false);
+ } catch (IllegalAccessException | NoSuchFieldException e) {
+ throw new RuntimeException("Failed to set ConfigKey value", e);
+ }
+ }
+
+ private static void resetConfigKeyValue(ConfigKey<?> configKey) {
+ try {
+ Field valueField = ConfigKey.class.getDeclaredField("_value");
+ valueField.setAccessible(true);
+ valueField.set(configKey, null);
+
+ Field dynamicField =
ConfigKey.class.getDeclaredField("_isDynamic");
+ dynamicField.setAccessible(true);
+ dynamicField.setBoolean(configKey, true);
+ } catch (IllegalAccessException | NoSuchFieldException e) {
+ throw new RuntimeException("Failed to reset ConfigKey value", e);
+ }
+ }
Review Comment:
The tests mutate `ConfigKey` internals (`_value`, `_isDynamic`) globally via
reflection, but the teardown resets to hardcoded defaults (`null` / `true`)
instead of restoring the prior values. This can leak state into other tests in
the same JVM and cause order-dependent failures. Prefer capturing the original
`_value` and `_isDynamic` for `EnableLoginAfterSAMLDisable` before overriding
(e.g., in `@Before`) and restoring those exact originals in `@After`;
alternatively use any existing test utility/framework mechanism for temporarily
overriding ConfigKeys to avoid reflective dependence on private fields.
##########
plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java:
##########
@@ -117,7 +167,142 @@ public void testAuthorizeUser() {
Mockito.verify(userDao,
Mockito.atLeastOnce()).update(Mockito.anyLong(), Mockito.any(user.getClass()));
}
+ @Test
+ public void testAuthorizeUserStoresPreSamlSourceOnEnable() {
+ UserVO user = new UserVO(200L);
+ user.setUsername("someuser");
+ user.setSource(User.Source.LDAP);
+ when(userDao.getUser(Mockito.anyLong())).thenReturn(user);
+
+ saml2AuthManager.authorizeUser(200L, "someID", true);
+
+ verify(userDetailsDao).addDetail(200L, "PreSamlSource", "LDAP", false);
+ assertEquals(User.Source.SAML2, user.getSource());
+ }
+
+ @Test
+ public void testAuthorizeUserDoesNotStorePreSamlSourceWhenAlreadySaml() {
Review Comment:
This hunk introduces CRLF/LF inconsistency (visible as stray `\r` characters
after `@Test` / method lines). Mixed line endings frequently trigger
formatting/checkstyle diffs and can be disruptive in reviews. Please normalize
the file’s line endings (ideally LF) and re-save the file so these carriage
returns are removed.
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java:
##########
@@ -446,26 +455,52 @@ public boolean isUserAuthorized(Long userId, String
entityId) {
@Override
public boolean authorizeUser(Long userId, String entityId, boolean enable)
{
UserVO user = _userDao.getUser(userId);
- if (user != null) {
- if (enable) {
- user.setExternalEntity(entityId);
- user.setSource(User.Source.SAML2);
- } else {
- boolean enableLoginAfterSAMLDisable =
SAML2AuthManager.EnableLoginAfterSAMLDisable.value();
- if (user.getSource().equals(User.Source.SAML2)) {
- if(enableLoginAfterSAMLDisable) {
- user.setSource(User.Source.UNKNOWN);
- } else {
- user.setSource(User.Source.SAML2DISABLED);
- }
- } else {
- return false;
- }
+ if (user == null) {
+ return false;
+ }
+ if (enable) {
+ enableSAMLForUser(user, entityId);
+ } else if (!disableSAMLForUser(user)) {
+ return false;
+ }
+ _userDao.update(user.getId(), user);
+ return true;
+ }
+
+ private void enableSAMLForUser(UserVO user, String entityId) {
+ if (user.getSource() != null &&
!User.Source.SAML2.equals(user.getSource()) &&
!User.Source.SAML2DISABLED.equals(user.getSource())) {
+ userDetailsDao.addDetail(user.getId(), PRE_SAML_SOURCE_DETAIL_KEY,
user.getSource().name(), false);
+ }
+ user.setExternalEntity(entityId);
+ user.setSource(User.Source.SAML2);
+ }
Review Comment:
`addDetail(...)` is called every time SAML is enabled for a user with a
non-SAML source, without checking for an existing `PreSamlSource` entry. If
`addDetail` inserts rather than upserts, repeated enable operations could
accumulate rows for the same key and complicate restores/debugging. Consider
making this write idempotent by updating an existing detail (or
deleting/replacing) when the key is already present.
##########
plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java:
##########
@@ -115,6 +121,9 @@ public String getSAMLIdentityProviderMetadataURL(){
@Inject
private UserDao _userDao;
+ @Inject
+ private UserDetailsDao userDetailsDao;
Review Comment:
The newly added field name `userDetailsDao` is inconsistent with the
surrounding member naming pattern (e.g., `_userDao`, `_domainMgr`). To keep the
class consistent and easier to scan, rename this field to match the existing
convention (for example `_userDetailsDao`) and update references accordingly.
--
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]
