Handle SAML enforcement challenge from the server#5054
Conversation
Whenever a SSO challenge gets issued by the server by means of a URL in the `X-GitHub-SSO` response header, gh now additionally prints that URL on the standard error stream. This is achieved by installing a middleware to all HTTP requests and storing the server challenge to a value accessible by `factory.SSOURL()`. Such approach was made necessary mainly because of the `shurcool-graphql` client which doesn't give access to response headers when a GraphQL error case is encountered.
| return api.NewHTTPClient(opts...), nil | ||
| } | ||
|
|
||
| var ssoHeader string |
There was a problem hiding this comment.
Keeping this state around from the request feels a bit off. I am wondering if we could actually do something similar to what we do with scope suggestions and modify the http response error message to indicate that it was a SSO auth challenge error and what the SSO URL is?
There was a problem hiding this comment.
Sure, it's not the most elegant solution, but the shurcool-graphql library does not allow access to HTTP response headers once it has parsed the GraphQL error information. Additionally, its errors type is an unexported type. Thus, we cannot decorate its error object with this information.
There was a problem hiding this comment.
That makes sense. The current approach seems appropriate then. Also it looks like this was explained in the PR body which I didn't read closely enough, sorry about that.
pkg/cmd/factory/http.go
Outdated
| } | ||
|
|
||
| var ssoHeader string | ||
| var ssoURLRE = regexp.MustCompile(`\burl=(\S+)`) |
There was a problem hiding this comment.
Considering the possibility of adding values after the url, wouldn't this regular expression be better? 😃
\burl=([^;]+)
There was a problem hiding this comment.
That's a good thought, but I wonder whether a literal ; would ever be present in the URL? Since GitHub uses & for param delimiter, I would guess that it's unlikely that we ever see a literal ;, in which case your proposed regex would be more future-proof.
There was a problem hiding this comment.
Sorry, I was not concerned about the url param, but about the value being added to X-GitHub-SSO.
But if there are no such changes in the future, it seems to be fine. 😃
Whenever an SSO challenge gets issued by the server by means of a URL in the
X-GitHub-SSOresponse header, gh now additionally prints that URL on the standard error stream. Users will typically be presented with this challenge if they attempt to use a Personal Access Token that wasn't authorized to view resources in their org.Printing of the challenge URL is achieved by installing a middleware to all HTTP requests and storing the server challenge to a value accessible by
factory.SSOURL(). Such heavy-handed approach was made necessary mainly because of theshurcool-graphqlclient which doesn't give access to response headers when a GraphQL error case is encountered.The URL is printed to stderr even when using
gh api. (Previously,gh apiconsumers would have to usegh api --includeto manually scan for theX-GitHub-SSOresponse header.)Fixes #5002, followup to #4241