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/31 00:08:05 UTC

[GitHub] guacamole-client pull request #233: GUACAMOLE-96: Allow extensions to volunt...

GitHub user mike-jumper opened a pull request:

    https://github.com/apache/guacamole-client/pull/233

    GUACAMOLE-96: Allow extensions to voluntarily store each other's data.

    Part of the core requirements of implementing the TOTP authentication extension is the ability for extensions to voluntarily store object attributes which have been added by another extension, and for the extension adding said attributes to detect whether this is supported.
    
    This change modifies the database authentication to store custom attributes which have been added to supported objects by other extensions. The REST services for managing these objects have also been modified such that attributes not explicitly declared as supported will be stripped from received sets of attributes, allowing extensions to trust that any attributes which they don't explicitly support must have come from an extension which intentionally added those attributes (and not from a malicious user). It is the duty of the extension adding such attributes to handle its own validation.
    
    The TOTP extension leverages this to rely upon storage provided by the database authentication (or any other extension which chooses to provide storage) to store TOTP-specific keys and state information. In older versions of Guacamole, the only possible approach would have been for the TOTP extension to provide its own storage.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/mike-jumper/guacamole-client totp-02-of-03-storage

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/guacamole-client/pull/233.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 #233
    
----
commit ce5e31703d3ffbce818b20443646720a5656e6b8
Author: Michael Jumper <mj...@...>
Date:   2017-10-29T19:59:50Z

    GUACAMOLE-96: Restrict submitted attributes to those explicitly declared by the UserContext.

commit 743aef919b8a8d30a60871392e97a0acf614b7f3
Author: Michael Jumper <mj...@...>
Date:   2017-10-29T20:09:45Z

    GUACAMOLE-96: Document semantics of voluntary attribute storage and guaranteed sanitization.

commit 503cbe6535ca50513e573d00b71d110a5f7c52d3
Author: Michael Jumper <mj...@...>
Date:   2017-10-29T22:33:10Z

    GUACAMOLE-96: Extract Attributes interface from objects which provide getAttributes() / setAttributes().

commit b1d8da6fd10d08cdb242979b1ed4bcf934a06813
Author: Michael Jumper <mj...@...>
Date:   2017-11-19T00:42:13Z

    GUACAMOLE-96: Add object- and model-level support for storage of arbitrary attributes.

commit 34d215a5582cb630db36bcfebb203dd9130dff7b
Author: Michael Jumper <mj...@...>
Date:   2017-11-22T20:29:58Z

    GUACAMOLE-96: Add base support within JDBC auth for storage of arbitrary attributes from other extensions.

commit e3debac27955e9ab27f43ebecffff033ca55e556
Author: Michael Jumper <mj...@...>
Date:   2017-11-23T00:47:56Z

    GUACAMOLE-96: Map base JDBC support for arbitrary attributes to PostgreSQL tables.

commit be489b899460ba5d4925b1abaf3a92e528a3ccfa
Author: Michael Jumper <mj...@...>
Date:   2017-11-25T19:41:54Z

    GUACAMOLE-96: Map base JDBC support for arbitrary attributes to MySQL tables.

commit 58c1c1a863b5ed4aede95900dae6ff45a439572c
Author: Michael Jumper <mj...@...>
Date:   2017-11-25T20:29:39Z

    GUACAMOLE-96: Map base JDBC support for arbitrary attributes to SQL Server tables.

commit 88f0eb6ad43d114f11adb040cb83bb1b81d2cc19
Author: Michael Jumper <mj...@...>
Date:   2018-01-30T21:24:32Z

    GUACAMOLE-96: Correct version number of upgrade scripts for next release (it will be 1.0.0, not 0.9.15).

----


---

[GitHub] guacamole-client pull request #233: GUACAMOLE-96: Allow extensions to volunt...

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/233#discussion_r165176557
  
    --- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/resources/org/apache/guacamole/auth/jdbc/user/UserMapper.xml ---
    @@ -123,12 +148,29 @@
                 </foreach>
                 AND guacamole_user_permission.user_id = #{user.objectID,jdbcType=INTEGER}
                 AND permission = 'READ'
    -        GROUP BY guacamole_user.user_id
    +        GROUP BY guacamole_user.user_id;
    +
    +        SELECT
    +            guacamole_user_attribute.user_id,
    +            guacamole_user_attribute.attribute_name,
    +            guacamole_user_attribute.attribute_value
    +        FROM guacamole_user_attribute
    +        JOIN guacamole_user ON guacamole_user.user_id = guacamole_user_attribute.user_id
    +        JOIN guacamole_user_permission ON affected_user_id = guacamole_user.user_id
    +        WHERE username IN
    +            <foreach collection="identifiers" item="identifier"
    +                     open="(" separator="," close=")">
    +                #{identifier,jdbcType=VARCHAR}
    +            </foreach>
    +            AND guacamole_user_permission.user_id = #{user.objectID,jdbcType=INTEGER}
    +            AND permission = 'READ';
    +>>>>>>> bdd43ee08... STORE-ATTRIBUTES: Map base JDBC support for arbitrary attributes to MySQL tables.
    --- End diff --
    
    Whoa whoops. Clearly I botched a rebase.


