Copilot commented on code in PR #13953:
URL: https://github.com/apache/cloudstack/pull/13953#discussion_r4322841388
##########
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().toString(), false);
+ }
+ user.setExternalEntity(entityId);
+ user.setSource(User.Source.SAML2);
+ }
+
+ private boolean disableSAMLForUser(UserVO user) {
+ if (!user.getSource().equals(User.Source.SAML2)) {
+ return false;
+ }
Review Comment:
`user.getSource().equals(...)` will throw a `NullPointerException` if the
user's source is null (your tests already indicate null sources are possible).
Use a null-safe comparison (e.g., `User.Source.SAML2.equals(user.getSource())`)
and treat null as "not currently SAML" (return false) to keep behavior
consistent and avoid NPEs.
##########
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().toString(), false);
+ }
Review Comment:
The value is persisted using `user.getSource().toString()` but later parsed
with `User.Source.valueOf(...)`, which expects the enum constant name. Persist
`user.getSource().name()` instead to guarantee round-tripping even if
`toString()` is overridden or changed in the future.
##########
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:
These helpers mutate global static `ConfigKey` internals and “reset” by
forcing `_value=null` and `_isDynamic=true`, which may not restore the previous
state if something else set it earlier in the same JVM. Capturing the original
`_value` and `_isDynamic` before mutation and restoring those exact values in
`tearDown()` will make the tests less order-dependent and reduce cross-test
interference.
##########
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:
Field naming is inconsistent with the surrounding injected DAOs (e.g.,
`_userDao`, `_domainMgr`). Renaming to match the established convention (e.g.,
`_userDetailsDao`) would improve consistency and reduce cognitive overhead when
scanning the class.
##########
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
testAuthorizeUserDoesNotRestorePreSamlSourceWhenAlreadyAuthorized() {
Review Comment:
This test calls `authorizeUser(..., enable=true)` and asserts that no detail
is added; it’s not restoring anything. Consider renaming to reflect what it
verifies (e.g., “DoesNotStorePreSamlSourceWhenAlreadyAuthorized/AlreadySAML”)
to avoid confusion for future readers.
--
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]
