fix: align auth failure error messages for oauth #5346

cmars · 2024-06-26T18:20:26Z

In the Typescript CLI, when OAuth fails due to an invalid or expired token, throw the same "missing auth" error that one would get if not logged in (no auth token).

The resulting error message will prompt the user to re-authenticate.

This has to be mediated by the legacycli http proxy because the Typescript side of the CLI is not involved in the OAuth flow at all; it is handled transparently by the go-application-framework's networking middleware.

A special HTTP request and response header is used by the legacycli proxy to indicate whether that middleware auth has failed. If it has, the functions which perform HTTP client requests with needle will throw an error that indicates "unauthenticated", resulting in an experience consistent with the legacy token auth flow in the CLI.

Outstanding TODOs on this:

Improve the header names
Improve the middleware auth error checking -- a specific error match would be better here
Review the approach with the team
Tests?

Pull Request Submission

Please check the boxes once done.

The pull request must:

Reviewer Documentation
- follow CONTRIBUTING rules
- be accompanied by a detailed description of the changes
- contain a risk assessment of the change (Low | Medium | High) with regards to breaking existing functionality. A change e.g. of an underlying language plugin can completely break the functionality for that language, but appearing as only a version change in the dependencies.
- highlight breaking API if applicable
- contain a link to the automatic tests that cover the updated functionality.
- contain testing instructions in case that the reviewer wants to manual verify as well, to add to the manual testing done by the author.
- link to the link to the PR for the User-facing documentation
User facing Documentation
- update any relevant documentation in gitbook by submitting a gitbook PR, and including the PR link here
- ensure that the message of the final single commit is descriptive and prefixed with either feat: or fix: , others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.
Testing
- Changes, removals and additions to functionality must be covered by acceptance / integration tests or smoke tests - either already existing ones, or new ones, created by the author of the PR.

Pull Request Review

All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.

Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.

For Node projects: It’s important to make sure changes in package.json are also affecting package-lock.json correctly.

If a dependency is not necessary, don’t add it.

When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.

Pull Request Approval

Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.

What does this PR do?

See top posting.

Where should the reviewer start?

How should this be manually tested?

Build this branch locally.
Authenticate with OAuth.
"Damage" the token as follows:
- Set expired in the past
- Flip some characters in the refresh token, making it invalid.
Observe that with this binary, all test commands will fail with the same error message as legacy token auth. Other than Code and SBOM, they'll tell you to snyk auth (that's a pre-existing issue that will be fixed in a followup).

Any background context you want to provide?

What are the relevant tickets?

CLI-392

Screenshots

Additional questions

cliv2/internal/proxy/proxy.go

j-luong · 2024-07-09T16:13:16Z

cliv2/internal/proxy/proxy.go

@@ -143,9 +145,28 @@ func (p *WrapperProxy) ProxyInfo() *ProxyInfo {
 	}
 }

+// headerSnykAuthFailed is used to indicate there was a failure to establish


praise: appreciate the context given here

j-luong · 2024-07-09T16:35:32Z

LGTM:

PeterSchafer · 2024-07-09T16:40:37Z

cliv2/internal/proxy/proxy.go

@@ -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)


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.

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.

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.

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).

PeterSchafer · 2024-07-09T16:58:31Z

FYI there is a failing test, which doesn't expect the new http header.

cmars · 2024-07-09T18:44:09Z

FYI there is a failing test, which doesn't expect the new http header.

Fixed

In the Typescript CLI, when OAuth fails due to an invalid or expired token, throw the same "missing auth" error that one would get if not logged in (no auth token). The resulting error message will prompt the user to re-authenticate. This has to be mediated by the legacycli http proxy because the Typescript side of the CLI is not involved in the OAuth flow at all; it is handled transparently by the go-application-framework's networking middleware. A special HTTP request and response header is used by the legacycli proxy to indicate whether that middleware auth has failed. If it has, the functions which perform HTTP client requests with needle will throw an error that indicates "unauthenticated", resulting in an experience consistent with the legacy token auth flow in the CLI. CLI-392

cmars force-pushed the fix/align-oauth-failure-messages branch 6 times, most recently from 6335749 to 8e398ee Compare June 26, 2024 21:38

cmars commented Jun 26, 2024

View reviewed changes

cliv2/internal/proxy/proxy.go Outdated Show resolved Hide resolved

cmars force-pushed the fix/align-oauth-failure-messages branch 6 times, most recently from a487f3d to 12929a1 Compare June 28, 2024 20:48

cmars marked this pull request as ready for review June 28, 2024 20:48

cmars requested a review from a team as a code owner June 28, 2024 20:48

cmars force-pushed the fix/align-oauth-failure-messages branch from 12929a1 to fe3b116 Compare June 28, 2024 20:49

cmars enabled auto-merge July 8, 2024 13:16

cmars force-pushed the fix/align-oauth-failure-messages branch from fe3b116 to 52f8f3a Compare July 9, 2024 13:17

j-luong reviewed Jul 9, 2024

View reviewed changes

cliv2/internal/proxy/proxy.go Outdated Show resolved Hide resolved

j-luong reviewed Jul 9, 2024

View reviewed changes

cmars force-pushed the fix/align-oauth-failure-messages branch from 52f8f3a to d947ed2 Compare July 9, 2024 16:28

j-luong approved these changes Jul 9, 2024

View reviewed changes

PeterSchafer reviewed Jul 9, 2024

View reviewed changes

cmars force-pushed the fix/align-oauth-failure-messages branch from d947ed2 to 963db44 Compare July 9, 2024 18:42

cmars disabled auto-merge July 9, 2024 18:44

sandor-trombitas approved these changes Jul 10, 2024

View reviewed changes

cmars force-pushed the fix/align-oauth-failure-messages branch from 963db44 to 88ff4a5 Compare July 10, 2024 12:33

cmars enabled auto-merge July 10, 2024 15:43

cmars force-pushed the fix/align-oauth-failure-messages branch from 88ff4a5 to e3bfec3 Compare July 11, 2024 13:07

cmars merged commit bdeaa44 into main Jul 11, 2024
11 checks passed

cmars deleted the fix/align-oauth-failure-messages branch July 11, 2024 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: align auth failure error messages for oauth #5346

fix: align auth failure error messages for oauth #5346

cmars commented Jun 26, 2024 •

edited

Loading

j-luong Jul 9, 2024

j-luong commented Jul 9, 2024

PeterSchafer Jul 9, 2024

PeterSchafer Jul 9, 2024

cmars Jul 9, 2024

PeterSchafer Jul 10, 2024

PeterSchafer commented Jul 9, 2024

cmars commented Jul 9, 2024

fix: align auth failure error messages for oauth #5346

fix: align auth failure error messages for oauth #5346

Conversation

cmars commented Jun 26, 2024 • edited Loading

Pull Request Submission

Pull Request Review

Pull Request Approval

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

j-luong Jul 9, 2024

Choose a reason for hiding this comment

j-luong commented Jul 9, 2024

PeterSchafer Jul 9, 2024

Choose a reason for hiding this comment

PeterSchafer Jul 9, 2024

Choose a reason for hiding this comment

cmars Jul 9, 2024

Choose a reason for hiding this comment

PeterSchafer Jul 10, 2024

Choose a reason for hiding this comment

PeterSchafer commented Jul 9, 2024

cmars commented Jul 9, 2024

cmars commented Jun 26, 2024 •

edited

Loading