fix: bug with --experimental-dep-graph and wrong auth token #882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
dkontorovskyy
force-pushed
the
fix/monitor-auth-bug
branch
2 times, most recently
from
c690006
to
a511bc5
Compare
dkontorovskyy
requested review from
shaninja and
gitphill
and removed request for
lwywoo
gitphill
reviewed
Nov 26, 2019
| export function AuthFailedError( | ||
| errorMessage: string = 'Authentication failed. Please check the API token on ' + | ||
| config.ROOT, | ||
| errorCode = 401, |
src/cli/commands/monitor/index.ts
Outdated
| @@ -83,6 +88,10 @@ async function monitor(...args0: MethodArgs): Promise<any> { | |||
|
|
|||
| apiTokenExists(); | |||
|
|
|||
| if (!(await isAuthed())) { | |||
There was a problem hiding this comment.
Other modules perform isAuthed before apiTokenExists, expecting apiTokenExists to throw MissingApiTokenError when auth fails. Seems a bit subtle to me, however we probably have this pattern to support commands that can be run when unauthenticated?
- ignore: https://github.com/snyk/snyk/blob/c965884b69624d7ab293391c0cd9107e3d03c27b/src/cli/commands/ignore.ts#L18-L26
- protect wizard: https://github.com/snyk/snyk/blob/79b792b57407408eb7df71e9cec3efc2605169a8/src/cli/commands/protect/wizard.ts#L99-L108
- test: I couldn't see isAuthed being used in test, but apiTokenExists is called - maybe same bug there?
| @@ -133,11 +133,37 @@ test('monitor for package with no name in lockfile', async (t) => { | |||
| t.pass('succeed'); | |||
| }); | |||
|
|
|||
| test('`monitor npm-package with experimental-dep-graph not enabled`', async (t) => { | |||
| test('`monitor npm-package with experimental-dep-graph enabled, but bad auth token`', async (t) => { | |||
dkontorovskyy
force-pushed
the
fix/monitor-auth-bug
branch
2 times, most recently
from
4af3b20
to
d70b173
Compare
|
Just want to make sure - did you test this with FF enabled for the group and not the org? (which was the original issue reported, so it would be nice to validate it) |
dkontorovskyy
force-pushed
the
fix/monitor-auth-bug
branch
from
d70b173
to
89fa972
Compare
|
Doing a smaller refactor to fix the issue and creating a ticket for later for overall CLI improvement, cos this is much bigger scope that was thought in the beginning. |
dkontorovskyy
force-pushed
the
fix/monitor-auth-bug
branch
from
89fa972
to
924f10a
Compare
gitphill
approved these changes
Nov 27, 2019
|
🎉 This PR is included in version 1.251.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Sep 15, 2021
This was referenced Sep 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fix auth error if experimental-dep-graph exists.
If cli was invoked with experimental-dep-graph flag and invalid token, user will get error message
Feature flag 'experimental-dep-graph' is not currently enabled for your org, to enable please contact snyk support, cos token wasn't checked for validity and line https://github.com/snyk/snyk/blob/master/src/cli/commands/monitor/index.ts#L91 will return actually 401 Not Authorised, but because we do isFFSupported.ok and it was undefined, user will see totally unrelated error message.