[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #389: GUACAMOLE-708: Enable auto-creation of users in JDBC modules

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #389: GUACAMOLE-708: Enable auto-creation of users in JDBC modules
URL: https://github.com/apache/guacamole-client/pull/389#discussion_r315087712
 
 

 ##########
 File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/UserService.java
 ##########
 @@ -460,6 +460,47 @@ public ModeledUser retrieveSkeletonUser(AuthenticationProvider authenticationPro
         return user;
         
     }
+    
+    /**
+     * Create a user in the database that does not already exist, setting up an
+     * empty model and inserting both the entity and the user object, and
+     * generating a random password for the account.
+     * 
+     * @param authenticationProvider
+     *     The authentication provider that authenticated the user.
+     * 
+     * @param authenticatedUser
+     *     The authenticated user that is being added to the database.
+     * 
+     * @return
+     *     The ModeledUser associated with the newly-created database object
+     *     for the user.
+     * 
+     * @throws GuacamoleException
+     *     If a ModeledUser cannot be created, or if the user cannot be
+     *     inserted into the database.
+     */
+    public ModeledUser createMissingUser(AuthenticationProvider authenticationProvider,
+            AuthenticatedUser authenticatedUser) throws GuacamoleException {
+        
+        ModeledUser user = getObjectInstance(null,
+                new UserModel(authenticatedUser.getIdentifier()));
+        
+        // Insert the database object
+        entityMapper.insert(user.getModel());
+            
+        // Auto-generate a password
+        user.setPassword(null);
+            
+        // Set up cyclic reference
+        user.setCurrentUser(new ModeledAuthenticatedUser(authenticatedUser,
+            authenticationProvider, user));
+            
+        // Insert the user object
+        userMapper.insert(user.getModel());
 
 Review comment:
   Tweaking `createObject()` (and pretty much everything else) such that it has defined behavior for some unrestricted internal system user makes sense. It would also ease the implementation of some future addition to the extension API which allows third-party extensions to perform actions without requiring the logged-in user to have sufficient privileges (enrolling in TOTP despite lacking `UPDATE` permission on your own user object, for example).
   
   Defining behavior for `null` would be simple but dangerous. It would be all to easy for a bug to result in an unexpected `null` and then bam: privilege escalation.
   
   I like the idea of an internal system user definition. That would require very explicit usage which would not be error prone. I can think of two possible approaches:
   
   1. Creating some wrapper class which *might* contain a `ModeledAuthenticatedUser` (but isn't guaranteed to) and which has some well-defined way to declare that an instance represents a privileged system user that lacks a model.
   2. Creating some base interface which both `RemoteAuthenticatedUser` and `ModeledAuthenticatedUser` implement and which abstracts away permission retrieval. Existing usages of those classes can then be replaced with the interface, and a concrete, privileged implementation of that interface can be created which simply returns true for all permission checks.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services