From d878a5e4b3425802da6653e1eddde24f69908d18 Mon Sep 17 00:00:00 2001
From: Martin Weise <martin.weise@tuwien.ac.at>
Date: Sun, 4 Dec 2022 11:20:18 +0100
Subject: [PATCH] Fixed the error message and forbidden keywords

---
 docker-compose.dev.yml                        | 25 ----------
 fda-query-service/Dockerfile                  |  1 +
 .../at/tuwien/endpoint/AbstractEndpoint.java  | 50 +++++++++++--------
 .../at/tuwien/endpoint/ExportEndpoint.java    |  5 +-
 .../at/tuwien/endpoint/QueryEndpoint.java     |  7 ++-
 .../at/tuwien/endpoint/StoreEndpoint.java     |  6 +--
 .../at/tuwien/endpoint/TableDataEndpoint.java |  5 +-
 .../tuwien/endpoint/TableHistoryEndpoint.java |  5 +-
 .../java/at/tuwien/endpoint/ViewEndpoint.java |  5 +-
 .../src/main/resources/application-docker.yml |  1 +
 .../src/main/resources/application-local.yml  |  1 +
 .../src/main/resources/application.yml        |  1 +
 .../src/main/resources/forbidden.txt          |  1 -
 .../java/at/tuwien/config/QueryConfig.java    | 14 ++++++
 fda-ui/components/query/Builder.vue           | 35 +++++++++++--
 fda-ui/components/query/Results.vue           |  4 +-
 16 files changed, 97 insertions(+), 69 deletions(-)
 delete mode 100644 fda-query-service/rest-service/src/main/resources/forbidden.txt
 create mode 100644 fda-query-service/services/src/main/java/at/tuwien/config/QueryConfig.java

diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml
index 20d39e1074..ae242539e8 100644
--- a/docker-compose.dev.yml
+++ b/docker-compose.dev.yml
@@ -54,8 +54,6 @@ services:
     image: dbrepo/discovery-service:latest
     networks:
       core:
-    ports:
-      - "9090:9090"
     env_file:
       - .env
     logging:
@@ -86,8 +84,6 @@ services:
     networks:
       userdb:
       core:
-    ports:
-      - "9092:9092"
     env_file:
       - .env
     volumes:
@@ -109,8 +105,6 @@ services:
     image: dbrepo/container-service:latest
     networks:
       core:
-    ports:
-      - "9091:9091"
     env_file:
       - .env
     volumes:
@@ -128,8 +122,6 @@ services:
     image: dbrepo/authentication-service:latest
     networks:
       core:
-    ports:
-      - "9097:9097"
     env_file:
       - .env
     depends_on:
@@ -150,8 +142,6 @@ services:
     networks:
       core:
       userdb:
-    ports:
-      - "9093:9093"
     env_file:
       - .env
     volumes:
@@ -172,8 +162,6 @@ services:
     networks:
       core:
       userdb:
-    ports:
-      - "9094:9094"
     env_file:
       - .env
     volumes:
@@ -196,8 +184,6 @@ services:
     image: dbrepo/identifier-service:latest
     networks:
       core:
-    ports:
-      - "9096:9096"
     env_file:
       - .env
     volumes:
@@ -219,8 +205,6 @@ services:
       core:
     env_file:
       - .env
-    ports:
-      - "9099:9099"
     depends_on:
       metadata-db:
         condition: service_started
@@ -236,8 +220,6 @@ services:
       core:
       userdb:
     command: sh -c "/wait && flask run" # docker-compose should not test the implementation
-    ports:
-      - "5000:5000"
     volumes:
       - ${SHARED_FILESYSTEM}:/tmp
       - /var/run/docker.sock:/var/run/docker.sock
@@ -254,8 +236,6 @@ services:
     image: dbrepo/units-service:latest
     networks:
       core:
-    ports:
-      - "5010:5010"
     volumes:
       - /tmp:/tmp
       - /var/run/docker.sock:/var/run/docker.sock
@@ -273,8 +253,6 @@ services:
     networks:
       core:
     ports:
-      - "5672:5672"
-      - "9098:9098"
       - "15672:15672"
     env_file:
       - .env
@@ -301,9 +279,6 @@ services:
     depends_on:
       discovery-service:
         condition: service_healthy
