Upgrade Apache HttpClient 4.5.13 -> 5.1.3 #35

labkey-adam · 2022-09-07T01:03:25Z

Rationale

Latest

Related Pull Requests

Upgrade Apache HttpClient 4.5.13 -> 5.1.3
Switch to Basic auth challenge/response, instead of pre-emptive auth
Switch self-signed cert handling to HttpClient 5.x approach: SSLConnectionSocketFactory must now be set on the ConnectionManager, not on each HttpClientBuilder.
Switch to adding parameters one-by-one to ensure correct escaping (see Command.addParameters()). setCustomQuery() now requires an unescaped query string, which is not compatible with tricky characters (e.g., "&" in a schema name).
Remove deprecated methods

…()` now requires an unescaped query string, which is not compatible with tricky characters (e.g., "&" in a schema name).

labkey-susanh · 2022-09-13T17:30:19Z

labkey-client-api/build.gradle

    api ("com.googlecode.json-simple:json-simple:${jsonSimpleVersion}")
            {
                // exclude this because it gets in the way of our own JSON object implementations from server/api
                exclude group: "org.json", module:"json"
            }
+    api "org.apache.httpcomponents.client5:httpclient5:${httpclient5Version}"
+    api "org.apache.httpcomponents.core5:httpcore5:${httpcore5Version}"


Just checking that these are actually api dependencies (that is, we expose some objects from these jar files in our interfaces). I assume so since they were before.

Yes, unfortunately our wrappers expose a few native HttpClient/HttpCore classes. For example, our Connection.executeRequest() returns CloseableHttpResponse (from HttpClient) which implements ClassicHttpResponse (from HttpCore). I've made a few internal methods private in this PR to reduce the leakage, but we can't eliminate it without more work.

labkey-tchad

Connection.stopImpersonating seems to be broken.
Biologics tests use it fairly broadly and it seems to be failing every time:

org.labkey.remoteapi.CommandException: Failed to stop impersonating
  at app//org.labkey.remoteapi.Connection.stopImpersonate(Connection.java:403)
  at app//org.labkey.test.util.TestUser.stopImpersonating(TestUser.java:123)
  at app//org.labkey.test.tests.biologics.crossfolder.CrossFolderRegistryTest.testSubfolderUserCanEditSubfolderSequence(CrossFolderRegistryTest.java:584)

labkey-adam · 2022-09-13T17:47:50Z

Connection.stopImpersonating seems to be broken. Biologics tests use it fairly broadly and it seems to be failing every time:

org.labkey.remoteapi.CommandException: Failed to stop impersonating
  at app//org.labkey.remoteapi.Connection.stopImpersonate(Connection.java:403)
  at app//org.labkey.test.util.TestUser.stopImpersonating(TestUser.java:123)
  at app//org.labkey.test.tests.biologics.crossfolder.CrossFolderRegistryTest.testSubfolderUserCanEditSubfolderSequence(CrossFolderRegistryTest.java:584)

Yep, I noticed that. Not sure if it's returning a different status code now or what.

labkey-tchad · 2022-09-13T17:56:16Z

Yep, I noticed that. Not sure if it's returning a different status code now or what.

I just ran it locally. It is getting a 200 response now. The normal "Stop Impersonating" button (which hits the same action), still returns a 302.

labkey-adam · 2022-09-13T18:12:41Z

Yep, I noticed that. Not sure if it's returning a different status code now or what.

I just ran it locally. It is getting a 200 response now. The normal "Stop Impersonating" button (which hits the same action), still returns a 302.

Yes, I believe the button causes JavaScript to do a window.location.replace() to redirect. The server is now responding to the API call with that same HTML and 200. I haven't figured out why yet.

labkey-tchad · 2022-09-13T18:44:31Z

Yes, I believe the button causes JavaScript to do a window.location.replace() to redirect. The server is now responding to the API call with that same HTML and 200. I haven't figured out why yet.

Whatever is happening, it seems to be localized to the remote API. StopImpersonatingCommand gets a 200 response even when targeting a 22.7 server; it even gets the same HTML view with window.location.replace. I wonder if the new httpclient is following redirects automatically.

labkey-adam · 2022-09-13T18:47:29Z

Yes, I believe the button causes JavaScript to do a window.location.replace() to redirect. The server is now responding to the API call with that same HTML and 200. I haven't figured out why yet.

Whatever is happening, it seems to be localized to the remote API. StopImpersonatingCommand gets a 200 response even when targeting a 22.7 server; it even gets the same HTML view with window.location.replace. I wonder if the new httpclient is following redirects automatically.

Just stepped through the client code and yes, that's exactly what's happening. Need to ask the HttpClient not to follow redirects or else accept 200 as success.

labkey-tchad

While we're breaking backward compatibility, could we rename Connection.stopImpersonate to stopImpersonating?

labkey-tchad · 2022-09-13T19:24:48Z

labkey-client-api/src/org/labkey/remoteapi/Command.java

+        catch (URISyntaxException e)
        {
            throw new CommandException(e.getMessage());
        }


setParameters is declared as throwing URISyntaxException, why not just let this bubble up instead of wrapping in a CommandException?
Also, I would argue that Command._execute shouldn't wrap it in a CommandException either. Wrapping in an IllegalArgumentException seems more appropriate to me since the most likely cause of such an exception is somebody specifying bad query parameters or some such thing.
Although, I suppose, throwing unchecked exceptions from an API is probably bad practice.

I was just following the existing pattern, which I agree is inconsistent. As I think I noted somewhere, I'm planning to merge the create URL and add parameters steps into a single method at some point. Will look at reconciling exception handling at that point. But I think this set of PRs is close enough and I need to move on...

…heck for success, not redirect.

labkey-adam · 2022-09-13T23:32:45Z

@labkey-tchad as you may have seen, I just added a new StopImpersonatingApiAction that doesn't redirect and shifted the remote API to use it. And I restored the default redirect behavior. We'll see what all the tests think of that...

labkey-tchad · 2022-09-13T23:56:34Z

labkey-client-api/src/org/labkey/remoteapi/security/StopImpersonatingCommand.java

@@ -10,6 +10,6 @@ public class StopImpersonatingCommand extends PostCommand<CommandResponse>
 {
    public StopImpersonatingCommand()
    {
-        super("login", "stopImpersonating.api");
+        super("login", "stopImpersonatingApi.api");


I'm not sure what advantage this has. It seems to just break backward compatibility.

labkey-adam · 2022-09-14T16:55:03Z

Sorry for the delay... was distracted with moving yesterday. Thought is it's cleaner to have our libraries call API actions (more appropriate error and success handling, API-appropriate return values) and I wanted to avoid the pointless redirect. But I hear you on backward compatibility. New plan: I think I've figured out how to disable redirects for this specific Command and will revert to calling the non-API action. But I'll leave the API action in place (and fix the error case) so we can switch to it in the future, e.g., when it's been in the server for a couple years.

labkey-adam · 2022-09-14T17:35:25Z

...which of course means we're back to a 302 response from that Command :)

labkey-adam · 2022-09-14T17:39:25Z

While we're breaking backward compatibility, could we rename Connection.stopImpersonate to stopImpersonating?

I added stopImpersonating() and deprecated stopImpersonate(). We'll leave the old method in place for a while.

…ards compatibility. Disable redirects just for this command. Connection.stopImpersonating()

labkey-jeckels

Looks OK to me

labkey-adam added 3 commits September 6, 2022 18:01

Upgrade Apache HttpClient 4.5.13 -> 5.1.3

331ba51

Label as snapshot version

f745b71

Update change log

d18229f

This was referenced Sep 7, 2022

Upgrade to newest Java remote API version LabKey/server#320

Merged

Upgrade to newest Java remote API version LabKey/testAutomation#1232

Merged

labkey-adam added 3 commits September 12, 2022 23:46

Avoid double encoding of query string

a620284

Add parameters one-by-one to ensure correct escaping. `setCustomQuery…

