refactor: sms gateway configuration stored in protected datastore ns …

…[DHIS2-18025] (#18560) * refactor: sms gateway configuration stored in protected datastore ns [DHIS2-18025] * chore: remove obsolete SMS config setting key * fix: tests and cleanup throws clauses * fix: sonar warnings --------- Co-authored-by: Morten Hansen <[email protected]>
dhis2 · Sep 10, 2024 · 675a8d3 · 675a8d3
1 parent 07af1d0
commit 675a8d3
Show file tree

Hide file tree

Showing 10 changed files with 157 additions and 87 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/datastore/DatastoreService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/datastore/DatastoreService.java
@@ -38,7 +38,9 @@
 import org.hisp.dhis.feedback.BadRequestException;
 import org.hisp.dhis.feedback.ConflictException;
 import org.hisp.dhis.feedback.ForbiddenException;
+import org.hisp.dhis.user.UserDetails;
 import org.springframework.security.access.AccessDeniedException;
+import org.springframework.transaction.annotation.Transactional;
 
 /**
  * Datastore is a key-value store with namespaces to isolate different collections of key-value
@@ -143,6 +145,9 @@ <T> T getEntries(DatastoreQuery query, Function<Stream<DatastoreFields>, T> tran
    */
   DatastoreEntry getEntry(String namespace, String key) throws ForbiddenException;
 
+  @Transactional(readOnly = true)
+  DatastoreEntry getEntry(String namespace, String key, UserDetails user) throws ForbiddenException;
+
   /**
    * Adds a new entry.
    *

diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sms/config/GatewayAdministrationService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sms/config/GatewayAdministrationService.java
@@ -28,16 +28,21 @@
 package org.hisp.dhis.sms.config;
 
 import javax.annotation.CheckForNull;
+import org.hisp.dhis.feedback.BadRequestException;
 import org.hisp.dhis.feedback.ConflictException;
+import org.hisp.dhis.feedback.ForbiddenException;
 import org.hisp.dhis.feedback.NotFoundException;
 
 /**
  * @author Zubair <[email protected]>
  */
 public interface GatewayAdministrationService {
-  void setDefaultGateway(SmsGatewayConfig config);
 
-  boolean removeGatewayByUid(String uid);
+  void setDefaultGateway(SmsGatewayConfig config)
+      throws ForbiddenException, ConflictException, BadRequestException;
+
+  boolean removeGatewayByUid(String uid)
+      throws ForbiddenException, ConflictException, BadRequestException;
 
   boolean hasGateways();
 
@@ -47,9 +52,10 @@ public interface GatewayAdministrationService {
 
   SmsGatewayConfig getByUid(String uid);
 
-  boolean addGateway(SmsGatewayConfig config);
+  boolean addGateway(SmsGatewayConfig config)
+      throws ForbiddenException, ConflictException, BadRequestException;
 
   void updateGateway(
       @CheckForNull SmsGatewayConfig persisted, @CheckForNull SmsGatewayConfig updatedConfig)
-      throws NotFoundException, ConflictException;
+      throws NotFoundException, ConflictException, ForbiddenException, BadRequestException;
 }
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sms/config/SmsConfigurationManager.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/sms/config/SmsConfigurationManager.java
@@ -27,9 +27,17 @@
  */
 package org.hisp.dhis.sms.config;
 
+import javax.annotation.Nonnull;
+import org.hisp.dhis.feedback.BadRequestException;
+import org.hisp.dhis.feedback.ConflictException;
+import org.hisp.dhis.feedback.ForbiddenException;
+
 /** Zubair <[email protected]> */
 public interface SmsConfigurationManager {
+
+  @Nonnull
   SmsConfiguration getSmsConfiguration();
 
-  void updateSmsConfiguration(SmsConfiguration config);
+  void updateSmsConfiguration(@Nonnull SmsConfiguration config)
+      throws ConflictException, ForbiddenException, BadRequestException;
 }
diff --git a/...ices/dhis-service-core/src/main/java/org/hisp/dhis/datastore/DefaultDatastoreService.java b/...ices/dhis-service-core/src/main/java/org/hisp/dhis/datastore/DefaultDatastoreService.java
@@ -30,6 +30,7 @@
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static java.util.Comparator.comparing;
+import static org.hisp.dhis.user.CurrentUserUtil.getCurrentUserDetails;
 
 import java.util.Date;
 import java.util.List;
@@ -49,7 +50,6 @@
 import org.hisp.dhis.feedback.ForbiddenException;
 import org.hisp.dhis.jsontree.JsonNode;
 import org.hisp.dhis.security.acl.AclService;
-import org.hisp.dhis.user.CurrentUserUtil;
 import org.hisp.dhis.user.UserDetails;
 import org.hisp.dhis.user.sharing.Sharing;
 import org.springframework.security.access.AccessDeniedException;
@@ -101,7 +101,8 @@ public void removeProtection(String namespace) {
   @Override
   @Transactional(readOnly = true)
   public List<String> getNamespaces() {
-    return store.getNamespaces().stream().filter(this::isNamespaceVisible).toList();
+    UserDetails user = getCurrentUserDetails();
+    return store.getNamespaces().stream().filter(ns -> isNamespaceVisible(user, ns)).toList();
   }
 
   @Override
@@ -138,6 +139,13 @@ public DatastoreEntry getEntry(String namespace, String key) throws ForbiddenExc
     return readProtectedIn(namespace, null, () -> store.getEntry(namespace, key));
   }
 
+  @Override
+  @Transactional(readOnly = true)
+  public DatastoreEntry getEntry(String namespace, String key, UserDetails user)
+      throws ForbiddenException {
+    return readProtectedIn(user, namespace, null, () -> store.getEntry(namespace, key));
+  }
+
   @Override
   @Transactional
   public void addEntry(DatastoreEntry entry)
@@ -195,6 +203,11 @@ public void deleteEntry(DatastoreEntry entry) {
     writeProtectedIn(entry.getNamespace(), () -> singletonList(entry), () -> store.delete(entry));
   }
 
+  private <T> T readProtectedIn(String namespace, T whenHidden, Supplier<T> read)
+      throws ForbiddenException {
+    return readProtectedIn(getCurrentUserDetails(), namespace, whenHidden, read);
+  }
+
   /**
    * There are 2 levels of access to be aware of in a Datastore: <br>
    *
@@ -216,33 +229,35 @@ public void deleteEntry(DatastoreEntry entry) {
    *     to {@link DatastoreEntry} or {@link org.hisp.dhis.user.User} has no {@link Sharing} access
    *     for restricted namespace {@link DatastoreEntry}
    */
-  private <T> T readProtectedIn(String namespace, T whenHidden, Supplier<T> read)
+  private <T> T readProtectedIn(UserDetails user, String namespace, T whenHidden, Supplier<T> read)
       throws ForbiddenException {
     DatastoreNamespaceProtection protection = protectionByNamespace.get(namespace);
-    if (userHasNamespaceReadAccess(protection)) {
+    if (userHasNamespaceReadAccess(user, protection)) {
       T res = read.get();
-      if (res instanceof DatastoreEntry de
-          && (!aclService.canRead(CurrentUserUtil.getCurrentUserDetails(), de))) {
+      if (res instanceof DatastoreEntry de && (!aclService.canRead(user, de))) {
         throw new ForbiddenException(
             String.format("Access denied for key '%s' in namespace '%s'", de.getKey(), namespace));
       }
       return res;
-    } else if (protection.getReads() == ProtectionType.RESTRICTED) {
+    }
+    if (protection.getReads() == ProtectionType.RESTRICTED) {
       throw accessDeniedTo(namespace);
     }
     return whenHidden;
   }
 
-  private boolean userHasNamespaceReadAccess(DatastoreNamespaceProtection protection) {
+  private boolean userHasNamespaceReadAccess(
+      UserDetails user, DatastoreNamespaceProtection protection) {
     return protection == null
         || protection.getReads() == ProtectionType.NONE
-        || currentUserHasAuthority(protection.getReadAuthorities());
+        || currentUserHasAuthority(user, protection.getReadAuthorities());
   }
 
-  private boolean userHasNamespaceWriteAccess(DatastoreNamespaceProtection protection) {
+  private boolean userHasNamespaceWriteAccess(
+      UserDetails user, DatastoreNamespaceProtection protection) {
     return protection == null
         || protection.getWrites() == ProtectionType.NONE
-        || currentUserHasAuthority(protection.getWriteAuthorities());
+        || currentUserHasAuthority(user, protection.getWriteAuthorities());
   }
 
   /**
@@ -267,10 +282,11 @@ private boolean userHasNamespaceWriteAccess(DatastoreNamespaceProtection protect
    */
   private void writeProtectedIn(
       String namespace, Supplier<List<DatastoreEntry>> entries, Runnable write) {
+    UserDetails user = getCurrentUserDetails();
     DatastoreNamespaceProtection protection = protectionByNamespace.get(namespace);
-    if (userHasNamespaceWriteAccess(protection)) {
+    if (userHasNamespaceWriteAccess(user, protection)) {
       for (DatastoreEntry entry : entries.get()) {
-        if (!aclService.canWrite(CurrentUserUtil.getCurrentUserDetails(), entry)) {
+        if (!aclService.canWrite(getCurrentUserDetails(), entry)) {
           throw accessDeniedTo(namespace, entry.getKey());
         }
       }
@@ -291,17 +307,15 @@ private AccessDeniedException accessDeniedTo(String namespace, String key) {
         String.format("Access denied for key '%s' in namespace '%s'", key, namespace));
   }
 
-  private boolean isNamespaceVisible(String namespace) {
+  private boolean isNamespaceVisible(UserDetails user, String namespace) {
     DatastoreNamespaceProtection protection = protectionByNamespace.get(namespace);
     return protection == null
         || protection.getReads() != ProtectionType.HIDDEN
-        || currentUserHasAuthority(protection.getReadAuthorities());
+        || currentUserHasAuthority(user, protection.getReadAuthorities());
   }
 
-  private boolean currentUserHasAuthority(Set<String> authorities) {
-    UserDetails currentUserDetails = CurrentUserUtil.getCurrentUserDetails();
-    return currentUserDetails.isSuper()
-        || !authorities.isEmpty() && currentUserDetails.hasAnyAuthority(authorities);
+  private boolean currentUserHasAuthority(UserDetails user, Set<String> authorities) {
+    return user.isSuper() || !authorities.isEmpty() && user.hasAnyAuthority(authorities);
   }
 
   private void validateEntry(DatastoreEntry entry) throws BadRequestException {

diff --git a/...vice-core/src/main/java/org/hisp/dhis/sms/config/DefaultGatewayAdministrationService.java b/...vice-core/src/main/java/org/hisp/dhis/sms/config/DefaultGatewayAdministrationService.java
@@ -41,7 +41,9 @@
 import org.hisp.dhis.common.CodeGenerator;
 import org.hisp.dhis.common.IndirectTransactional;
 import org.hisp.dhis.common.NonTransactional;
+import org.hisp.dhis.feedback.BadRequestException;
 import org.hisp.dhis.feedback.ConflictException;
+import org.hisp.dhis.feedback.ForbiddenException;
 import org.hisp.dhis.feedback.NotFoundException;
 import org.jasypt.encryption.pbe.PBEStringEncryptor;
 import org.springframework.beans.factory.annotation.Qualifier;
@@ -54,7 +56,7 @@
  */
 @Slf4j
 @RequiredArgsConstructor
-@Service("org.hisp.dhis.sms.config.GatewayAdministrationService")
+@Service
 public class DefaultGatewayAdministrationService implements GatewayAdministrationService {
 
   private final AtomicBoolean hasGateways = new AtomicBoolean();
@@ -64,18 +66,15 @@ public class DefaultGatewayAdministrationService implements GatewayAdministratio
   @Qualifier("tripleDesStringEncryptor")
   private final PBEStringEncryptor pbeStringEncryptor;
 
-  // -------------------------------------------------------------------------
-  // GatewayAdministrationService implementation
-  // -------------------------------------------------------------------------
-
   @EventListener
   public void handleContextRefresh(ContextRefreshedEvent event) {
     updateHasGatewaysState();
   }
 
   @Override
   @IndirectTransactional
-  public void setDefaultGateway(SmsGatewayConfig config) {
+  public void setDefaultGateway(SmsGatewayConfig config)
+      throws ForbiddenException, ConflictException, BadRequestException {
     SmsConfiguration configuration = getSmsConfiguration();
 
     configuration
@@ -88,7 +87,8 @@ public void setDefaultGateway(SmsGatewayConfig config) {
 
   @Override
   @IndirectTransactional
-  public boolean addGateway(SmsGatewayConfig config) {
+  public boolean addGateway(SmsGatewayConfig config)
+      throws ForbiddenException, ConflictException, BadRequestException {
     if (config == null) {
       return false;
     }
@@ -122,7 +122,7 @@ public boolean addGateway(SmsGatewayConfig config) {
   @IndirectTransactional
   public void updateGateway(
       @CheckForNull SmsGatewayConfig persisted, @CheckForNull SmsGatewayConfig updated)
-      throws NotFoundException, ConflictException {
+      throws NotFoundException, ConflictException, ForbiddenException, BadRequestException {
     if (updated == null) throw new ConflictException("Gateway configuration cannot be null");
     if (persisted == null) throw new NotFoundException(SmsGatewayConfig.class, updated.getUid());
     if (persisted.getClass() != updated.getClass())
@@ -180,7 +180,8 @@ public void updateGateway(
 
   @Override
   @IndirectTransactional
-  public boolean removeGatewayByUid(String uid) {
+  public boolean removeGatewayByUid(String uid)
+      throws ForbiddenException, ConflictException, BadRequestException {
     SmsConfiguration smsConfiguration = getSmsConfiguration();
 
     List<SmsGatewayConfig> gateways = smsConfiguration.getGateways();
@@ -228,40 +229,13 @@ public boolean hasGateways() {
     return hasGateways.get();
   }
 
-  // -------------------------------------------------------------------------
-  // Supportive methods
-  // -------------------------------------------------------------------------
-
   private SmsConfiguration getSmsConfiguration() {
-    SmsConfiguration smsConfiguration = smsConfigurationManager.getSmsConfiguration();
-
-    if (smsConfiguration != null) {
-      return smsConfiguration;
-    }
-
-    return new SmsConfiguration();
+    return smsConfigurationManager.getSmsConfiguration();
   }
 
   private void updateHasGatewaysState() {
-    SmsConfiguration smsConfiguration = smsConfigurationManager.getSmsConfiguration();
-
-    if (smsConfiguration == null) {
-      log.info("SMS configuration not found");
-      hasGateways.set(false);
-      return;
-    }
-
-    List<SmsGatewayConfig> gatewayList = smsConfiguration.getGateways();
-
-    if (gatewayList == null || gatewayList.isEmpty()) {
-      log.info("No Gateway configuration found");
-
-      hasGateways.set(false);
-      return;
-    }
-
-    log.info("Gateway configuration found: " + gatewayList);
-
-    hasGateways.set(true);
+    SmsConfiguration config = smsConfigurationManager.getSmsConfiguration();
+    List<SmsGatewayConfig> gateways = config.getGateways();
+    hasGateways.set(gateways != null && !gateways.isEmpty());
   }
 }