You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by mike-jumper <gi...@git.apache.org> on 2018/01/14 02:14:21 UTC
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
GitHub user mike-jumper opened a pull request:
https://github.com/apache/guacamole-client/pull/225
GUACAMOLE-96: Allow extensions to decorate each other's objects.
Part of the core requirements of implementing the TOTP authentication extension is the ability to add and manage TOTP-specific attributes on the `User` objects of other extensions (to store each user's unique, secret key). To achieve this, this change adds the fundamental ability for extensions to modify each other's objects through adding two new functions to `UserContext`:
1. `decorate()` - allows a `UserContext` from another `AuthenticationProvider` to be wrapped by the current `AuthenticationProvider`
2. `redecorate()` - allows a previously-decorated `UserContext` to be updated in case the underlying `UserContext` has been updated.
See also: https://en.wikipedia.org/wiki/Decorator_pattern
This is admittedly a very large change, but it is only part 1 of 3 as far as TOTP is concerned. Following this is the other core requirement of allowing extensions to voluntarily store each other's data (and modifying the JDBC auth to do so), and finally the TOTP implementation itself.
I expect the documentation surrounding this new API is in need of particular review attention, as describing `decorate()`, `redecorate()`, and related functions proved difficult.
The core decorate/redecorate API
--------------------------------------------------
The `decorate()` function is analogous to `getUserContext()` and is invoked only during the initial authentication process for a user. For a `UserContext` retrieved via `getUserContext()` from a particular `AuthenticationProvider`, `decorate()` is invoked to decorate that `UserContext` for all **other** `AuthenticationProvider`s.
The `redecorate()` function is analogous to `updateUserContext() and is invoked only for new requests related to an existing session. If an `AuthenticationProvider` previously decorated a `UserContext` via `decorate()` or `redecorate()`, it is given that `UserContext` again as well as the `UserContext` returned by `updateUserContext() to allow the `AuthenticationProvider` to clean up its old object and decorate the new object.
Simplified decorate/undecorate API
--------------------------------------------------
To make things easier, I've also added several abstract classes for wrapping objects, as well as abstract `Directory` implementations which simplify decoration semantics. A typical pair of `decorate()` and `redecorate()` implementations would be something like:
```java
@Override
public UserContext decorate(UserContext context,
AuthenticatedUser authenticatedUser, Credentials credentials)
throws GuacamoleException {
// Decorate the UserContext such that all Connection objects are wrapped
return new DelegatingUserContext(context) {
@Override
public Directory<Connection> getConnectionDirectory()
throws GuacamoleException {
return new DecoratingDirectory<Connection>(super.getConnectionDirectory()) {
@Override
protected Connection decorate(Connection object)
throws GuacamoleException {
// Return a wrapped version of the original object which
// adds functionality of some kind
return new MyConnection(object);
}
@Override
protected Connection undecorate(Connection object)
throws GuacamoleException {
// Return the original object
return ((MyConnection) object).getWrappedConnection();
}
};
}
};
}
@Override
public UserContext redecorate(UserContext decorated, UserContext context,
AuthenticatedUser authenticatedUser, Credentials credentials)
throws GuacamoleException {
// No need to clean up / update the previously-decorated UserContext
return decorate(context, authenticatedUser, credentials);
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mike-jumper/guacamole-client totp-01-of-03-decorate
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/225.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #225
----
commit ffad1898b64411afcf1484de4419c0cc47758c25
Author: Michael Jumper <mj...@...>
Date: 2017-10-27T18:07:35Z
GUACAMOLE-96: Add API support for augmenting functionality of other extensions.
commit 41059f5e09016669b7384f71fd636ee28a8bc7c6
Author: Michael Jumper <mj...@...>
Date: 2017-10-27T18:08:08Z
GUACAMOLE-96: Add convenience class for overriding the behavior of an existing UserContext.
commit a745569f1303dd2f1f4a16190096680d53dbc9cd
Author: Michael Jumper <mj...@...>
Date: 2017-10-27T21:30:42Z
GUACAMOLE-96: Invoke decorate() for all AuthenticationProviders when creating or updating the UserContext.
commit 7357e51b589f94d85d52b2e336812033f61d1767
Author: Michael Jumper <mj...@...>
Date: 2017-10-29T02:12:05Z
GUACAMOLE-96: Add redecorate() function with semantics analogous to updateUserContext().
commit a915f7b190bd6a0ce01e3e991daabed95d26493f
Author: Michael Jumper <mj...@...>
Date: 2017-10-29T05:43:31Z
GUACAMOLE-96: Add convenience class for decorating the objects returned by a Directory.
commit b37e041d3ee263873cc55388cfa0cb9079f32bfb
Author: Michael Jumper <mj...@...>
Date: 2017-10-29T05:56:07Z
GUACAMOLE-96: Add convenience classes for overriding the behavior of objects commonly stored in a Directory.
commit 63bb3a033ae344f81ad29795c4f19e277d39d7b0
Author: Michael Jumper <mj...@...>
Date: 2017-11-22T17:52:59Z
GUACAMOLE-96: Objects should be decorated upon add(), not undecorated (they by definition come from an external source, not the decorated extension).
----
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/225#discussion_r161805667
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/DecoratingDirectory.java ---
@@ -0,0 +1,133 @@
+/*
+ * 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.net.auth;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Directory implementation which simplifies decorating the objects within an
+ * underlying Directory. The decorate() and undecorate() functions must be
+ * implemented to define how each object is decorated, and how that decoration
+ * may be removed.
+ *
+ * @param <ObjectType>
+ * The type of objects stored within this Directory.
+ */
+public abstract class DecoratingDirectory<ObjectType extends Identifiable>
+ extends DelegatingDirectory<ObjectType> {
+
+ /**
+ * Creates a new DecoratingDirectory which decorates the objects within
+ * the given directory.
+ *
+ * @param directory
+ * The Directory whose objects are being decorated.
+ */
+ public DecoratingDirectory(Directory<ObjectType> directory) {
+ super(directory);
+ }
+
+ /**
+ * Given an object retrieved from a Directory which originates from a
+ * different AuthenticationProvider, returns an identical type of object
+ * optionally wrapped with additional information, functionality, etc. If
+ * this directory chooses to decorate the object provided, it is up to the
+ * implementation of that decorated object to properly pass through
+ * operations as appropriate, as well as provide for an eventual
+ * undecorate() operation. All objects retrieved from this
+ * DecoratingDirectory will first be passed through this function.
+ *
+ * @param object
+ * An object from a Directory which originates from a different
+ * AuthenticationProvider.
+ *
+ * @return
+ * An object which may have been decorated by this
+ * DecoratingDirectory. If the object was not decorated, the original,
+ * unmodified object may be returned instead.
+ *
+ * @throws GuacamoleException
+ * If the provided object cannot be decorated due to an error.
+ */
+ protected abstract ObjectType decorate(ObjectType object)
+ throws GuacamoleException;
+
+ /**
+ * Given an object originally returned from a call to this
+ * DecoratingDirectory's decorate() function, reverses the decoration
+ * operation, returning the original object. This function is effectively
+ * the exact inverse of the decorate() function. The return value of
+ * undecorate(decorate(X)) must be identically X. All objects given to this
+ * DecoratingDirectory via add() or update() will first be passed through
+ * this function.
+ *
+ * @param object
+ * An object which was originally returned by a call to this
+ * DecoratingDirectory's decorate() function.
+ *
+ * @return
+ * The original object which was provided to this DecoratingDirectory's
+ * decorate() function.
+ *
+ * @throws GuacamoleException
+ * If the provided object cannot be undecorated due to an error.
+ */
+ protected abstract ObjectType undecorate(ObjectType object)
+ throws GuacamoleException;
+
+ @Override
+ public ObjectType get(String identifier) throws GuacamoleException {
+
+ // Decorate only if object exists
+ ObjectType object = super.get(identifier);
+ if (object == null)
+ return object;
+
--- End diff --
This is more a curiosity question for me than anything, but is there some reason you chose this implementation rather than, for example:
if (object != null)
return decorate(object);
return null;
? I guess I'm a little fuzzy on why you'd return the object if there's nothing there, anyway, instead of flipping the logic?
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/225#discussion_r162331824
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java ---
@@ -260,6 +260,23 @@ public UserContext updateUserContext(UserContext context,
}
+ @Override
+ public UserContext decorate(UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+
+ // Simply return the given context, decorating nothing
+ return context;
+
+ }
+
+ @Override
+ public UserContext redecorate(UserContext decorated, UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+ return decorate(context, authenticatedUser, credentials);
--- End diff --
:+1:
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/225#discussion_r161807541
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java ---
@@ -260,6 +260,23 @@ public UserContext updateUserContext(UserContext context,
}
+ @Override
+ public UserContext decorate(UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+
+ // Simply return the given context, decorating nothing
+ return context;
+
+ }
+
+ @Override
+ public UserContext redecorate(UserContext decorated, UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+ return decorate(context, authenticatedUser, credentials);
--- End diff --
Any particular reason in this instance you chose to pass up to the `decorate()` method rather than just returning the context? In the extension modules you just `return context`, instead - why the difference?
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/225
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/225#discussion_r162236186
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/DecoratingDirectory.java ---
@@ -0,0 +1,133 @@
+/*
+ * 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.net.auth;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import org.apache.guacamole.GuacamoleException;
+
+/**
+ * Directory implementation which simplifies decorating the objects within an
+ * underlying Directory. The decorate() and undecorate() functions must be
+ * implemented to define how each object is decorated, and how that decoration
+ * may be removed.
+ *
+ * @param <ObjectType>
+ * The type of objects stored within this Directory.
+ */
+public abstract class DecoratingDirectory<ObjectType extends Identifiable>
+ extends DelegatingDirectory<ObjectType> {
+
+ /**
+ * Creates a new DecoratingDirectory which decorates the objects within
+ * the given directory.
+ *
+ * @param directory
+ * The Directory whose objects are being decorated.
+ */
+ public DecoratingDirectory(Directory<ObjectType> directory) {
+ super(directory);
+ }
+
+ /**
+ * Given an object retrieved from a Directory which originates from a
+ * different AuthenticationProvider, returns an identical type of object
+ * optionally wrapped with additional information, functionality, etc. If
+ * this directory chooses to decorate the object provided, it is up to the
+ * implementation of that decorated object to properly pass through
+ * operations as appropriate, as well as provide for an eventual
+ * undecorate() operation. All objects retrieved from this
+ * DecoratingDirectory will first be passed through this function.
+ *
+ * @param object
+ * An object from a Directory which originates from a different
+ * AuthenticationProvider.
+ *
+ * @return
+ * An object which may have been decorated by this
+ * DecoratingDirectory. If the object was not decorated, the original,
+ * unmodified object may be returned instead.
+ *
+ * @throws GuacamoleException
+ * If the provided object cannot be decorated due to an error.
+ */
+ protected abstract ObjectType decorate(ObjectType object)
+ throws GuacamoleException;
+
+ /**
+ * Given an object originally returned from a call to this
+ * DecoratingDirectory's decorate() function, reverses the decoration
+ * operation, returning the original object. This function is effectively
+ * the exact inverse of the decorate() function. The return value of
+ * undecorate(decorate(X)) must be identically X. All objects given to this
+ * DecoratingDirectory via add() or update() will first be passed through
+ * this function.
+ *
+ * @param object
+ * An object which was originally returned by a call to this
+ * DecoratingDirectory's decorate() function.
+ *
+ * @return
+ * The original object which was provided to this DecoratingDirectory's
+ * decorate() function.
+ *
+ * @throws GuacamoleException
+ * If the provided object cannot be undecorated due to an error.
+ */
+ protected abstract ObjectType undecorate(ObjectType object)
+ throws GuacamoleException;
+
+ @Override
+ public ObjectType get(String identifier) throws GuacamoleException {
+
+ // Decorate only if object exists
+ ObjectType object = super.get(identifier);
+ if (object == null)
+ return object;
+
--- End diff --
Yeah, that's a fair point. I'll change this to `return null`.
---
[GitHub] guacamole-client pull request #225: GUACAMOLE-96: Allow extensions to decora...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/225#discussion_r162236498
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/simple/SimpleAuthenticationProvider.java ---
@@ -260,6 +260,23 @@ public UserContext updateUserContext(UserContext context,
}
+ @Override
+ public UserContext decorate(UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+
+ // Simply return the given context, decorating nothing
+ return context;
+
+ }
+
+ @Override
+ public UserContext redecorate(UserContext decorated, UserContext context,
+ AuthenticatedUser authenticatedUser, Credentials credentials)
+ throws GuacamoleException {
+ return decorate(context, authenticatedUser, credentials);
--- End diff --
Mainly because this is part of the public API and not the internal workings of an extension. I chose to do things this way so that the class remains compliant with the expectations of `decorate()` / `redecorate()` if `decorate()` is overridden while `redecorate()` is not.
---