GUACAMOLE-2250: Add an option to create groups on connection import.#1197
GUACAMOLE-2250: Add an option to create groups on connection import.#1197eugen-keeper wants to merge 1 commit into
Conversation
eugen-keeper
force-pushed
the
guacamole-2250-create-groups-auto
branch
5 times, most recently
from
3941efa to
6d28ce9
Compare
eugen-keeper
force-pushed
the
guacamole-2250-create-groups-auto
branch
3 times, most recently
from
02790ff to
3985947
Compare
| return connectionGroupService.saveConnectionGroup(dataSource, newGroup) | ||
| .then(() => { | ||
| identifierByPath[nextPath] = newGroup.identifier; | ||
| }); |
There was a problem hiding this comment.
My understanding is that we can't create a multi-level group in a single REST API call, since we need the parent identifier before we can create each child group.
Since we don't catch failures here, a failed group creation will abort the import, potentially leaving behind full and intermediate groups that were created earlier... Should we track and delete those groups on failure, or is it better to document that partial group creation can occur?
There was a problem hiding this comment.
Yes — that’s correct. Guacamole’s connection group API creates one group per POST, and each child needs its parent’s identifier. Only ROOT is known upfront; every other parent gets a server-assigned identifier when it’s created, so a multi-level hierarchy has to be built level by level, not in a single call.
Documented (lines 373 - 377). A rollback would be complex (track only new groups, delete deepest-first, handle delete errors) and wouldn’t make the whole import atomic anyway — groups can also remain if connection import fails later.
| parseResult.connectionObjects.forEach(connectionObject => { | ||
| if (!connectionObject.parentIdentifier) { | ||
| connectionObject.parentIdentifier = | ||
| identifierByPath[connectionObject.group] || identifierByPath['ROOT']; |
There was a problem hiding this comment.
If the group path can't be (somehow) resolved, we'll silently place the connection under ROOT. Is that intentional, or should we abort importing that connection instead? And add a warning, either way?
There was a problem hiding this comment.
Fixed! Unresolved paths now show in the import error table and block the import.
There was a problem hiding this comment.
I think there is still an issue here. I Import (Automatically create groups) the following.
[
{ "name": "repro-5", "protocol": "vnc", "group": "ROOT//Repro5" }
]
But no error is reported:
And the group was created.
There was a problem hiding this comment.
The issue was in the double slash after root. I added some normalization to fix the issue. Also I improved the error watch a little to catch such cases in the future.
There was a problem hiding this comment.
Just noticed this:
If not, any errors will be displayed and any already-created entities
* will be rolled back.
| // If an error occurs while trying to create users or groups, | ||
| // display the error to the user. | ||
| .catch(error => { | ||
| $scope.processing = false; |
There was a problem hiding this comment.
Add console.error(error) here, so this can be seen on the DevTools console?? Or maybe better, add to handleError()? Multiple instances in file.
There was a problem hiding this comment.
Added it into handleError
There was a problem hiding this comment.
Existing nit: I noticed lookups.groupPathsByIdentifier names are reversed. `lookups.groupIdentifiersByPath' May be worth renaming correctly to prevent a future bug.
I searched the file for use to see how they were indexed, and found a couple discrepancies it may be worth looking into.
groupPathsByIdentifier (mapped by path):
&& !groupPathsByIdentifier[providedIdentifier]) { <<<
group = groupPathsByIdentifier[providedIdentifier]; <<<
| <span ng-attr-title="{{'IMPORT.HELP_EXISTING_PERMISSION_MODE' | translate}}" class="help"></span> | ||
| </li> | ||
| <li> | ||
| <input type="checkbox" |
There was a problem hiding this comment.
You may want to look into guacamole-manual/src/batach-import.md and update it with the new UI setting.
There was a problem hiding this comment.
I will submit a PR to there.
eugen-keeper
force-pushed
the
guacamole-2250-create-groups-auto
branch
from
3985947 to
c1a168d
Compare
|
I noticed 1 other thing (unrelated to this PR): the CONNECTION IMPORT screen: if no file is selected, the Cancel button is grayed out & there's no way to exit the screen. Besides selecting another menu option. I'll file s separate issue for this as I think the fix is non-trivial. 
|
eugen-keeper
force-pushed
the
guacamole-2250-create-groups-auto
branch
from
c1a168d to
4c5e2e1
Compare
|
2 participants
No description provided.