-
Notifications
You must be signed in to change notification settings - Fork 138
SNOW-2041980 Kerberos Proxy Auth - NodeJS #1102
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1102 +/- ##
==========================================
- Coverage 89.63% 89.62% -0.02%
==========================================
Files 83 83
Lines 7689 7775 +86
Branches 67 67
==========================================
+ Hits 6892 6968 +76
- Misses 790 800 +10
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
package.json
Outdated
"test": "mocha 'test/unit/**/*.{js,ts}'", | ||
"test:authentication": "mocha 'test/authentication/**/*.{js,ts}'", | ||
"test:integration": "mocha 'test/integration/**/*.{js,ts}'", | ||
"test": "mocha test/unit/**/*.{js,ts}", |
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.
The quotes were added recently in #1079
Mocha's behaviour is poorly documented: when a test pattern is passed without quotes, it uses the default shell pattern matching mechanism (different on every OS/shell), and if a string is passed, it uses glob from the npm package.
You can confirm this by running npm run test:unit
on a command with quotes and without, and you'll see that it runs 300 fewer tests if quotes aren't set 🙈
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.
Hmmm, interesting. On my machine(Windows), I cannot execute the npm command with the quote, and the 791 unit testing were executed without the quote. Anyway, I will have a look and let you know if I find any other solutions. I will revert this back when I need to update the PR based on the feedbacks. Thank you.
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.
Maybe it should be double quotes instead of single to work correctly?
You should always quote your globs in npm scripts. If you use quotes, the node-glob module will handle its expansion. For maximum compatibility, surround the entire expression with double quotes and refrain from $, ", ^, and \ within your expression.
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.
Yes, it worked well when I replaced the single quote with the double quote. Currently, I revert it back. If you want me to fix this in this PR, I will update it. Please let me know.
lib/constants/error_messages.js
Outdated
@@ -86,6 +86,8 @@ exports[404059] = 'Invalid oauth client id. The specified value must not be an e | |||
exports[404060] = 'Invalid oauth client secret. The specified value must not be an empty string'; | |||
exports[404061] = 'Invalid oauth token request URL. The specified value must be a valid URL starting with the https or http protocol.'; | |||
exports[404062] = 'No workload identity credentials were found. Provider: %s'; | |||
exports[404063] = 'Invalid http header customizers. The specified value must contain the following functions: \'applices\', \'newHeaders\' and \'invokeOnce\''; |
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.
Would suggest to adjust eslint config: 'quotes': ['error', 'single', { 'avoidEscape': true }],
So it lets us use ""
instead of ''
when a string contains '
:)
It will let us make this line:
exports[404063] = "Invalid http header customizers. The specified value must contain the following functions: 'applices', 'newHeaders' and 'invokeOnce'";
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.
When I replaced the double quote with the single quote, the linter said:
Error: 89:19 error Strings must use singlequote quotes
Error: 89:[11](https://github.com/snowflakedb/snowflake-connector-nodejs/actions/runs/15858048698/job/44708146624?pr=1102#step:5:12)1 error Unnecessary escape character: \' no-useless-escape
Error: 89:[12](https://github.com/snowflakedb/snowflake-connector-nodejs/actions/runs/15858048698/job/44708146624?pr=1102#step:5:13)1 error Unnecessary escape character: \' no-useless-escape
Error: 89:125 error Unnecessary escape character: \' no-useless-escape
Error: 89:[13](https://github.com/snowflakedb/snowflake-connector-nodejs/actions/runs/15858048698/job/44708146624?pr=1102#step:5:14)7 error Unnecessary escape character: \' no-useless-escape
Error: 89:[14](https://github.com/snowflakedb/snowflake-connector-nodejs/actions/runs/15858048698/job/44708146624?pr=1102#step:5:15)4 error Unnecessary escape character: \' no-useless-escape
Error: 89:[15](https://github.com/snowflakedb/snowflake-connector-nodejs/actions/runs/15858048698/job/44708146624?pr=1102#step:5:16)6 error Unnecessary escape character: \' no-useless-escape
Currently, the linter enforces the use of single quotes only.
lib/util.ts
Outdated
@@ -678,3 +683,19 @@ export function escapeHTML(value: string) { | |||
export async function dynamicImportESMInTypescriptWithCommonJS(moduleName: string) { | |||
return Function(`return import("${moduleName}")`)() | |||
} | |||
|
|||
export function isValidHTTPHeaderCustomizers(customizers: HttpHeadersCustomizer[]) : boolean { |
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.
In one of the recent PRs, there was a discussion that util file is a mess and we should try to reduce it's size by moving helpers next to business logic that needs them.
Would suggest to move both this helper function and HttpHeadersCustomizer type http folder:
- add types.ts
- helper in request_util.js
package.json
Outdated
"test": "mocha 'test/unit/**/*.{js,ts}'", | ||
"test:authentication": "mocha 'test/authentication/**/*.{js,ts}'", | ||
"test:integration": "mocha 'test/integration/**/*.{js,ts}'", | ||
"test": "mocha test/unit/**/*.{js,ts}", |
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.
Maybe it should be double quotes instead of single to work correctly?
You should always quote your globs in npm scripts. If you use quotes, the node-glob module will handle its expansion. For maximum compatibility, surround the entire expression with double quotes and refrain from $, ", ^, and \ within your expression.
Wouldn't it make sense to use this HttpInterceptor class for headers customziation? |
I will have a look. It looks the interceptor you mentioned is for the testing purpose. |
Description
Please explain the changes you made here.
Checklist
npm run lint:check -- CHANGED_FILES
and fix problems in changed code)npm run test:unit
andnpm run test:integration
)