Copilot commented on code in PR #13953:
URL: https://github.com/apache/cloudstack/pull/13953#discussion_r4346391443
##########
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);
+ }
+
+ private boolean disableSAMLForUser(UserVO user) {
+ if (!User.Source.SAML2.equals(user.getSource())) {
+ return false;
+ }
+ if (SAML2AuthManager.EnableLoginAfterSAMLDisable.value()) {
+ user.setSource(getPreSamlSource(user.getId()));
+ } else {
+ user.setSource(User.Source.SAML2DISABLED);
+ }
+ return true;
+ }
+
+ /**
+ * The Source (e.g. LDAP) the user had before SAML was authorized for
them, so disabling
+ * SAML can restore it instead of always falling back to {@link
User.Source#UNKNOWN}.
+ */
+ private User.Source getPreSamlSource(long userId) {
+ UserDetailVO preSamlSource = userDetailsDao.findDetail(userId,
PRE_SAML_SOURCE_DETAIL_KEY);
+ if (preSamlSource != null) {
+ try {
+ return User.Source.valueOf(preSamlSource.getValue());
+ } catch (IllegalArgumentException e) {
+ logger.warn("Unrecognized pre-SAML source '{}' stored for user
{}; falling back to UNKNOWN", preSamlSource.getValue(), userId);
}
- _userDao.update(user.getId(), user);
- return true;
}
- return false;
+ return User.Source.UNKNOWN;
}
Review Comment:
When `EnableLoginAfterSAMLDisable` is true, disabling SAML can restore
whatever enum value is stored. If the stored value is `SAML2` or
`SAML2DISABLED` (e.g., stale/corrupted data or manual DB edits),
`disableSAMLForUser` may leave the user effectively still on SAML,
contradicting the intent of “disable SAML”. Consider validating the parsed
source and treating `SAML2`/`SAML2DISABLED` as invalid (fallback to `UNKNOWN`,
or to `SAML2DISABLED` depending on the desired semantics).
##########
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:
This injected field name doesn’t match the existing member naming convention
in this class (e.g., `_userDao`, `_domainMgr`). To keep consistency and
readability, consider renaming to `_userDetailsDao` (and updating references)
or aligning all DI members to a common convention.
##########
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 tests mutate `ConfigKey` internals via reflection (`_value`,
`_isDynamic`) and restore state by assuming `_isDynamic=true` on reset. This is
brittle (private field names/semantics can change) and can leak global state
across tests if the original `_isDynamic` wasn’t `true`. Prefer a supported way
to override config for tests (e.g., via configuration/dao test utilities if
available), or at least capture and restore the original field values (both
`_value` and `_isDynamic`) per key in the helper to avoid cross-test coupling.
--
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]