c464d13

…()` now requires an unescaped query string, which is not compatible with tricky characters (e.g., "&" in a schema name).

Initialize self-signed connection manager only if needed

70d84cb

labkey-adam requested review from labkey-susanh, labkey-jeckels and labkey-tchad September 13, 2022 16:44

labkey-susanh approved these changes Sep 13, 2022

View reviewed changes

labkey-tchad requested changes Sep 13, 2022

View reviewed changes

Don't follow redirects

1f464dc

labkey-tchad approved these changes Sep 13, 2022

View reviewed changes

Restore default redirect following. Use StopImpersonatingApiAction. C…

69f74c9

…heck for success, not redirect.

labkey-tchad reviewed Sep 13, 2022

View reviewed changes

StopImpersonatingCommand: Revert to calling existing action for backw…

07cadc3

…ards compatibility. Disable redirects just for this command. Connection.stopImpersonating()

labkey-jeckels approved these changes Sep 14, 2022

View reviewed changes

Update change log. Increment version for next round of development.

9392fe6

labkey-adam merged commit 6ba3150 into develop Sep 14, 2022

labkey-adam deleted the fb_httpclient branch September 14, 2022 20:59

Upgrade Apache HttpClient 4.5.13 -> 5.1.3 #35

Upgrade Apache HttpClient 4.5.13 -> 5.1.3 #35

Uh oh!

Conversation

labkey-adam commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Uh oh!

labkey-susanh Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

labkey-adam Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

labkey-tchad left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-adam commented Sep 13, 2022

Uh oh!

labkey-tchad commented Sep 13, 2022

Uh oh!

labkey-adam commented Sep 13, 2022

Uh oh!

labkey-tchad commented Sep 13, 2022

Uh oh!

labkey-adam commented Sep 13, 2022

Uh oh!

labkey-tchad left a comment

Choose a reason for hiding this comment

Uh oh!

labkey-tchad Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

labkey-adam Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

labkey-adam commented Sep 13, 2022

Uh oh!

labkey-tchad Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

labkey-adam commented Sep 14, 2022

Uh oh!

labkey-adam commented Sep 14, 2022

Uh oh!

labkey-adam commented Sep 14, 2022

Uh oh!

labkey-jeckels left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

labkey-adam commented Sep 7, 2022 •

edited

Loading