-    ports:
-      - 9200:9200
-      - 9600:9600
     volumes:
       - search-service-data:/usr/share/elasticsearch/data
     logging:
diff --git a/fda-query-service/Dockerfile b/fda-query-service/Dockerfile
index b2dac0fa18..8b071ee336 100644
--- a/fda-query-service/Dockerfile
+++ b/fda-query-service/Dockerfile
@@ -31,6 +31,7 @@ ENV GATEWAY_ENDPOINT=http://gateway-service:9095
 ENV SHARED_FILESYSTEM=/tmp
 ENV BROKER_CONSUMERS=2
 ENV LOG_LEVEL=debug
+ENV NOT_SUPPORTED_KEYWORDS=\\*,AVG,BIT_AND,BIT_OR,BIT_XOR,COUNT,COUNTDISTINCT,GROUP_CONCAT,JSON_ARRAYAGG,JSON_OBJECTAGG,MAX,MIN,STD,STDDEV,STDDEV_POP,STDDEV_SAMP,SUM,VARIANCE,VAR_POP,VAR_SAMP,--
 
 COPY ./service_ready /usr/bin
 RUN chmod +x /usr/bin/service_ready
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/AbstractEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/AbstractEndpoint.java
index af0232f142..674a1f9c4c 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/AbstractEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/AbstractEndpoint.java
@@ -2,21 +2,20 @@ package at.tuwien.endpoint;
 
 import at.tuwien.SortType;
 import at.tuwien.api.database.query.ExecuteStatementDto;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.entities.database.Database;
-import at.tuwien.entities.database.table.Table;
 import at.tuwien.entities.identifier.Identifier;
 import at.tuwien.exception.*;
 import at.tuwien.service.DatabaseService;
 import at.tuwien.service.IdentifierService;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.io.FileUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.security.core.Authentication;
 
-import java.io.File;
 import java.io.IOException;
-import java.nio.charset.Charset;
 import java.security.Principal;
