Restrict user profile write acces to owner and super user

oVirt · Oct 27, 2022 · f759f54 · f759f54
1 parent 5374bab
commit f759f54
Show file tree

Hide file tree

Showing 11 changed files with 45 additions and 23 deletions.
diff --git a/...er/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommand.java b/...er/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommand.java
@@ -46,7 +46,8 @@ protected boolean validate() {
                 userProfileDao.getProfile(newProp.getUserId()),
                 newProp,
                 getCurrentUser(),
-                this::validate);
+                this::validate,
+                isSystemSuperUser());
     }
 
     public static String buildUserName(DbUserDao userDao, Guid userId) {

diff --git a/...es/bll/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertiesByUserIdQuery.java b/...es/bll/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertiesByUserIdQuery.java
@@ -20,7 +20,7 @@ public GetUserProfilePropertiesByUserIdQuery(P parameters, EngineContext engineC
     }
 
     private boolean validate() {
-        return validate(validator.authorized(getUser(), getParameters().getId()));
+        return validate(validator.authorized(getUser(), getParameters().getId(), getUser() != null && getUser().isAdmin()));
     }
 
     @Override

diff --git a/...l/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertyByNameAndUserIdQuery.java b/...l/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertyByNameAndUserIdQuery.java
@@ -18,7 +18,7 @@ public GetUserProfilePropertyByNameAndUserIdQuery(P parameters, EngineContext en
     }
 
     private boolean validate() {
-        return validate(validator.authorized(getUser(), getParameters().getId()));
+        return validate(validator.authorized(getUser(), getParameters().getId(), getUser() != null && getUser().isAdmin()));
     }
 
     @Override

diff --git a/...ager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertyQuery.java b/...ager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUserProfilePropertyQuery.java
@@ -19,7 +19,7 @@ public GetUserProfilePropertyQuery(P parameters, EngineContext engineContext) {
     }
 
     private boolean validate(UserProfileProperty prop) {
-        return validate(validator.authorized(getUser(), prop.getUserId()));
+        return validate(validator.authorized(getUser(), prop.getUserId(), getUser() != null && getUser().isAdmin()));
     }
 
     @Override

diff --git a/...modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommand.java b/...modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommand.java
@@ -29,7 +29,6 @@ public class RemoveUserProfilePropertyCommand<T extends IdParameters> extends Co
     @Inject
     protected DbUserDao userDao;
 
-
     private final UserProfileValidator validator = new UserProfileValidator();
 
     public RemoveUserProfilePropertyCommand(T parameters, CommandContext commandContext) {
@@ -64,8 +63,10 @@ protected boolean validate() {
                         .map(UserProfileProperty::getName)
                         .orElse(getParameters().getId().toString()));
         return validate(validator.propertyProvided(currentProp)) &&
-                validate(validator.authorized(getCurrentUser(),
-                        Optional.ofNullable(currentProp).map(UserProfileProperty::getUserId).orElse(null)));
+                validate(validator.authorized(
+                        getCurrentUser(),
+                        Optional.ofNullable(currentProp).map(UserProfileProperty::getUserId).orElse(null),
+                        isSystemSuperUser()));
     }
 
     @Override

diff --git a/...modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateUserProfilePropertyCommand.java b/...modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateUserProfilePropertyCommand.java
@@ -64,7 +64,8 @@ protected boolean validate() {
                 userProfileDao.get(propertyToUpdate.getPropertyId()),
                 getCurrentUser(),
                 this::validate,
-                propertyToUpdate
+                propertyToUpdate,
+                isSystemSuperUser()
         );
     }
 

diff --git a/...r/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/UserProfileValidator.java b/...r/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/UserProfileValidator.java
@@ -40,12 +40,13 @@ public ValidationResult propertyProvided(UserProfileProperty property) {
                 .when(property == null);
     }
 
-    public ValidationResult authorized(DbUser currentUser, Guid targetUserId) {
+    public ValidationResult authorized(DbUser currentUser, Guid targetUserId, boolean sufficientAdminRights) {
         return ValidationResult
                 .failWith(EngineMessage.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION)
                 .when(currentUser == null
                         || currentUser.getId() == null
-                        || !Objects.equals(currentUser.getId(), targetUserId) && !currentUser.isAdmin());
+                        || targetUserId == null
+                        || !Objects.equals(currentUser.getId(), targetUserId) && !sufficientAdminRights);
     }
 
     public ValidationResult validPublicSshKey(UserProfileProperty property) {
@@ -88,8 +89,9 @@ public ValidationResult firstPublicSshKey(UserProfile profile, UserProfileProper
     public boolean validateAdd(UserProfile existingProfile,
             UserProfileProperty newProp,
             DbUser currentUser,
-            Function<ValidationResult, Boolean> validate) {
-        return validate.apply(authorized(currentUser, newProp.getUserId())) &&
+            Function<ValidationResult, Boolean> validate,
+            boolean isSuperUser) {
+        return validate.apply(authorized(currentUser, newProp.getUserId(), isSuperUser)) &&
                 validate.apply(sameOwner(existingProfile.getUserId(), newProp.getUserId())) &&
                 validate.apply(validPublicSshKey(newProp)) &&
                 // /sshpublickeys endpoint is not aware of names so first check by SSH type
@@ -101,10 +103,11 @@ public boolean validateAdd(UserProfile existingProfile,
     public boolean validateUpdate(UserProfileProperty currentProp,
             DbUser currentUser,
             Function<ValidationResult, Boolean> validate,
-            UserProfileProperty update) {
+            UserProfileProperty update,
+            boolean isSuperUser) {
         return validate.apply(propertyProvided(currentProp)) &&
                 validate.apply(sameOwner(currentProp.getUserId(), update.getUserId())) &&
-                validate.apply(authorized(currentUser, update.getUserId())) &&
+                validate.apply(authorized(currentUser, update.getUserId(), isSuperUser)) &&
                 validate.apply(validPublicSshKey(update)) &&
                 validate.apply(sameName(currentProp.getName(), update.getName(), currentProp.getPropertyId())) &&
                 validate.apply(sameType(currentProp.getType(), update.getType(), currentProp.getPropertyId()));

diff --git a/...odules/bll/src/test/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommandTest.java b/...odules/bll/src/test/java/org/ovirt/engine/core/bll/AddUserProfilePropertyCommandTest.java
@@ -12,6 +12,8 @@
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.checkAssertsForSshProp;
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.createAdmin;
 
+import java.util.function.Predicate;
+
 import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
@@ -31,6 +33,9 @@ class AddUserProfilePropertyCommandTest extends BaseCommandTest {
     @Mock
     protected DbUserDao userDaoMock;
 
+    @Mock
+    private Predicate<DbUser> isSystemSuperUserPredicate;
+
     private UserProfilePropertyParameters parameters = mock(UserProfilePropertyParameters.class);
 
     @InjectMocks
@@ -54,6 +59,7 @@ void addSinglSshPropToEmptyProfileForAnotherUser() {
         when(userDaoMock.get(any())).thenReturn(mock(DbUser.class));
 
         when(parameters.getUserProfileProperty()).thenReturn(inputProp);
+        when(isSystemSuperUserPredicate.test(any())).thenReturn(true);
         addCommand.setCurrentUser(createAdmin(Guid.newGuid()));
 
         assertTrue(addCommand.validate(), buildValidationMessage(addCommand.getReturnValue()));

diff --git a/...les/bll/src/test/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommandTest.java b/...les/bll/src/test/java/org/ovirt/engine/core/bll/RemoveUserProfilePropertyCommandTest.java
@@ -11,12 +11,15 @@
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.buildValidationMessage;
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.createWithId;
 
+import java.util.function.Predicate;
+
 import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.ovirt.engine.core.bll.context.CommandContext;
 import org.ovirt.engine.core.common.action.IdParameters;
 import org.ovirt.engine.core.common.businessentities.UserProfileProperty;
+import org.ovirt.engine.core.common.businessentities.aaa.DbUser;
 import org.ovirt.engine.core.compat.Guid;
 import org.ovirt.engine.core.dao.DbUserDao;
 import org.ovirt.engine.core.dao.UserProfileDao;
@@ -28,6 +31,9 @@ class RemoveUserProfilePropertyCommandTest extends BaseCommandTest {
     @Mock
     private DbUserDao userDaoMock;
 
+    @Mock
+    private Predicate<DbUser> isSystemSuperUserPredicate;
+
     private IdParameters parameters = mock(IdParameters.class);
 
     @InjectMocks

diff --git a/...les/bll/src/test/java/org/ovirt/engine/core/bll/UpdateUserProfilePropertyCommandTest.java b/...les/bll/src/test/java/org/ovirt/engine/core/bll/UpdateUserProfilePropertyCommandTest.java
@@ -14,6 +14,8 @@
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.checkAssertsForSshProp;
 import static org.ovirt.engine.core.bll.UserProfileTestHelper.createWithId;
 
+import java.util.function.Predicate;
+
 import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
@@ -32,6 +34,9 @@ class UpdateUserProfilePropertyCommandTest extends BaseCommandTest {
     @Mock
     private DbUserDao userDaoMock;
 
+    @Mock
+    private Predicate<DbUser> isSystemSuperUserPredicate;
+
     private UserProfilePropertyParameters parameters = mock(UserProfilePropertyParameters.class);
 
     @InjectMocks

diff --git a/...dules/bll/src/test/java/org/ovirt/engine/core/bll/validator/UserProfileValidatorTest.java b/...dules/bll/src/test/java/org/ovirt/engine/core/bll/validator/UserProfileValidatorTest.java
@@ -1,6 +1,5 @@
 package org.ovirt.engine.core.bll.validator;
 
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
@@ -45,41 +44,41 @@ void noPropertyWithGivenName() {
 
     @Test
     void notAuthorizedDueToNullCurrentUser() {
-        assertFalse(validator.authorized(null, Guid.Empty).isValid());
+        assertFalse(validator.authorized(null, Guid.Empty, true).isValid());
     }
 
     @Test
     void notAuthorizedDueToNullCurrentUserId() {
-        assertFalse(validator.authorized(mock(DbUser.class), Guid.Empty).isValid());
+        assertFalse(validator.authorized(mock(DbUser.class), Guid.Empty, true).isValid());
     }
 
     @Test
     void notAuthorizedDueToNullTargetId() {
-        assertFalse(validator.authorized(emptyUser(), null).isValid());
+        assertFalse(validator.authorized(emptyUser(), null, true).isValid());
     }
 
     @Test
     void notAuthorizedDueToDifferentTargetId() {
-        assertFalse(validator.authorized(emptyUser(), Guid.newGuid()).isValid());
+        assertFalse(validator.authorized(emptyUser(), Guid.newGuid(), false).isValid());
     }
 
     @Test
     void authorized() {
-        assertTrue(validator.authorized(emptyUser(), Guid.Empty).isValid());
+        assertTrue(validator.authorized(emptyUser(), Guid.Empty, false).isValid());
     }
 
     @Test
     void authorizedAsAdminForOtherUser() {
         DbUser user = emptyUser();
         user.setAdmin(true);
-        assertTrue(validator.authorized(user, Guid.newGuid()).isValid());
+        assertTrue(validator.authorized(user, Guid.newGuid(), true).isValid());
     }
 
     @Test
     void authorizedAsAdminForHimself() {
         DbUser user = emptyUser();
         user.setAdmin(true);
-        assertTrue(validator.authorized(user, Guid.Empty).isValid());
+        assertTrue(validator.authorized(user, Guid.Empty, true).isValid());
     }
 
     @Test
@@ -131,7 +130,7 @@ void secondSshKey() {
     @Test
     void noSshKeyYet() {
         UserProfileProperty sshProp = UserProfileProperty.builder().withDefaultSshProp().build();
-        assertThat(validator.firstPublicSshKey(new UserProfile(), sshProp).isValid());
+        assertTrue(validator.firstPublicSshKey(new UserProfile(), sshProp).isValid());
     }
 
     @Test