necouchman commented on code in PR #809: URL: https://github.com/apache/guacamole-client/pull/809#discussion_r1139288588
########## guacamole/src/main/frontend/src/app/import/controllers/importConnectionsController.js: ########## @@ -0,0 +1,760 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* global _ */ Review Comment: Something new, different, extra? ########## guacamole/src/main/frontend/src/app/import/directives/connectionImportErrors.js: ########## @@ -0,0 +1,234 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/* global _ */ Review Comment: ? ########## guacamole/src/main/java/org/apache/guacamole/rest/jsonpatch/APIPatchOutcome.java: ########## @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.rest.jsonpatch; + +import org.apache.guacamole.rest.jsonpatch.APIPatch.Operation; + +/** + * A successful outcome associated with a particular patch within a JSON Patch + * request. The outcome contains the operation requested by the original patch, + * the path from the original patch, and the identifier of the object corresponding + * to the value from the original patch. + * + * The purpose of this class is to present a relatively lightweight outcome for + * the user who submitted the Patch request. Rather than including the full + * contents of the value, only the identifier is included, allowing the user to + * determine the identifier of any newly-created objects as part of the request. + * + */ +public class APIPatchOutcome { + + /** + * The requested operation for the patch corresponding to this outcome. + */ + private final Operation op; + + /** + * The identifier for the value in patch corresponding to this outcome. + * If the value in the patch was null, this identifier should also be null. + */ + private String identifier; + + /** + * The path for the patch corresponding to this outcome. + */ + private final String path; + + /** + * Create an outcome associated with a submitted patch, as part of a JSON + * patch API request. + * + * @param op + * @param identifier + * @param path Review Comment: Looks like these parameters are missing documentation? ########## guacamole/src/main/frontend/src/translations/en.json: ########## @@ -5,7 +5,7 @@ "APP" : { "NAME" : "Apache Guacamole", - "VERSION" : "${project.version}", + "VERSION" : "1.5.0", Review Comment: Any reason this is being set statically, here? ########## guacamole/src/main/frontend/src/translations/en.json: ########## @@ -183,6 +183,79 @@ }, + "IMPORT": { + + "ACTION_ACKNOWLEDGE" : "@:APP.ACTION_ACKNOWLEDGE", + + "BUTTON_CANCEL": "Cancel", + "BUTTON_CLEAR": "Clear", + "BUTTON_IMPORT": "Import Connections", + + "CONNECTIONS_IMPORTED_SUCCESS": "{NUMBER} connections imported successfully.", + + "DIALOG_HEADER_ERROR" : "@:APP.DIALOG_HEADER_ERROR", + "DIALOG_HEADER_SUCCESS": "Success", + + "FIELD_PLACEHOLDER_FILTER" : "@:APP.FIELD_PLACEHOLDER_FILTER", + + "HEADER": "Connection Import", + + "HELP_HEADER": "Connection Import File Format", + + "HELP_FILE_TYPE_HEADER": "File Types", + "HELP_FILE_TYPE_DESCRIPTION" : "Three file types are supported for connection import: CSV, JSON, and YAML. The same data may be specified by each file type. This must include the connection name and protocol. Optionally, a connection group location, a list of users and/or user groups to grant access, connection parameters, or connection protocols may also be specified. Any users or user groups that do not exist in the current data source will be automatically created.", + + "HELP_CSV_HEADER": "CSV Format", + "HELP_CSV_DESCRIPTION": "A connection import CSV file has one connection record per row. Each column will specify a connection field. At minimum the connection name and protocol must be specified.", + "HELP_CSV_MORE_DETAILS": "The CSV header for each row specifies the connection field. The connection group ID that the connection should be imported into may be directly specified with \"parentIdentifier\", or the path to the parent group may be specified using \"group\" as shown below. In most cases, there should be no conflict between fields, but if needed, an \" (attribute)\" or \" (parameter)\" suffix may be added to disambiguate. Lists of user or user group identifiers must be semicolon-seperated.ยน", + + "HELP_JSON_HEADER": "JSON Format", + "HELP_JSON_DESCRIPTION": "A connection import JSON file is a list of connection objects. At minimum the connection name and protocol must be specified in each connection object.", + "HELP_JSON_MORE_DETAILS": "The connection group ID that the connection should be imported into may be directly specified with a \"parentIdentifier\" field, or the path to the parent group may be specified using a \"group\" field as shown below. An array of user and user group identifiers to grant access to may be specified per connection.", + + "HELP_YAML_HEADER": "YAML Format", + "HELP_YAML_DESCRIPTION": "A connection import YAML file is a list of connection objects with exactly the same structure as the JSON format.", + + "HELP_SEMICOLON_FOOTNOTE": "If present, semicolons can be escaped with a backslash, e.g. \"first\\\\;last\"", + + "ERROR_AMBIGUOUS_CSV_HEADER": + "Ambiguous CSV Header \"{HEADER}\" could be either a connection attribute or parameter", + "ERROR_ARRAY_REQUIRED": + "The provided file must contain a list of connections", + "ERROR_DUPLICATE_CSV_HEADER": + "Duplicate CSV Header: {HEADER}", + "ERROR_EMPTY_FILE": "The provided file is empty", + "ERROR_INVALID_CSV_HEADER": + "Invalid CSV Header \"{HEADER}\" is neither an attribute or parameter", + "ERROR_INVALID_GROUP": "No group matching \"{GROUP}\" found", + "ERROR_INVALID_FILE_TYPE": + "Unsupported file type: \"{TYPE}\"", + "ERROR_INVALID_USER_IDENTIFIERS": + "Users not found: {IDENTIFIER_LIST}", + "ERROR_INVALID_USER_GROUP_IDENTIFIERS": + "User Groups not found: {IDENTIFIER_LIST}", + "ERROR_NO_FILE_SUPPLIED": "Please select a file to import", + "ERROR_AMBIGUOUS_PARENT_GROUP": + "Both group and parentIdentifier may be not specified at the same time", + "ERROR_REQUIRED_PROTOCOL": + "No connection protocol found in the provided file", + "ERROR_REQUIRED_NAME": + "No connection name found in the provided file", + + "ERROR_FILE_SINGLE_ONLY": "Please upload only a single file at a time", + + "TABLE_HEADER_NAME" : "Name", + "TABLE_HEADER_PROTOCOL" : "Protocol", + "TABLE_HEADER_ERRORS" : "Errors", + "TABLE_HEADER_ROW_NUMBER": "Row #", + + "UPLOAD_FILE_TYPES": "CSV, JSON, or YAML", + "UPLOAD_HELP_LINK": "View Format Tips", + "UPLOAD_DROP_TITLE": "Drop a File Here", + "UPLOAD_BROWSE_LINK": "Browse for File" + + }, Review Comment: Seems like the style, here, doesn't match the other sections, in a couple of ways: * Generally things are organized alphabetically, and so `ERROR_` should be between `DIALOG_` and `FIELD_`. * There are several line breaks here between the key and value pairs. ########## guacamole/src/main/java/org/apache/guacamole/rest/directory/DirectoryResource.java: ########## @@ -386,46 +417,210 @@ public Map<String, ExternalType> getObjects( /** * Applies the given object patches, updating the underlying directory - * accordingly. This operation currently only supports deletion of objects - * through the "remove" patch operation. The path of each patch operation is - * of the form "/ID" where ID is the identifier of the object being - * modified. + * accordingly. This operation supports addition and removal of objects + * through the "add" and "remove" patch operation. The path of each patch + * operation is of the form "/ID" where ID is the identifier of the object + * being modified. In the case of object creation, the identifier is + * ignored, as the identifier will be automatically provided. This operation + * is atomic. * * @param patches * The patches to apply for this request. * * @throws GuacamoleException - * If an error occurs while deleting the objects. + * If an error occurs while adding, updating, or removing objects. + * + * @return + * A response describing the outcome of each patch. Only the identifier + * of each patched object will be included in the response, not the + * full object. */ @PATCH - public void patchObjects(List<APIPatch<String>> patches) + public APIPatchResponse patchObjects(List<APIPatch<ExternalType>> patches) throws GuacamoleException { - // Apply each operation specified within the patch - for (APIPatch<String> patch : patches) { + // An outcome for each patch included in the request. This list + // may include both success and failure responses, though the + // presense of any failure would indicated that the entire + // request has failed and no changes have been made. + List<APIPatchOutcome> patchOutcomes = new ArrayList<>(); - // Only remove is supported - if (patch.getOp() != APIPatch.Operation.remove) - throw new GuacamoleUnsupportedException("Only the \"remove\" " - + "operation is supported."); + // Perform all requested operations atomically + directory.tryAtomically(new AtomicDirectoryOperation<InternalType>() { - // Retrieve and validate path - String path = patch.getPath(); - if (!path.startsWith("/")) - throw new GuacamoleClientException("Patch paths must start with \"/\"."); + @Override + public void executeOperation(boolean atomic, Directory<InternalType> directory) + throws GuacamoleException { + + // If the underlying directory implentation does not support + // atomic operations, abort the patch operation. This REST + // endpoint requires that operations be performed atomically. + if (!atomic) + throw new GuacamoleUnsupportedException( + "Atomic operations are not supported. " + + "The patch cannot be executed."); + + // Keep a list of all objects that have been successfully + // added or removed + Collection<InternalType> addedObjects = new ArrayList<>(); + Collection<String> removedIdentifiers = new ArrayList<>(); + + // A list of all responses associated with the successful + // creation of new objects + List<APIPatchOutcome> creationSuccesses = new ArrayList<>(); + + // True if any operation in the patch failed. Any failure will + // fail the request, though won't result in immediate stoppage + // since more errors may yet be uncovered. + boolean failed = false; + + // Apply each operation specified within the patch + for (APIPatch<ExternalType> patch : patches) { + + // Retrieve and validate path + String path = patch.getPath(); + if (!path.startsWith("/")) + throw new GuacamoleClientException("Patch paths must start with \"/\"."); + + if(patch.getOp() == APIPatch.Operation.add) { Review Comment: I hesitate to question this at all, because I know you know what you're doing, but i"ll throw a couple of things out there - feel free to ignore them if you've already considered and have reasons for doing this the way you have it written, here... * Throughout this section, there are multiple calls to `patch.getOp()`. I don't know what the cost is in terms of doing that versus assigning the value to a variable and checking throughout this section? Maybe it's negligible, and doesn't matter, and it makes sense to keep it this way? Or maybe it occupies more memory than it's worth and the cost of doing it this way is less than going the variable route? * I know there are only two operations here (three if you count the catch-all at the end), but any reason to go `if/else` rather than a `switch()` statement? ########## extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/JDBCDirectory.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.auth.jdbc.base; + +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.net.auth.AtomicDirectoryOperation; +import org.apache.guacamole.net.auth.Directory; +import org.apache.guacamole.net.auth.Identifiable; +import org.mybatis.guice.transactional.Transactional; + +/** + * An implementation of Directory that uses database transactions to guarantee + * atomicity for any operations supplied to tryAtomically(). + */ Review Comment: Does this implementation, coupled with the fact that it is used elsewhere, mean that all JDBC operations will be attempted atomically? And does that have any potential adverse impacts from a performance perspective, particularly when you have multiple people accessing the Guacamole UI, starting and stopping connections, which will involve DB reads to retrieve connection data and DB writes for history entries, etc. Sorry if that's a stupid or obvious question... ########## guacamole/src/main/java/org/apache/guacamole/rest/directory/DirectoryResource.java: ########## @@ -386,46 +417,210 @@ public Map<String, ExternalType> getObjects( /** * Applies the given object patches, updating the underlying directory - * accordingly. This operation currently only supports deletion of objects - * through the "remove" patch operation. The path of each patch operation is - * of the form "/ID" where ID is the identifier of the object being - * modified. + * accordingly. This operation supports addition and removal of objects + * through the "add" and "remove" patch operation. The path of each patch + * operation is of the form "/ID" where ID is the identifier of the object + * being modified. In the case of object creation, the identifier is + * ignored, as the identifier will be automatically provided. This operation + * is atomic. * * @param patches * The patches to apply for this request. * * @throws GuacamoleException - * If an error occurs while deleting the objects. + * If an error occurs while adding, updating, or removing objects. + * + * @return + * A response describing the outcome of each patch. Only the identifier + * of each patched object will be included in the response, not the + * full object. */ @PATCH - public void patchObjects(List<APIPatch<String>> patches) + public APIPatchResponse patchObjects(List<APIPatch<ExternalType>> patches) throws GuacamoleException { - // Apply each operation specified within the patch - for (APIPatch<String> patch : patches) { + // An outcome for each patch included in the request. This list + // may include both success and failure responses, though the + // presense of any failure would indicated that the entire Review Comment: presense -> presence ? ########## guacamole/src/main/java/org/apache/guacamole/rest/APIError.java: ########## @@ -21,6 +21,8 @@ import java.util.Collection; import java.util.Collections; +import java.util.List; + Review Comment: Extra line. -- 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]
