[OAuth2] Replace GET http verb for PROPFIND on DetermineAuthTypeJob

[SamuAlfageme]

Note that this issue is highly dependent on the server configuration (i.e. firewalls, extra security layers...) and does not affect most of OAuth2 server setups.

On account creation wizard, to determine the authentication type, the client performs a dummy (GET) request to the webdav endpoint where it checks the WWW-Authenticate headers on the response. However It seems that some firewalls are picky on which verbs can reach that endpoint and only answers what we expect if it gets hit by a PROPFIND. - this seems to be the original problem behind the behavior I saw on #5954 (comment)

Expected behavior

As an illustrative example, here's the cURL version of the request the iOS client does right after checking status.php (as said, it's a "dummy" req. since we expect the server to reply w/ a 401 and the body will be ignored; the important part would be the HTTP method)

curl \
	-H 'Host:<host>' \
	-H 'Content-Type:application/xml' \
	-H 'Depth:0' \
	-H 'Connection:keep-alive' \
	-H 'Accept:*/*' \
	-H 'User-Agent:Mozilla/5.0 (iOS) ownCloud-iOS/3.7.0' \
	-H 'Content-Length:383' \
	-H 'Accept-Language:en-us' \
	-H 'Accept-Encoding:gzip, deflate' \
	-X PROPFIND 'https://<host>/remote.php/webdav/' \
	--data-binary '<?xml version="1.0" encoding="UTF-8"?>
        <d:propfind xmlns:d="DAV:">
        <d:prop>
            <d:resourcetype />
            <d:getlastmodified />
            <size xmlns="http://owncloud.org/ns" />
            <d:creationdate />
            <id xmlns="http://owncloud.org/ns" />
            <d:getcontentlength />
            <d:displayname />
            <d:quota-available-bytes />
            <d:getetag />
            <permissions xmlns="http://owncloud.org/ns" />
            <d:quota-used-bytes />
            <d:getcontenttype />
        </d:prop>
        </d:propfind>'

Actual behavior

curl \
	-H 'User-Agent:Mozilla/5.0 (Macintosh) mirall/2.5.0-nightly20171030 (build 8517) (testpilotcloud)' \
	-H 'Accept:*/*' \
	-H 'X-Request-ID:bcbfb8bd-d160-4999-9e93-e6c34c7bebca' \
	-H 'Connection:Keep-Alive' \
	-H 'Accept-Encoding:gzip, deflate' \
	-H 'Accept-Language:en-US,*' \
	-H 'Host:<host>' 'https://<host>/remote.php/webdav/'

Coming from:

client/src/libsync/networkjobs.cpp

Lines 881 to 893 in 05c1bfb

	void DetermineAuthTypeJob::send(const QUrl &url)
	{
	QNetworkRequest req;
	// Prevent HttpCredentialsAccessManager from setting an Authorization header.
	req.setAttribute(HttpCredentials::DontAddCredentialsAttribute, true);
	// Don't reuse previous auth credentials
	req.setAttribute(QNetworkRequest::AuthenticationReuseAttribute, QNetworkRequest::Manual);
	// Don't send cookies, we can't determine the auth type if we're logged in
	req.setAttribute(QNetworkRequest::CookieLoadControlAttribute, QNetworkRequest::Manual);
	sendRequest("GET", url, req);
	}

cc/ @ogoffart

[michaelstingl]

@SamuAlfageme needs documentation for use in complex scenarios. Maybe https://github.com/owncloud/oauth2/wiki/OAuth-code-Flow-Sequence-Diagram or owncloud-archive/documentation#3456

[ckamm]

This is a regression in 2.4. Previously we assumed Basic auth if that request failed in some way. If we used Basic auth when the auth type can't be determined we'd restore previous behavior. It would not be correct though: In 2.4 we need the request to succeed to be able to tell whether to use Basic or OAuth.

DetermineAuthTypeJob also must be able to detect servers using shibboleth for auth. It currently does that by checking whether the GET is redirected in a particular way. Simply changing GET to PROPFIND is unsafe since we can't guarantee (or can we?) that the same redirect pattern will hold for the changed request.

What could be safely done is to GET first to check for shibboleth and then - if there's no shib redirect - follow up with a PROPFIND to determine if it's OAuth or Basic auth.

[SamuAlfageme]

@ckamm about:

Simply changing GET to PROPFIND is unsafe since we can't guarantee (or can we?) that the same redirect pattern will hold for the changed request.

I think that behavior should be preserved regardless of the request verb. Thinking about SAML-wrapped-in-OAuth scenario. @butonic whatcha' think?

What could be safely done is to GET first to check for shibboleth and then - if there's no shib redirect - follow up with a PROPFIND to determine if it's OAuth or Basic auth.

The thing is, OAuth flow must have higher preference than SAML (to abstract it), if we check first for Shibboleth redirection(s), the client will open the webview and fall in that authentication flow instead of the wrapped one.

[ckamm]

The thing is, OAuth flow must have higher preference than SAML

That an important aspect I wasn't aware of yet! In the PR I run a PROPFIND and a GET in parallel. Adjusting it to give OAuth preference will be straightforward.

[ckamm]

@SamuAlfageme The PR is updated to account for that. Now I'm wondering whether we should be able to upgrade shibboleth accounts to use oauth? Currently we allow switching between basic and oauth depending on what the server supports, but shib accounts will always use shibboleth.

[SamuAlfageme]

@ckamm hm, for now I guess this is more than enough. We had similar concerns for the "basic to OAuth" upgrade in #5848 (comment) (also see owncloud/core#29300 & owncloud/core#29324 for an account-type "forced migration")

Since one of the long term plans after supporting OAuth2 in the client is to drop the QtWebkit dependency (and therefore shibboleth accounts), I'd say this would happen sooner or later, so might as well be in the near future.

cc/ @michaelstingl @pmaier1 @butonic for their POV

[SamuAlfageme]

Closing here as confirmed working with one of the picky-firewall setups; @ckamm's concerns in #6135 (comment)

Now I'm wondering whether we should be able to upgrade shibboleth accounts to use oauth? Currently we allow switching between basic and oauth depending on what the server supports, but shib accounts will always use shibboleth.

... moved to #6198