[squeak-dev] The Inbox: WebClient-Core-ct.126.mcz

Thiede, Christoph Christoph.Thiede at student.hpi.uni-potsdam.de
Mon Oct 12 18:47:25 UTC 2020


For the case we want to make pre-authentication the default behavior, the attached changeset activates this.


What about tests? Do you see the need to add some for this patch? :-)


Best,

Christoph

<http://www.hpi.de/>
________________________________
Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von commits at source.squeak.org <commits at source.squeak.org>
Gesendet: Montag, 12. Oktober 2020 19:05:34
An: squeak-dev at lists.squeakfoundation.org
Betreff: [squeak-dev] The Inbox: WebClient-Core-ct.126.mcz

A new version of WebClient-Core was added to project The Inbox:
http://source.squeak.org/inbox/WebClient-Core-ct.126.mcz

==================== Summary ====================

Name: WebClient-Core-ct.126
Author: ct
Time: 12 October 2020, 7:05:32.190311 pm
UUID: a64ca560-814a-f940-8f64-e66a25cedc61
Ancestors: WebClient-Core-ul.123

Proposal to implement pre-authentication on WebClient.

MOTIVATION.
Until now, the authentication flow in Squeak's WebClient looks like this:
First, a request is made without trying to authenticate the user. If the request fails with an error 401 (Unauthorized) or an error 407 (Proxy Authentication Required), the authentication headers are added to the request, and the request is retried.

However, this does not work properly in some situations.
For example, many modern REST APIs use to return an error 404 if an attempt is made to access a private resource without authenticating before [1] which currrently makes it impossible to authenticate to these APIs using the WebClient.
Another issue I encountered today lies in some particular servers not requesting a specific authentication method via the WWW-Authenticate header along a 401 response as specified by the protocol [2]. Concretely, I encountered this problem with the quite popular GitHub API so I think our client should be robust enough to handle this contract violation.

APPROACH.
This patch adds a new property for the #preAuthenticationMethod to the WebClient class. It can be set to a symbol indicating any authentication method that is supported by the WebClient, e.g. #basic or #bearer. (Digest access authentication, however, cannot be used at this place because it depends on a realm specified by the server.)
If this property is set, the relevant authentication headers will be added to the request already before the first attempt is made to request the resource.

In addition, the patch refactors and reformatst the methods #authenticate:from: and #sendRequest:contentBlock:.

The TESTS work as well as always (a number of them failing sporadically, but after some trials, I get a green bar again).

WHAT REMAINS TO BE DONE.
I deleted the fixme "Pre-authenticate the request if we have valid auth credentials" comment which I think was exactly what I implemented in this patch. I hope this assumption was correct? Also, another fixme comment requests to preserve the authState after following a redirect. Instead, with this patch, any specified pre-authentication method will be reused after every redirect. I did not fix this because I do not have a use-case scenario for it. Can we leave this as-is, and in a future version, could we simply delete this send to #flushAuthState?

Also, I'm not sure about whether pre-authentication maybe should be the default for every request containing a username/password specification. This would speed up every web request that uses credentials by up to the factor 2 because we could save one futile query. Also, it appears to be the state-of-the-art solution, popular tools such as curl specify the credentials in the first run already. On the contrary, it would be a breaking change, and looking at the comment in #preAuthenticationMethod, pre-authentication as an opt-out feature might break or at least slow down NTLM/Negotiate use cases. However, I never heard of this before. Are these protocols still relevant at all?

Please give this patch a careful review because still, all knowledge I have about this domain is collected from StackOverflow and Wikipedia a few hours ago.

