[WIP / Discussion] Add guzzle for proper proxy and SSL support #14913

LukasReschke · 2015-03-16T10:45:20Z

This adds a wrapper around Guzzle to allow developers to perform HTTP requests using a somewhat saner API. Without this SSL and proxy support is basically broken in ownCloud as stated in #14913 (comment)

Todo

Get rid of private API
Deprecate HTTPHelper
Implement SSL in such a way that it actually works……
Follow proxy configured in config.php
Determine how to replace a users' custom trusted root certificate list
Fix unit tests after we agreed on an API design
Ensure that S2S still works properly

Requires owncloud-archive/3rdparty#167

Fixes #9054, #14963, #14840 and a lot of other issues

cc @bantu @nickvergessen @DeepDiver1975

LukasReschke · 2015-03-16T10:50:39Z

Especially a review from an API PoV would be welcome – any feedback welcome 😄

nickvergessen · 2015-03-16T10:54:19Z

lib/private/ocsclient.php


-		if ($xml == false) {
+		if($response->getStatusCode() !== (int)200) {


(int)200 ? 👻

Right. Let me cast the status code at our wrapper level 🙈

nickvergessen · 2015-03-16T11:38:40Z

lib/public/http/client/iresponse.php

+ */
+interface IResponse {
+	/**
+	 * @return \OC\Http\Client\StreamInterface


DeepDiver1975 · 2015-03-16T12:07:07Z

00:01:19.649 PHP Warning:  require(/var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/react/promise/src/functions_include.php): failed to open stream: No such file or directory in /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/composer/autoload_real.php on line 58
00:01:19.649 PHP Stack trace:
00:01:19.649 PHP   1. {main}() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/occ:0
00:01:19.649 PHP   2. require_once() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/occ:11
00:01:19.649 PHP   3. require_once() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/console.php:14
00:01:19.649 PHP   4. OC::init() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/base.php:1031
00:01:19.649 PHP   5. require_once() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/base.php:473
00:01:19.649 PHP   6. ComposerAutoloaderInit45518c348c80881aa936caecc452b13e::getLoader() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/autoload.php:7
00:01:19.649 PHP   7. composerRequire45518c348c80881aa936caecc452b13e() /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/composer/autoload_real.php:49

LukasReschke · 2015-03-16T12:07:43Z

Yes. Jenkins is too stupid again for that 3rdparty repo. Great fun. Let's create again another PR in 3rdparty for that 😠

LukasReschke · 2015-03-16T12:25:10Z

Did some rebasing on this PR and the 3rdparty one… Hopefully that is enough for Jenkins.

LukasReschke · 2015-03-16T12:31:42Z

Apparently not……

ghost · 2015-03-16T13:23:17Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10487/
Test PASSed.

LukasReschke · 2015-03-17T17:33:50Z

I went digging further and discovered:

ownCloud currently implements proxy handling completely wrong and inconsistent.
The SSL verification implemented by ownCloud is also completely wrong and TOTALLY broken

Thus I extended this PR and added proper proxy handling as well as SSL and certificate handling. Also ownCloud will now ship a list of root certificates to finally get rid of bug reports by those annoying users and enterprise distributions using 10 year old stuff (because the older the more secure!).
Regardless of that: The current approach will append the default root certificates to the users custom rootcerts.crt, however, after an update the shipped bundle can change. How can we regenerate this bundle for every user the easiest? – IMHO a "loop over all directories in data and regenerate the certificates if there is a folder files_external would be the easiest approach.

Basically this means that if we merge this ownCloud 8.1 will be the first release that properly handles proxies and SSL………

cc @icewind1991 For input
@DeepDiver1975 FYI

LukasReschke · 2015-03-17T17:47:02Z

That said I'd really LOVE to have this in 8.1 before we have to wait yet again 4 months to have proper SSL and proxy support… Pretty annoying and would solve a lot of problems reported by users and customers…

LukasReschke · 2015-03-17T17:48:31Z

For discussion regarding API design:

@MorrisJobke @DeepDiver1975 @PVince81 @th3fallen @Raydiation @icewind1991 @karlitschek @nickvergessen @bantu

LukasReschke · 2015-03-17T17:52:55Z

Will fix the failing tests (basically adjusting the constructor in the tests etc…) once we have agreed on an API design.

th3fallen · 2015-03-17T19:21:32Z

🙌 Guzzle ftw

nickvergessen · 2015-03-18T08:17:58Z

lib/private/server.php

@@ -651,7 +661,7 @@ function getDb() {

 	/**
 	 * Returns an instance of the HTTP helper class
-	 *
+	 * @deprecated Use \OCP\Http\Client\IClientService


"Use getHTTPClientService()"

nickvergessen · 2015-03-18T08:18:33Z

Well I guess it makes sense like that

nickvergessen · 2015-03-20T11:46:28Z

Needs rebase

LukasReschke · 2015-03-25T15:23:54Z

@owncloud-bot Retest this please.

scrutinizer-notifier · 2015-03-25T15:32:06Z

The inspection completed: 20 new issues, 43 updated code elements

LukasReschke added the 3 - To Review label Mar 16, 2015

LukasReschke mentioned this pull request Mar 16, 2015

Check if app archive is correctly downloaded and unpacked #14869

Closed

nickvergessen reviewed Mar 16, 2015
View reviewed changes

LukasReschke force-pushed the add-guzzle branch from be05f29 to 68237a4 Compare March 16, 2015 11:37

LukasReschke added the discussion label Mar 16, 2015

nickvergessen reviewed Mar 16, 2015
View reviewed changes

lib/public/http/client/iresponse.php

*/

interface IResponse {

/**

* @return \OC\Http\Client\StreamInterface

Copy link

Contributor

nickvergessen Mar 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OCP?

LukasReschke changed the title ~~Add guzzle~~ [WIP / Discussion] Add guzzle Mar 16, 2015

LukasReschke force-pushed the add-guzzle branch from 68237a4 to 6e0a55b Compare March 16, 2015 12:24

LukasReschke force-pushed the add-guzzle branch from 586d70c to 94e856b Compare March 16, 2015 12:55

LukasReschke mentioned this pull request Mar 16, 2015

Add guzzle owncloud-archive/3rdparty#167

Merged

LukasReschke force-pushed the add-guzzle branch from 94e856b to 5930337 Compare March 17, 2015 13:25

LukasReschke added 2 - Developing and removed 3 - To Review labels Mar 17, 2015

LukasReschke mentioned this pull request Mar 17, 2015

Add to your owncloud button / function #14858

Closed

LukasReschke force-pushed the add-guzzle branch from 5930337 to 3326eb0 Compare March 17, 2015 17:26

LukasReschke force-pushed the add-guzzle branch from 3326eb0 to f71afc0 Compare March 17, 2015 17:35

LukasReschke added this to the 8.1-current milestone Mar 17, 2015

LukasReschke added Type:Bug security enhancement technical debt labels Mar 17, 2015

LukasReschke changed the title ~~[WIP / Discussion] Add guzzle~~ [WIP / Discussion] Add guzzle for proper proxy and SSL support Mar 17, 2015

LukasReschke changed the title ~~[WIP / Discussion] Add guzzle for proper proxy and SSL support~~ [WIP / Discussion] Add guzzle for proper proxy and SSL support Mar 17, 2015

LukasReschke mentioned this pull request Mar 17, 2015

"App does not provide an info.xml file" After fresh installation of OC 8.0.2 #14963

Closed

nickvergessen reviewed Mar 18, 2015
View reviewed changes

LukasReschke force-pushed the add-guzzle branch from f71afc0 to 12144cd Compare March 18, 2015 14:52

LukasReschke force-pushed the add-guzzle branch 2 times, most recently from 162f9e4 to a2cc98a Compare March 23, 2015 14:55

Add wrapper for Guzzle

5f044eb

LukasReschke force-pushed the add-guzzle branch from a2cc98a to 5f044eb Compare March 25, 2015 15:04

LukasReschke closed this Mar 25, 2015

LukasReschke deleted the add-guzzle branch March 25, 2015 15:30

LukasReschke mentioned this pull request Mar 25, 2015

Add wrapper for Guzzle #15195

Merged

7 tasks

MorrisJobke removed this from the 8.1-current milestone Mar 25, 2015

LukasReschke mentioned this pull request Apr 7, 2015

OC 8.0: s2s does not recognise certificates from personal store (fix included) #14790

Closed

oparoz mentioned this pull request Apr 17, 2015

Let the user decide if he wants to load images shared from other clouds owncloud/gallery#124

Closed

LukasReschke mentioned this pull request Aug 16, 2015

[Feature request] Federated cloud share self signed certificate notification #18328

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP / Discussion] Add guzzle for proper proxy and SSL support #14913

[WIP / Discussion] Add guzzle for proper proxy and SSL support #14913

LukasReschke commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

nickvergessen Mar 16, 2015

LukasReschke Mar 16, 2015

nickvergessen Mar 16, 2015

DeepDiver1975 commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

ghost commented Mar 16, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

th3fallen commented Mar 17, 2015

nickvergessen Mar 18, 2015

nickvergessen commented Mar 18, 2015

nickvergessen commented Mar 20, 2015

LukasReschke commented Mar 25, 2015

scrutinizer-notifier commented Mar 25, 2015


		if ($xml == false) {
		if($response->getStatusCode() !== (int)200) {

[WIP / Discussion] Add guzzle for proper proxy and SSL support #14913

[WIP / Discussion] Add guzzle for proper proxy and SSL support #14913

Conversation

LukasReschke commented Mar 16, 2015

Todo

LukasReschke commented Mar 16, 2015

nickvergessen Mar 16, 2015

Choose a reason for hiding this comment

LukasReschke Mar 16, 2015

Choose a reason for hiding this comment

nickvergessen Mar 16, 2015

Choose a reason for hiding this comment

DeepDiver1975 commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

LukasReschke commented Mar 16, 2015

ghost commented Mar 16, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

LukasReschke commented Mar 17, 2015

th3fallen commented Mar 17, 2015

nickvergessen Mar 18, 2015

Choose a reason for hiding this comment

nickvergessen commented Mar 18, 2015

nickvergessen commented Mar 20, 2015

LukasReschke commented Mar 25, 2015

scrutinizer-notifier commented Mar 25, 2015