+import java.util.Arrays;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -26,11 +25,14 @@ import static at.tuwien.entities.identifier.VisibilityType.EVERYONE;
 @Slf4j
 public abstract class AbstractEndpoint {
 
+    private final QueryConfig queryConfig;
     private final DatabaseService databaseService;
     private final IdentifierService identifierService;
 
     @Autowired
-    protected AbstractEndpoint(DatabaseService databaseService, IdentifierService identifierService) {
+    protected AbstractEndpoint(QueryConfig queryConfig, DatabaseService databaseService,
+                               IdentifierService identifierService) {
+        this.queryConfig = queryConfig;
         this.databaseService = databaseService;
         this.identifierService = identifierService;
     }
@@ -103,23 +105,27 @@ public abstract class AbstractEndpoint {
         }
     }
 
-    protected void validateForbiddenStatements(ExecuteStatementDto data) throws QueryMalformedException,
-            QueryStoreException {
-        final StringBuilder regex = new StringBuilder("[");
-        try {
-            FileUtils.readLines(new File("src/main/resources/forbidden.txt"), Charset.defaultCharset())
-                    .forEach(regex::append);
-        } catch (IOException e) {
-            log.error("Failed to load forbidden keywords list, reason {}", e.getMessage());
-            throw new QueryStoreException("Failed to load forbidden keywords list", e);
-        }
-        final Pattern pattern = Pattern.compile(regex + "]");
-        final Matcher matcher = pattern.matcher(data.getStatement());
-        final boolean found = matcher.find();
-        if (found) {
-            log.error("Query contains blacklisted character");
-            throw new QueryMalformedException("Query contains blacklisted character");
-        }
+    /**
+     * Do not allow aggregate functions and comments
+     * https://mariadb.com/kb/en/aggregate-functions/
+     */
+    protected void validateForbiddenStatements(ExecuteStatementDto data) throws QueryMalformedException {
+        final List<String> words = new LinkedList<>();
+        Arrays.stream(queryConfig.getNotSupportedKeywords())
+                .forEach(keyword -> {
+                    final Pattern pattern = Pattern.compile(keyword);
+                    final Matcher matcher = pattern.matcher(data.getStatement());
+                    final boolean found = matcher.find();
+                    if (found) {
+                        words.add(keyword);
+                    }
+                });
+        if (words.size() == 0) {
+            return;
+        }
+        log.error("Query contains forbidden keyword(s): {}", words);
+        log.debug("forbidden keywords: {}", words);
+        throw new QueryMalformedException("Query contains forbidden keyword(s): " + Arrays.toString(words.toArray()));
     }
 
     protected Boolean hasQueuePermission(Long containerId, Long databaseId, Long tableId, String permissionCode,
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ExportEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ExportEndpoint.java
index edac44b717..d7522723ee 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ExportEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ExportEndpoint.java
@@ -1,6 +1,7 @@
 package at.tuwien.endpoint;
 
 import at.tuwien.ExportResource;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.exception.*;
 import at.tuwien.service.DatabaseService;
 import at.tuwien.service.IdentifierService;
@@ -29,9 +30,9 @@ public class ExportEndpoint extends AbstractEndpoint {
     private final QueryService queryService;
 
     @Autowired
-    public ExportEndpoint(QueryService queryService, DatabaseService databaseService,
+    public ExportEndpoint(QueryConfig queryConfig, QueryService queryService, DatabaseService databaseService,
                           IdentifierService identifierService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.queryService = queryService;
     }
 
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/QueryEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/QueryEndpoint.java
index 3abdccb05c..b43e001949 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/QueryEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/QueryEndpoint.java
@@ -3,6 +3,7 @@ package at.tuwien.endpoint;
 import at.tuwien.ExportResource;
 import at.tuwien.SortType;
 import at.tuwien.api.database.query.*;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.querystore.Query;
 import at.tuwien.exception.*;
 import at.tuwien.service.*;
@@ -20,8 +21,6 @@ import org.springframework.web.bind.annotation.*;
 import javax.validation.Valid;
 import javax.validation.constraints.NotNull;
 import java.security.Principal;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 @Log4j2
 @RestController
@@ -32,9 +31,9 @@ public class QueryEndpoint extends AbstractEndpoint {
     private final StoreService storeService;
 
     @Autowired
-    public QueryEndpoint(QueryService queryService, StoreService storeService, DatabaseService databaseService,
+    public QueryEndpoint(QueryConfig queryConfig, QueryService queryService, StoreService storeService, DatabaseService databaseService,
                          IdentifierService identifierService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.queryService = queryService;
         this.storeService = storeService;
     }
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/StoreEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/StoreEndpoint.java
index 7582107b8d..1de0af7505 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/StoreEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/StoreEndpoint.java
@@ -2,7 +2,7 @@ package at.tuwien.endpoint;
 
 import at.tuwien.api.database.query.QueryBriefDto;
 import at.tuwien.api.database.query.QueryDto;
-import at.tuwien.api.database.query.QueryResultDto;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.entities.user.User;
 import at.tuwien.mapper.UserMapper;
 import at.tuwien.querystore.Query;
@@ -36,10 +36,10 @@ public class StoreEndpoint extends AbstractEndpoint {
     private final StoreService storeService;
 
     @Autowired
-    public StoreEndpoint(UserMapper userMapper, QueryMapper queryMapper,
+    public StoreEndpoint(QueryConfig queryConfig, UserMapper userMapper, QueryMapper queryMapper,
                          UserService userService, StoreService storeService, DatabaseService databaseService,
                          IdentifierService identifierService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.userMapper = userMapper;
         this.queryMapper = queryMapper;
         this.userService = userService;
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableDataEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableDataEndpoint.java
index 85120ecf90..e7215bac27 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableDataEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableDataEndpoint.java
@@ -6,6 +6,7 @@ import at.tuwien.api.database.query.QueryResultDto;
 import at.tuwien.api.database.table.TableCsvDeleteDto;
 import at.tuwien.api.database.table.TableCsvDto;
 import at.tuwien.api.database.table.TableCsvUpdateDto;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.exception.*;
 import at.tuwien.service.*;
 import io.micrometer.core.annotation.Timed;
@@ -32,9 +33,9 @@ public class TableDataEndpoint extends AbstractEndpoint {
     private final QueryService queryService;
 
     @Autowired
-    public TableDataEndpoint(QueryService queryService, DatabaseService databaseService,
+    public TableDataEndpoint(QueryConfig queryConfig, QueryService queryService, DatabaseService databaseService,
                              IdentifierService identifierService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.queryService = queryService;
     }
 
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableHistoryEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableHistoryEndpoint.java
index 2845209af4..73fa9fe847 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableHistoryEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/TableHistoryEndpoint.java
@@ -1,6 +1,7 @@
 package at.tuwien.endpoint;
 
 import at.tuwien.api.database.table.TableHistoryDto;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.exception.*;
 import at.tuwien.service.*;
 import io.micrometer.core.annotation.Timed;
@@ -25,9 +26,9 @@ public class TableHistoryEndpoint extends AbstractEndpoint {
     private final TableService tableService;
 
     @Autowired
-    public TableHistoryEndpoint(TableService tableService, DatabaseService databaseService,
+    public TableHistoryEndpoint(QueryConfig queryConfig, TableService tableService, DatabaseService databaseService,
                                 IdentifierService identifierService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.tableService = tableService;
     }
 
diff --git a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ViewEndpoint.java b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ViewEndpoint.java
index 70bbf4461a..69b86dbea8 100644
--- a/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ViewEndpoint.java
+++ b/fda-query-service/rest-service/src/main/java/at/tuwien/endpoint/ViewEndpoint.java
@@ -6,6 +6,7 @@ import at.tuwien.api.database.ViewDto;
 import at.tuwien.api.database.query.ExecuteStatementDto;
 import at.tuwien.api.database.query.QueryResultDto;
 import at.tuwien.api.database.query.QueryTypeDto;
+import at.tuwien.config.QueryConfig;
 import at.tuwien.entities.database.Database;
 import at.tuwien.entities.database.View;
 import at.tuwien.exception.*;
@@ -38,9 +39,9 @@ public class ViewEndpoint extends AbstractEndpoint {
     private final DatabaseService databaseService;
 
     @Autowired
-    public ViewEndpoint(ViewService viewService, DatabaseService databaseService, IdentifierService identifierService,
+    public ViewEndpoint(QueryConfig queryConfig, ViewService viewService, DatabaseService databaseService, IdentifierService identifierService,
                         ViewMapper viewMapper, QueryService queryService) {
-        super(databaseService, identifierService);
+        super(queryConfig, databaseService, identifierService);
         this.viewService = viewService;
         this.databaseService = databaseService;
         this.viewMapper = viewMapper;
diff --git a/fda-query-service/rest-service/src/main/resources/application-docker.yml b/fda-query-service/rest-service/src/main/resources/application-docker.yml
index 933074053e..de1b4101c1 100644
--- a/fda-query-service/rest-service/src/main/resources/application-docker.yml
+++ b/fda-query-service/rest-service/src/main/resources/application-docker.yml
@@ -39,3 +39,4 @@ fda:
   gateway.endpoint: "${GATEWAY_ENDPOINT}"
   ready.path: /ready
   consumers: 2
+  unsupported: "${NOT_SUPPORTED_KEYWORDS}"
diff --git a/fda-query-service/rest-service/src/main/resources/application-local.yml b/fda-query-service/rest-service/src/main/resources/application-local.yml
index 6708486f5e..d9fbe51756 100644
--- a/fda-query-service/rest-service/src/main/resources/application-local.yml
+++ b/fda-query-service/rest-service/src/main/resources/application-local.yml
@@ -39,3 +39,4 @@ fda:
   gateway.endpoint: http://localhost:9095
   ready.path: ./ready
   consumers: 2
+  unsupported: \\*,AVG,BIT_AND,BIT_OR,BIT_XOR,COUNT,COUNTDISTINCT,GROUP_CONCAT,JSON_ARRAYAGG,JSON_OBJECTAGG,MAX,MIN,STD,STDDEV,STDDEV_POP,STDDEV_SAMP,SUM,VARIANCE,VAR_POP,VAR_SAMP,--
diff --git a/fda-query-service/rest-service/src/main/resources/application.yml b/fda-query-service/rest-service/src/main/resources/application.yml
index 4139a27347..a309b0a6ba 100644
--- a/fda-query-service/rest-service/src/main/resources/application.yml
+++ b/fda-query-service/rest-service/src/main/resources/application.yml
@@ -39,3 +39,4 @@ fda:
   gateway.endpoint: "${GATEWAY_ENDPOINT}"
   ready.path: /ready
   consumers: "${BROKER_CONSUMERS}"
+  unsupported: "${NOT_SUPPORTED_KEYWORDS}"
diff --git a/fda-query-service/rest-service/src/main/resources/forbidden.txt b/fda-query-service/rest-service/src/main/resources/forbidden.txt
deleted file mode 100644
index 89bdcae710..0000000000
--- a/fda-query-service/rest-service/src/main/resources/forbidden.txt
+++ /dev/null
@@ -1 +0,0 @@
- \*
\ No newline at end of file
diff --git a/fda-query-service/services/src/main/java/at/tuwien/config/QueryConfig.java b/fda-query-service/services/src/main/java/at/tuwien/config/QueryConfig.java
new file mode 100644
index 0000000000..3b9be44361
--- /dev/null
+++ b/fda-query-service/services/src/main/java/at/tuwien/config/QueryConfig.java
@@ -0,0 +1,14 @@
+package at.tuwien.config;
+
+import lombok.Getter;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.context.annotation.Configuration;
+
+@Getter
+@Configuration
+public class QueryConfig {
+
+    @Value("${fda.unsupported}")
+    private String[] notSupportedKeywords;
+
+}
diff --git a/fda-ui/components/query/Builder.vue b/fda-ui/components/query/Builder.vue
index 81888b813f..aa4be84475 100644
--- a/fda-ui/components/query/Builder.vue
+++ b/fda-ui/components/query/Builder.vue
@@ -102,7 +102,11 @@
                   <v-alert
                     border="left"
                     color="info">
-                    Currently, comments in the query (e.g. <code>-- Comment</code>) are not supported!
+                    Currently, comments and <a href="https://mariadb.com/kb/en/aggregate-functions/" target="_blank">aggregation functions</a>
+                    <sup>
+                      <v-icon dense x-small>mdi-open-in-new</v-icon>
+                    </sup>
+                    are not supported!
                   </v-alert>
                 </v-col>
               </v-row>
@@ -146,9 +150,34 @@ export default {
       table: {},
       tables: [],
       views: [],
+      foundForbiddenKeywords: [],
+      forbiddenKeywords: [
+        '\\*',
+        'AVG',
+        'BIT_AND',
+        'BIT_OR',
+        'BIT_XOR',
+        'COUNT',
+        'COUNT', 'DISTINCT',
+        'GROUP_CONCAT',
+        'JSON_ARRAYAGG',
+        'JSON_OBJECTAGG',
+        'MAX',
+        'MIN',
+        'STD',
+        'STDDEV',
+        'STDDEV_POP',
+        'STDDEV_SAMP',
+        'SUM',
+        'VARIANCE',
+        'VAR_POP',
+        'VAR_SAMP',
+        '--'
+      ],
       tableDetails: null,
       resultId: null,
       valid: false,
+      errorKeyword: null,
       query: {
         sql: ''
       },
@@ -195,11 +224,9 @@ export default {
       if (this.tabs === 0) {
         return this.query.sql
       } else if (this.tabs === 1) {
-        const sql = this.rawSQL.replaceAll('\n', ' ') /* remove newline */
+        return this.rawSQL.replaceAll('\n', ' ') /* remove newline */
           .replaceAll(/\s+/g, ' ') /* remove whitespace */
           .trim()
-        console.debug('raw sql', sql)
-        return sql
       }
       return null
     },
diff --git a/fda-ui/components/query/Results.vue b/fda-ui/components/query/Results.vue
index 235e8a44b1..d5c13f8112 100644
--- a/fda-ui/components/query/Results.vue
+++ b/fda-ui/components/query/Results.vue
@@ -69,8 +69,8 @@ export default {
         this.loading = false
         parent.resultId = res.data.id
       } catch (err) {
-        console.error('failed to execute query', err)
-        this.$toast.error('Failed to execute query: ' + err.response.data.message)
+        console.error('Failed to execute query', err.response.data)
+        this.$toast.error(err.response.data.message)
         this.loading = false
       }
     },
-- 
GitLab