Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't try to parse body if HTTP status is not 200 #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 8 additions & 35 deletions auth.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
package winrm

import (
"crypto/tls"
"fmt"
"io/ioutil"
"net"
"net/http"
"strings"
"time"

"github.com/masterzen/azure-sdk-for-go/core/http"
Copy link
Owner

Choose a reason for hiding this comment

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

Removing this means we're breaking the compatibility with Go < 1.8.
But maybe we don't care anymore nowadays...

"github.com/masterzen/azure-sdk-for-go/core/tls"

"github.com/masterzen/winrm/soap"
)

Expand Down Expand Up @@ -50,28 +48,6 @@ func (c *ClientAuthRequest) Transport(endpoint *Endpoint) error {
return nil
}

// parse func reads the response body and return it as a string
func parse(response *http.Response) (string, error) {

// if we recived the content we expected
if strings.Contains(response.Header.Get("Content-Type"), "application/soap+xml") {
body, err := ioutil.ReadAll(response.Body)
defer func() {
// defer can modify the returned value before
// it is actually passed to the calling statement
if errClose := response.Body.Close(); errClose != nil && err == nil {
err = errClose
}
}()
if err != nil {
return "", fmt.Errorf("error while reading request body %s", err)
}

return string(body), nil
}

return "", fmt.Errorf("invalid content type")
}

func (c ClientAuthRequest) Post(client *Client, request *soap.SoapMessage) (string, error) {
httpClient := &http.Client{Transport: c.transport}
Expand All @@ -89,18 +65,15 @@ func (c ClientAuthRequest) Post(client *Client, request *soap.SoapMessage) (stri
return "", fmt.Errorf("unknown error %s", err)
}

body, err := parse(resp)
// error in case on incorrect exit code
if resp.StatusCode != 200 {
return "", fmt.Errorf("http unexpected status: %s", resp.Status)
}

body, err := ParseSoapResponse(resp)
if err != nil {
return "", fmt.Errorf("http response error: %d - %s", resp.StatusCode, err.Error())
}

// if we have different 200 http status code
// we must replace the error
defer func() {
if resp.StatusCode != 200 {
body, err = "", fmt.Errorf("http error %d: %s", resp.StatusCode, body)
}
}()

return body, err
}
28 changes: 13 additions & 15 deletions http.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,23 @@ package winrm
import (
"crypto/tls"
"fmt"
"io/ioutil"
"net"
"net/http"
"strings"
"time"

"github.com/masterzen/winrm/soap"
"io/ioutil"
)

var soapXML = "application/soap+xml"

// body func reads the response body and return it as a string
func body(response *http.Response) (string, error) {
// parse func reads the response body and return it as a string
func ParseSoapResponse(response *http.Response) (string, error) {

// if we recived the content we expected
if strings.Contains(response.Header.Get("Content-Type"), "application/soap+xml") {
contentType := response.Header.Get("Content-Type")
// if we received the content we expected
if strings.Contains(contentType, soapXML) {
body, err := ioutil.ReadAll(response.Body)
defer func() {
// defer can modify the returned value before
Expand All @@ -34,7 +35,7 @@ func body(response *http.Response) (string, error) {
return string(body), nil
}

return "", fmt.Errorf("invalid content type")
return "", fmt.Errorf("invalid content type: %s", contentType)
}

type clientRequest struct {
Expand Down Expand Up @@ -84,18 +85,15 @@ func (c clientRequest) Post(client *Client, request *soap.SoapMessage) (string,
return "", fmt.Errorf("unknown error %s", err)
}

body, err := body(resp)
// error in case of incorrect exit code
if resp.StatusCode != 200 {
return "", fmt.Errorf("http unexpected status: %s", resp.Status)
Copy link
Owner

Choose a reason for hiding this comment

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

The problem here is that if you get an operation timeout (which is also an HTTP 500 error), you'll return an error here that the upper layer will not "ignore" anymore (see command.go:134 and you'll break long running commands.

Note that OperationTimeout are SOAP errors, so they need to be parsed.
That's the reason your code breaks the TestOperationTimeoutSupport test (the test is designed to show that getting a timeout doesn't mean the command is aborted, it still runs on the server until there are more output to display).

So, I think we first have to check if the body is a soap+xml, then error as before, but we also have to carry over correctly the error message if it is a straight http error (and not a soap+xml error).

}

body, err := ParseSoapResponse(resp)
if err != nil {
return "", fmt.Errorf("http response error: %d - %s", resp.StatusCode, err.Error())
}

// if we have different 200 http status code
// we must replace the error
defer func() {
if resp.StatusCode != 200 {
body, err = "", fmt.Errorf("http error %d: %s", resp.StatusCode, body)
}
}()

return body, err
}