[GitHub] mike-jumper opened a new pull request #360: GUACAMOLE-524: Update connect() API changes for backward compatibility with 1.0.0.
mike-jumper opened a new pull request #360: GUACAMOLE-524: Update connect() API changes for backward compatibility with 1.0.0. URL: https://github.com/apache/guacamole-client/pull/360 [As discussed on dev@](https://lists.apache.org/thread.html/c74221a629bd2da53996b0f6e4cfebc4647ac1239aa8817a45fd0c12@%3Cdev.guacamole.apache.org%3E), these changes modify the `Connectable` interface such that implementations of the previous version of the `connect()` function will continue to work, even if those implementations extended `SimpleConnection` and overrode the old, now-deprecated function. This is achieved through: 1. Continuing to use the old `connect()` as the basis for the implementation. 2. Using a `ThreadLocal` to make the additional `Map` parameter available to the old `connect()` within a strict context. The `SimpleConnection` class and related classes are also modified to provide the same behavior as past releases, particularly for downstream subclasses. With these changes, things are ABI-compatible with 1.0.0. Things are also API-compatible with 1.0.0, though continuing to use/override the old `connect()` will result in compiler warnings. Behavior of the relevant classes should now be as follows: | Class | Behavior in 1.0.0 and older | New behavior | | - | --- | | | `SimpleAuthenticationProvider` | Tokens are applied and "baked-in" when the user authenticates. | Tokens are applied dynamically when connecting. | | `SimpleUserContext` | Tokens are applied only if explicitly implemented. | Tokens are applied only if explicitly implemented *or* if requested via the constructor. | | `SimpleConnection` | Tokens are applied only if explicitly implemented. | Tokens are applied only if explicitly implemented *or* if requested via the constructor. | Overall, behavior should be identical to 1.0.0 with the following exceptions: * Subclasses of `SimpleAuthenticationProvider` will now handle additional tokens and apply those tokens in context of the connection, not at time of authentication. * The result of calling `getConfiguration()` on a connection accessed via a subclass of `SimpleAuthenticationProvider` will now contain any tokens in uninterpreted form. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
Re: [DISCUSS] Beyond 1.0.0
On Mon, Jan 21, 2019 at 5:29 PM Mike Jumper wrote: > On Sun, Jan 20, 2019 at 2:24 PM Mike Jumper wrote: > >> On Fri, Jan 18, 2019 at 12:52 PM Nick Couchman wrote: >> >>> On Fri, Jan 18, 2019 at 2:37 PM Mike Jumper wrote: >>> ... >>> > >>> > A compat layer would be pretty tricky. >>> > >>> > If we can somehow modify the API changes such that things are >>> inherently >>> > compatible, then fairly easy. Maybe something can be done with default >>> > methods now that we're Java 8+? >>> > >>> >>> So, for a change like this (to the Connectable interface): >>> >>> https://github.com/apache/guacamole-client/commit/dfd43327618bd625bac7ce4fd35f9ccfe729ec6e#diff-1d2cb5f9d0009ea051d8a6211b20aaea >>> >>> something like this: >>> >>> https://github.com/necouchman/guacamole-client/commit/d53a6c3be576526ace6acf0432cab2480785a0ae >>> >>> ?? >>> >>> >> Something similar to that, yes. We would need default implementations for >> both old and new versions of connect(): >> >> >> https://github.com/mike-jumper/guacamole-client/commit/4a1527b1d4311bbf9d76468141dc68d02a90efa4 >> >> We would still run into trouble with the SimpleConnection class for any >> downstream uses which override the old connect(). Those overrides would >> suddenly cease to have any effect. Further, if such a downstream use also >> uses SimpleAuthenticationProvider, they might expect the provided >> GuacamoleConfiguration to already have tokens applied (the old behavior) >> rather than dynamically applied upon connect() (the new behavior). >> >> Maybe it's not possible without a compatibility layer... >> > > I think we can allow the old connect() to be overridden and still work as > intended by leveraging thread-local storage. We could use a thread-local > variable to effectively pass the tokens received by the new connect() such > that they are available internally by SimpleConnection's implementation of > the old connect(). With the new version internally leveraging the old, > users which override either will see the behavior they expect. > > The GuacamoleConfiguration returned by getConfiguration() would still no > longer have tokens applied, unlike past releases where tokens were baked in > before each instance of SimpleConnection was created, but perhaps that's a > reasonable enough sacrifice? > For example: https://github.com/mike-jumper/guacamole-client/commit/7e67dde75155ab88af4570bcfeb3a94175093fc9
Re: [DISCUSS] Beyond 1.0.0
On Sun, Jan 20, 2019 at 2:24 PM Mike Jumper wrote: > On Fri, Jan 18, 2019 at 12:52 PM Nick Couchman wrote: > >> On Fri, Jan 18, 2019 at 2:37 PM Mike Jumper wrote: >> ... >> > >> > A compat layer would be pretty tricky. >> > >> > If we can somehow modify the API changes such that things are inherently >> > compatible, then fairly easy. Maybe something can be done with default >> > methods now that we're Java 8+? >> > >> >> So, for a change like this (to the Connectable interface): >> >> https://github.com/apache/guacamole-client/commit/dfd43327618bd625bac7ce4fd35f9ccfe729ec6e#diff-1d2cb5f9d0009ea051d8a6211b20aaea >> >> something like this: >> >> https://github.com/necouchman/guacamole-client/commit/d53a6c3be576526ace6acf0432cab2480785a0ae >> >> ?? >> >> > Something similar to that, yes. We would need default implementations for > both old and new versions of connect(): > > > https://github.com/mike-jumper/guacamole-client/commit/4a1527b1d4311bbf9d76468141dc68d02a90efa4 > > We would still run into trouble with the SimpleConnection class for any > downstream uses which override the old connect(). Those overrides would > suddenly cease to have any effect. Further, if such a downstream use also > uses SimpleAuthenticationProvider, they might expect the provided > GuacamoleConfiguration to already have tokens applied (the old behavior) > rather than dynamically applied upon connect() (the new behavior). > > Maybe it's not possible without a compatibility layer... > I think we can allow the old connect() to be overridden and still work as intended by leveraging thread-local storage. We could use a thread-local variable to effectively pass the tokens received by the new connect() such that they are available internally by SimpleConnection's implementation of the old connect(). With the new version internally leveraging the old, users which override either will see the behavior they expect. The GuacamoleConfiguration returned by getConfiguration() would still no longer have tokens applied, unlike past releases where tokens were baked in before each instance of SimpleConnection was created, but perhaps that's a reasonable enough sacrifice? - Mike
[GitHub] necouchman commented on a change in pull request #353: GUACAMOLE-577: Support for configuring the Guacamole Proxy in LDAP Connections
necouchman commented on a change in pull request #353: GUACAMOLE-577: Support
for configuring the Guacamole Proxy in LDAP Connections
URL: https://github.com/apache/guacamole-client/pull/353#discussion_r249611660
##
File path:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java
##
@@ -167,18 +201,34 @@
// Parse name
String name = parameter.substring(0, equals);
String value = parameter.substring(equals+1);
-
-config.setParameter(name, value);
+
+// Pull out and set proxy parameters, if present
+// Otherwise set the parameter.
+switch(name) {
+case PROXY_HOST_PARAMETER:
Review comment:
Cool. Would you rather go ahead and make those changes in this PR, or go
the route of separating that into another issue (I'd already opened a separate
JIRA issue for it...)?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] necouchman commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
necouchman commented on a change in pull request #359: GUACAMOLE-617: Extract
Permission Management from JDBC Authentication Module
URL: https://github.com/apache/guacamole-client/pull/359#discussion_r249591965
##
File path:
extensions/guacamole-auth-common-base/modules/guacamole-auth-common-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionAttributeModelInterface.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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.connection;
+
+public interface ConnectionAttributeModelInterface {
Review comment:
Okay, it just seems strange to have an interface that doesn't define
anything at all. However, as with the other empty interfaces that extend a
couple of other interfaces, I would think this should go in the
Morphia-specific code, not in the common extension.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] necouchman commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
necouchman commented on a change in pull request #359: GUACAMOLE-617: Extract
Permission Management from JDBC Authentication Module
URL: https://github.com/apache/guacamole-client/pull/359#discussion_r249591757
##
File path:
extensions/guacamole-auth-common-base/modules/guacamole-auth-common-base/src/main/java/org/apache/guacamole/auth/jdbc/base/ObjectEntityModelInterface.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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;
+
+public interface ObjectEntityModelInterface extends ObjectModelInterface,
EntityModelInterface {
Review comment:
Hmmm...okay, I think I understand. However, if something is specific to the
Morphia extension, I believe those pieces would belong in the code for that
extension and not in the common parts.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] necouchman commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
necouchman commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module URL: https://github.com/apache/guacamole-client/pull/359#issuecomment-456212242 > Before finishing the development of those things I wanted them to be reviewed by you in case I still had to make a big change and I did not have to revise the whole project for these things that you mentioned. I would advise you to go ahead with these changes. There are going to be several revisions, I'm sure, but it's hard to review the code partially. > From what I understand you mean the import of the classes and interfaces, in the upper part, this may have been done by the auto-import of eclipse, I will take it into account to correct it. Yeah, that makes sense if the IDE is trying to split it up. I'm guessing it may be doing the same thing to your indentations - there might be some settings in Eclipse that control these behaviors. I use Netbeans, and some of it is configurable, and some of it it tries to guess from the existing code. Doesn't always do exactly what I want it to do, but mostly it works. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract
Permission Management from JDBC Authentication Module
URL: https://github.com/apache/guacamole-client/pull/359#discussion_r249559627
##
File path:
extensions/guacamole-auth-common-base/modules/guacamole-auth-common-base/src/main/java/org/apache/guacamole/auth/jdbc/connection/ConnectionAttributeModelInterface.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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.connection;
+
+public interface ConnectionAttributeModelInterface {
Review comment:
In Morphia, all the tables need to have a class that identifies them. This
interfaces are used to make reference to those classes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract
Permission Management from JDBC Authentication Module
URL: https://github.com/apache/guacamole-client/pull/359#discussion_r24955
##
File path:
extensions/guacamole-auth-common-base/modules/guacamole-auth-common-base/src/main/java/org/apache/guacamole/auth/jdbc/base/ObjectEntityModelInterface.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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;
+
+public interface ObjectEntityModelInterface extends ObjectModelInterface,
EntityModelInterface {
Review comment:
In Morphia, the relation with EntityModel is implemented by an object within
the classes it is related with. In JDBC, the implementation is done by
inheritance. In order to identify in Morphia the classes that supposedly
inherit from EntityModel, this interface is used.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module URL: https://github.com/apache/guacamole-client/pull/359#discussion_r249556760 ## File path: extensions/guacamole-auth-common-base/modules/guacamole-auth-common-base/pom.xml ## @@ -0,0 +1,104 @@ + + +http://maven.apache.org/POM/4.0.0; +xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance; +xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 +http://maven.apache.org/maven-v4_0_0.xsd;> + +4.0.0 +org.apache.guacamole +guacamole-auth-common-base +jar +guacamole-auth-common-base +http://guacamole.apache.org/ + + +UTF-8 + + + +org.apache.guacamole +guacamole-auth-common +1.0.0 +../../ + + + + + + Review comment: The version has been updated This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on a change in pull request #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module URL: https://github.com/apache/guacamole-client/pull/359#discussion_r249556549 ## File path: .gitignore ## @@ -1,2 +1,3 @@ *~ target/ +/.project Review comment: I have removed this line This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] daniquir commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module
daniquir commented on issue #359: GUACAMOLE-617: Extract Permission Management from JDBC Authentication Module URL: https://github.com/apache/guacamole-client/pull/359#issuecomment-456172559 Like you have been able to verify it's a very laborious and extensive work. I take into account everything you mention about refactoring the nomenclature of the project, it's something I have pending, along with reviewing the documentation of the JavaDoc, the format, etc. Before finishing the development of those things I wanted them to be reviewed by you in case I still had to make a big change and I did not have to revise the whole project for these things that you mentioned. In any case, I will be available for what is necessary in order to modify and give support in this adaptation. I keep revising the code and testing the application, since it practically affects everything. Regarding your doubts, I answer you below. The reason why there are interfaces and empty classes is being able to share the code in a generic way with the extension of Morphia/MongoDB, since the data structure is not exactly the same because the relationships between entities are done by objects. I would like you to clarify a bit this phrase: "In multiple places you've broken the import blocks into multiple sections, which is not consistent with the style in the rest of the code." From what I understand you mean the import of the classes and interfaces, in the upper part, this may have been done by the auto-import of eclipse, I will take it into account to correct it. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
