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:
[email protected]
With regards,
Apache Git Services
