-
Notifications
You must be signed in to change notification settings - Fork 566
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
fix: align auth failure error messages for oauth #5346
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |
"context" | ||
"crypto/tls" | ||
"crypto/x509" | ||
"errors" | ||
"fmt" | ||
"log" | ||
"net" | ||
|
@@ -18,6 +19,7 @@ import ( | |
pkg_utils "github.com/snyk/go-application-framework/pkg/utils" | ||
|
||
"github.com/snyk/go-application-framework/pkg/networking/certs" | ||
"github.com/snyk/go-application-framework/pkg/networking/middleware" | ||
"github.com/snyk/go-httpauth/pkg/httpauth" | ||
|
||
"github.com/snyk/cli/cliv2/internal/constants" | ||
|
@@ -143,9 +145,28 @@ func (p *WrapperProxy) ProxyInfo() *ProxyInfo { | |
} | ||
} | ||
|
||
// headerSnykAuthFailed is used to indicate there was a failure to establish | ||
// authorization in a legacycli proxied HTTP request and response. | ||
// | ||
// The request header is used to propagate this indication from | ||
// NetworkAccess.AddHeaders all the way through proxy middleware into the | ||
// response. | ||
// | ||
// The response header is then used by the Typescript CLI to surface an | ||
// appropriate authentication failure error back to the user. | ||
// | ||
// These layers of indirection are necessary because the Typescript CLI is not | ||
// involved in OAuth authentication at all, but needs to know that an auth | ||
// failure specifically occurred. HTTP status and error catalog codes aren't | ||
// adequate for this purpose because there are non-authentication reasons an API | ||
// request might 401 or 403, such as permissions or entitlements. | ||
const headerSnykAuthFailed = "snyk-auth-failed" | ||
|
||
func (p *WrapperProxy) replaceVersionHandler(r *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { | ||
err := p.addHeaderFunc(r) | ||
if err != nil { | ||
if err := p.addHeaderFunc(r); err != nil { | ||
if errors.Is(err, middleware.ErrAuthenticationFailed) { | ||
r.Header.Set(headerSnykAuthFailed, "true") | ||
} | ||
p.DebugLogger.Printf("Failed to add header: %s", err) | ||
} | ||
|
||
|
@@ -177,6 +198,12 @@ func (p *WrapperProxy) Start() error { | |
proxy.Logger = log.New(&pkg_utils.ToZeroLogDebug{Logger: p.DebugLogger}, "", 0) | ||
proxy.OnRequest().DoFunc(p.replaceVersionHandler) | ||
proxy.OnRequest().HandleConnect(p) | ||
proxy.OnResponse().DoFunc(func(resp *http.Response, ctx *goproxy.ProxyCtx) *http.Response { | ||
if authFailed := resp.Request.Header.Get(headerSnykAuthFailed); authFailed != "" { | ||
resp.Header.Set(headerSnykAuthFailed, authFailed) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why can't we just print the message here? Instead of delegating this to the TS CLI. This way we could get to a consistent error written in the Extensible CLI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So basically what I'm wondering is, if we can't handle auth errors transparently to the TS CLI, similar to how we handle auth in general. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry about inter-process complexities with such an approach, at least while we're still dispatching command execution and console interactions entirely to the TS CLI. Decomposing console interactions to the Go CLI and treating the TS CLI like a library would mitigate this risk, but that'll take followups beyond the scope of this PR. If we did error out here in the middleware, I think we'd have to terminate the node subprocess immediately to prevent its output from getting mixed up with that of the parent process. I worry about the assumptions we'd be making about the order of operations scheduled in node's event loop, as well as what's been buffered for output in that process, how it handles signals, and resource cleanup. Things can get tricky with inter-process coordination. It might be fine in the typical case, but hard to debug and reproduce edge cases which could be timing or platform-dependent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the points and I should probably take one step back from proposing a solution to proposing/aligning on the goal. My goal is to achieve a consistent behaviour for authentication failures throughout the CLI, no matter if the domain logic is implemented in TS or Golang and in a not too distant future for LS as well. Some non-functional requirements for a solution would be, Single source of Truth and KISS (Keep it simple stupid). |
||
} | ||
return resp | ||
}) | ||
proxy.Verbose = true | ||
proxyServer := &http.Server{ | ||
Handler: proxy, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export const headerSnykAuthFailed = 'snyk-auth-failed'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
const mockMakeRequest = jest.fn(); | ||
|
||
import { Options } from '../../../../../src/lib/types'; | ||
import { MissingApiTokenError } from '../../../../../src/lib/errors'; | ||
|
||
import { resolveAndTestFacts } from '../../../../../src/lib/ecosystems/resolve-test-facts'; | ||
|
||
jest.mock('../../../../../src/lib/request/request', () => { | ||
return { | ||
makeRequest: mockMakeRequest, | ||
}; | ||
}); | ||
|
||
describe('oauth failure', () => { | ||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
it('rethrows same error for missing api token', async () => { | ||
mockMakeRequest.mockRejectedValue(new MissingApiTokenError()); | ||
await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow( | ||
expect.objectContaining({ | ||
message: | ||
'`snyk` requires an authenticated account. Please run `snyk auth` and try again.', | ||
}), | ||
); | ||
}); | ||
|
||
it('rethrows general error for other api auth failures', async () => { | ||
const err: any = new Error('nope'); | ||
err.code = 403; | ||
mockMakeRequest.mockRejectedValue(err); | ||
await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow( | ||
expect.objectContaining({ | ||
message: 'Unauthorized request to unmanaged service', | ||
}), | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
const mockNeedleRequest = jest.fn(); | ||
|
||
import { makeRequest } from '../../../../../src/lib/request/request'; | ||
import { Payload } from '../../../../../src/lib/request/types'; | ||
|
||
jest.mock('needle', () => { | ||
return { | ||
request: mockNeedleRequest, | ||
}; | ||
}); | ||
|
||
describe('needle header auth failed', () => { | ||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
it('throws missing api token on auth failed marker header', async () => { | ||
mockNeedleRequest.mockImplementation((method, url, data, options, fn) => { | ||
fn(null, { headers: { 'snyk-auth-failed': 'true' } }, {}); | ||
}); | ||
await expect( | ||
makeRequest({ url: 'https://example.com' } as Payload), | ||
).rejects.toThrow( | ||
expect.objectContaining({ | ||
message: | ||
'`snyk` requires an authenticated account. Please run `snyk auth` and try again.', | ||
}), | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: appreciate the context given here