REFERENCES.
[1] https://stackoverflow.com/a/17688080/13994294
[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401

=============== Diff against WebClient-Core-ul.123 ===============

Item was changed:
  Object subclass: #WebClient
+        instanceVariableNames: 'flags server scheme timeout stream cookies proxyServer lastScheme lastServer lastPort maxRedirect redirections userAgent authParams proxyParams accessLog debugLog preAuthenticationMethod'
-        instanceVariableNames: 'flags server scheme timeout stream cookies proxyServer lastScheme lastServer lastPort maxRedirect redirections userAgent authParams proxyParams accessLog debugLog'
         classVariableNames: 'DebugLog FlagAcceptCookies FlagAllowAuth FlagAllowRedirect ProxyHandler'
         poolDictionaries: ''
         category: 'WebClient-Core'!

  !WebClient commentStamp: 'ar 5/4/2010 13:17' prior: 0!
  WebClient provides a simple yet complete HTTP client implementation.

  To view the documentation evaluate:

         HelpBrowser openOn: WebClientHelp.
  !

Item was added:
+ ----- Method: WebClient>>authProcess:from:header:params: (in category 'authentication') -----
+ authProcess: request from: response header: authHeader params: params
+        "Process an authentication header.
+        Answer true if an authentication response could be generated; otherwise, false."
+
+        self
+                authDispatch: request
+                from: response
+                header: authHeader
+                params: params.
+
+        params at: #authResponse ifAbsent: [^ false].
+
+        "If we generated an authentication response for the header use it"
+        request
+                headerAt: ((response ifNotNil: [response code = 401] ifNil: [true])
+                        ifTrue: ['Authorization']
+                        ifFalse: ['Proxy-Authorization'])
+                put: (params at: #authMethod), ' ', (params at: #authResponse).
+
+        ^ true!

Item was changed:
  ----- Method: WebClient>>authenticate:from: (in category 'sending') -----
  authenticate: request from: response
         "Authenticate after having received a 401/407 response.
         Returns true if we should retry, false if we fail here."

+        | headers params |
+
-        "NOTE: The first time through we do NOT ask for credentials right away.
-        Some authentication mechanisms (NTLM/Negotiate) can use the credentials
-        of the currently logged on user. Consequently we only ask for credentials
-        if we're unable to do so without asking. Methods that require credentials
-        (basic, digest) test for their existence explicitly."
-
-        | headers authHeader params |
-
         "Pick the right set of parameters"
+        response code = 401
+                ifTrue: [
+                        params := authParams.
+                        headers := response headersAt: 'WWW-Authenticate'.
+                        "If the connection was closed, we need to flush the
+                        proxy params or we won't pick up prior credentials."
+                        self isConnected
+                                ifFalse: [self flushAuthState: proxyParams]]
+                ifFalse: [
+                        params := proxyParams.
+                        headers := response headersAt: 'Proxy-Authenticate'].
+
-        response code = 401 ifTrue:[
-                params := authParams.
-                headers := response headersAt: 'WWW-Authenticate'.
-                "If the connection was closed, we need to flush the
-                proxy params or we won't pick up prior credentials."
-                self isConnected
-                        ifFalse:[self flushAuthState: proxyParams].
-        ] ifFalse:[
-                params := proxyParams.
-                headers := response headersAt: 'Proxy-Authenticate'.
-        ].
-
         "Remove any old response"
+        params removeKey: #authResponse ifAbsent: [].
+
-        params removeKey: #authResponse ifAbsent:[].
-
         "Process the authentication header(s)"
+        headers
+                detect: [:authHeader |
+                        self
+                                authProcess: request
+                                from: response
+                                header: authHeader
+                                params: params]
+                ifFound: [:authHeader | ^ true].
+
+        "If we fall through here this can have two reasons: One is that we don't have a suitable authentication method. Check for that first."
+        params at: #authMethod ifAbsent: [^ false].
+
+        "The other possibility is that the credentials are wrong. Clean out the previous auth state and go ask for credentials."
-        1 to: headers size do:[:i|
-                authHeader := headers at: i.
-                self authDispatch: request from: response header: authHeader params: params.
-                "If we generated an authentication response for the header use it"
-                params at: #authResponse ifPresent:[:resp|
-                        request headerAt: (response code = 401
-                                                                ifTrue:['Authorization']
-                                                                ifFalse:['Proxy-Authorization'])
-                                        put: (params at: #authMethod), ' ', resp.
-                        ^true].
-        ].
-
-        "If we fall through here this can have two reasons: One is that we don't have
-        a suitable authentication method. Check for that first."
-        params at: #authMethod ifAbsent:[^false].
-
-        "The other possibility is that the credentials are wrong.
-        Clean out the previous auth state and go ask for credentials."
         self flushAuthState: params.
+
-
         "Clean out old authentication headers"
         response code = 401
+                ifTrue: [request removeHeader: 'Authorization'].
-                ifTrue:[request removeHeader: 'Authorization'].
         "Always clean out the proxy auth header since we don't support pre-authentication"
         request removeHeader: 'Proxy-Authorization'.
+
-
         "Signal WebAuthRequired"
         (WebAuthRequired client: self request: request response: response)
+                signal == true ifFalse: [^ false].
+
-                signal == true ifFalse:[^false].
-
         "And retry with the new credentials"
+        ^ self authenticate: request from: response!
-        ^self authenticate: request from: response!

Item was added:
+ ----- Method: WebClient>>preAuthenticationMethod (in category 'accessing') -----
+ preAuthenticationMethod
+        "The authentication method to be used for initial requests. Symbol, e.g. #basic or #bearer. If nil, no authentication will be used until the server requests an authentication.
+
+        NOTE: Some authentication mechanisms (NTLM/Negotiate) can use the credentials of the currently logged on user. Consequently, by default we only ask for credentials if we're unable to do so without asking."
+
+        ^ preAuthenticationMethod!

Item was added:
+ ----- Method: WebClient>>preAuthenticationMethod: (in category 'accessing') -----
+ preAuthenticationMethod: aSymbol
+        "The authentication method to be used for initial requests. See #preAuthenticationMethod."
+
+        preAuthenticationMethod := aSymbol!

Item was changed:
  ----- Method: WebClient>>sendRequest:contentBlock: (in category 'sending') -----
  sendRequest: request contentBlock: contentBlock
         "Send an http request"

         |  response repeatRedirect repeatAuth |
-
-        "XXXX: Fixme. Pre-authenticate the request if we have valid auth credentials"
-
         redirections := Dictionary new.

         ["The outer loop handles redirections"
+                repeatRedirect := false.
+
+                "Always update the host header due to redirect"
+                request headerAt: 'Host' put: server.
+
+                self preAuthenticationMethod ifNotNil: [:authMethod |
+                        self
+                                authProcess: request
+                                from: nil
+                                header: authMethod asString capitalized
+                                params: authParams].
+
-        repeatRedirect := false.
-
-        "Always update the host header due to redirect"
-        request headerAt: 'Host' put: server.
-
                 ["The inner loop handles authentication"
+                        repeatAuth := false.
+
+                        "Connect can fail if SSL proxy CONNECT is involved"
+                        self connect ifNotNil: [:resp| ^ resp].
+
+                        "Write the request to the debugLog if present"
+                        debugLog ifNotNil: [self writeRequest: request on: debugLog].
+
+                        "Send the request itself"
+                        self writeRequest: request on: stream.
+                        contentBlock value: stream.
+
+                        response := request newResponse readFrom: stream.
+                        response url: scheme, '://', server, request rawUrl.
+
+                        debugLog ifNotNil: [
+                                response writeOn: debugLog.
+                                debugLog flush].
+                        response setCookiesDo: [:cookie|
+                                self acceptCookie: cookie host: self serverUrlName path: request url].
+                        accessLog ifNotNil: [
+                                WebUtils logRequest: request response: response on: accessLog].
+                        "Handle authentication if needed"
+                        (self allowAuth and: [response code = 401 or: [response code = 407]]) ifTrue: [
+                                "Eat up the content of the previous response"
+                                response content.
+                                repeatAuth := self authenticate: request from: response].
+
+                        repeatAuth
+                ] whileTrue.
-                repeatAuth := false.
-
-                "Connect can fail if SSL proxy CONNECT is involved"
-                self connect ifNotNil:[:resp| ^resp].

+                "Flush previous authState.
+                XXXX: Fixme. authState must be preserved for pre-authentication of requests."
+                self flushAuthState.
+
+                "Handle redirect if needed"
+                (self allowRedirect and: [response isRedirect]) ifTrue:[
-                "Write the request to the debugLog if present"
-                debugLog ifNotNil:[self writeRequest: request on: debugLog].
-
-                "Send the request itself"
-                self writeRequest: request on: stream.
-                contentBlock value: stream.
-
-                response := request newResponse readFrom: stream.
-                response url: (scheme, '://', server, request rawUrl).
-
-                debugLog ifNotNil:[
-                        response writeOn: debugLog.
-                        debugLog flush.
-                ].
-                response setCookiesDo:[:cookie|
-                        self acceptCookie: cookie host: self serverUrlName path: request url.
-                ].
-                accessLog ifNotNil:[
-                        WebUtils logRequest: request response: response on: accessLog
-                ].
-                "Handle authentication if needed"
-                (self allowAuth and:[response code = 401 or:[response code = 407]]) ifTrue:[
                         "Eat up the content of the previous response"
                         response content.
+                        repeatRedirect := self redirect: request from: response].
+                repeatRedirect
+        ] whileTrue: [
-                        repeatAuth := self authenticate: request from: response.
-                ].
-
-                repeatAuth] whileTrue.
-
-        "Flush previous authState.
-        XXXX: Fixme. authState must be preserved for pre-authentication of requests."
-        self flushAuthState.
-
-        "Handle redirect if needed"
-        (self allowRedirect and:[response isRedirect]) ifTrue:[
-                "Eat up the content of the previous response"
-                response content.
-                repeatRedirect := self redirect: request from: response.
-        ].
-        repeatRedirect] whileTrue:[
                 "When redirecting, remove authentication headers"
                 request removeHeader: 'Authorization'.
                 request removeHeader: 'Proxy-Authorization'.
         ].
+
-
         "If the response is not a success, eat up its content"
+        (response isSuccess or: [response isInformational]) ifFalse: [
+                response content].
+
+        ^ response!
-        (response isSuccess or:[response isInformational]) ifFalse:[response content].
-
-        ^response!


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20201012/c2999c36/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: WebClient-preauth-default.1.cs
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20201012/c2999c36/attachment.ksh>


More information about the Squeak-dev mailing list