---

[GitHub] guacamole-client pull request #233: GUACAMOLE-96: Allow extensions to volunt...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/guacamole-client/pull/233


---

[GitHub] guacamole-client pull request #233: GUACAMOLE-96: Allow extensions to volunt...

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/233#discussion_r165129869
  
    --- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/ArbitraryAttributeMap.java ---
    @@ -0,0 +1,162 @@
    +/*
    + * 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 java.util.AbstractCollection;
    +import java.util.Collection;
    +import java.util.HashMap;
    +import java.util.Iterator;
    +
    +/**
    + * Map of arbitrary attribute name/value pairs which can alternatively be
    + * exposed as a collection of model objects.
    + */
    +public class ArbitraryAttributeMap extends HashMap<String, String> {
    +
    +    /**
    +     * Creates a new ArbitraryAttributeMap containing the name/value pairs
    +     * within the given collection of model objects.
    +     *
    +     * @param models
    +     *     The model objects of all attributes which should be stored in the
    +     *     new map as name/value pairs.
    +     *
    +     * @return
    +     *     A new ArbitraryAttributeMap containing the name/value pairs within
    +     *     the given collection of model objects.
    +     */
    +    public static ArbitraryAttributeMap fromModelCollection(Collection<ArbitraryAttributeModel> models) {
    +
    +        // Add all name/value pairs from the given collection to the map
    +        ArbitraryAttributeMap map = new ArbitraryAttributeMap();
    +        for (ArbitraryAttributeModel model : models)
    +            map.put(model.getName(), model.getValue());
    +
    +        return map;
    +
    +    }
    +
    +    /**
    +     * Returns a collection of model objects which mirrors the contents of this
    +     * ArbitraryAttributeMap. Each name/value pair within the map is reflected
    +     * by a corresponding model object within the returned collection. Removing
    +     * a model object from the collection removes the corresponding name/value
    +     * pair from the map. Adding a new model object to the collection adds a
    +     * corresponding name/value pair to the map. Changes to a model object
    +     * within the collection are NOT reflected on the map, however.
    +     *
    +     * @return
    +     *     A collection of model objects which mirrors the contents of this
    +     *     ArbitraryAttributeMap.
    +     */
    +    public Collection<ArbitraryAttributeModel> toModelCollection() {
    +        return new AbstractCollection<ArbitraryAttributeModel>() {
    +
    +            @Override
    +            public void clear() {
    +                ArbitraryAttributeMap.this.clear();
    +            }
    +
    +            @Override
    +            public boolean remove(Object o) {
    +
    +                // The Collection view of an ArbitraryAttributeMap can contain
    +                // only ArbitraryAttributeModel objects
    +                if (!(o instanceof ArbitraryAttributeModel))
    +                    return false;
    +
    +                // The only if the value matches
    --- End diff --
    
    I think this comment may have lost some content.


---

[GitHub] guacamole-client pull request #233: GUACAMOLE-96: Allow extensions to volunt...

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/233#discussion_r165123911
  
    --- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/resources/org/apache/guacamole/auth/jdbc/user/UserMapper.xml ---
    @@ -123,12 +148,29 @@
                 </foreach>
                 AND guacamole_user_permission.user_id = #{user.objectID,jdbcType=INTEGER}
                 AND permission = 'READ'
    -        GROUP BY guacamole_user.user_id
    +        GROUP BY guacamole_user.user_id;
    +
    +        SELECT
    +            guacamole_user_attribute.user_id,
    +            guacamole_user_attribute.attribute_name,
    +            guacamole_user_attribute.attribute_value
    +        FROM guacamole_user_attribute
    +        JOIN guacamole_user ON guacamole_user.user_id = guacamole_user_attribute.user_id
    +        JOIN guacamole_user_permission ON affected_user_id = guacamole_user.user_id
    +        WHERE username IN
    +            <foreach collection="identifiers" item="identifier"
    +                     open="(" separator="," close=")">
    +                #{identifier,jdbcType=VARCHAR}
    +            </foreach>
    +            AND guacamole_user_permission.user_id = #{user.objectID,jdbcType=INTEGER}
    +            AND permission = 'READ';
    +>>>>>>> bdd43ee08... STORE-ATTRIBUTES: Map base JDBC support for arbitrary attributes to MySQL tables.
    --- End diff --
    
    Don't think this line belongs here...


---