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

[WIP] fix: check token for validity before making any requests #881

Closed
wants to merge 1 commit into from

Conversation

dkontorovskyy
Copy link
Contributor

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

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:/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.

Copy link
Contributor

@gitphill gitphill left a comment

Choose a reason for hiding this comment

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

add a test for the issue reported? experimental-dep-graph and invalid token now produces correct error message

import * as config from './config';
import * as userConfig from './user-config';

export function api() {
// note: config.TOKEN will potentially come via the environment
return config.api || config.TOKEN || userConfig.get('api');
}
async function isTokenValid(token: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have very similar function in src/cli/commands/auth/is-authed.ts - could reuse (use this function there) to avoid duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was looking for this one but haven't found :( Nice catch 👍


export function apiTokenExists() {
return (response as any).body;
Copy link
Contributor

Choose a reason for hiding this comment

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

type the function return object?

@dkontorovskyy
Copy link
Contributor Author

Closing this PR, cos this is not right approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants