From ba182deba1d6482044482dc7d79d58793b43e59e Mon Sep 17 00:00:00 2001 From: Przemyslaw Motacki Date: Thu, 5 Oct 2023 14:54:08 +0200 Subject: [PATCH] SNOW-334890 - refactor of the base httpClient --- lib/authentication/auth_web.js | 4 +- lib/authentication/sso_url_provider.js | 15 +- lib/connection/connection.js | 4 +- lib/http/base.js | 179 ++++++++---------- lib/http/node.js | 9 +- .../ocsp_mock/https_ocsp_mock_agent.js | 1 - .../ocsp_mock/testConnectionOcspMock.js | 12 +- .../authentication/authentication_test.js | 17 +- 8 files changed, 100 insertions(+), 141 deletions(-) diff --git a/lib/authentication/auth_web.js b/lib/authentication/auth_web.js index 2f383affe..a95b3a703 100644 --- a/lib/authentication/auth_web.js +++ b/lib/authentication/auth_web.js @@ -7,7 +7,7 @@ const net = require('net'); const querystring = require('querystring'); const URLUtil = require('./../../lib/url_util'); const Util = require('./../../lib/util'); -var SsoUrlProvider = require('../authentication/sso_url_provider'); +const SsoUrlProvider = require('../authentication/sso_url_provider'); /** * Creates an external browser authenticator. @@ -23,7 +23,7 @@ function auth_web(connectionConfig, httpClient, webbrowser) { const host = connectionConfig.host; const browserActionTimeout = connectionConfig.getBrowserActionTimeout(); - const ssoUrlProvider = new SsoUrlProvider(connectionConfig, httpClient); + const ssoUrlProvider = new SsoUrlProvider(httpClient); if (!Util.exists(host)) { throw new Error(`Invalid value for host: ${host}`); diff --git a/lib/authentication/sso_url_provider.js b/lib/authentication/sso_url_provider.js index 862f52a7b..d84ad242e 100644 --- a/lib/authentication/sso_url_provider.js +++ b/lib/authentication/sso_url_provider.js @@ -9,13 +9,11 @@ const { rest } = require('../global_config'); /** * Creates a new instance of an SsoUrlProvider. * - * @param {Object} connectionConfig * @param {Object} httpClient * @constructor */ -function SsoUrlProvider(connectionConfig, httpClient) { +function SsoUrlProvider(httpClient) { - Errors.assertInternal(Util.isObject(connectionConfig)); Errors.assertInternal(Util.isObject(httpClient)); const port = rest.HTTPS_PORT; @@ -54,18 +52,17 @@ function SsoUrlProvider(connectionConfig, httpClient) { } }; - const agent = httpClient.getAgent(url, connectionConfig.getProxy()); - const requestOptions = { + method: 'post', + url: url, headers: header, - requestOCSP: false, - rejectUnauthorized: true, - httpsAgent: agent + data: body, + responseType: 'json' }; // Post request to get the SSO URL - return httpClient.post(url, body, requestOptions) + return httpClient.requestAsync(requestOptions) .then((response) => { const data = response['data']['data']; return data; diff --git a/lib/connection/connection.js b/lib/connection/connection.js index 3f0733c85..c09192a82 100644 --- a/lib/connection/connection.js +++ b/lib/connection/connection.js @@ -236,7 +236,7 @@ function Connection(context) } // Get authenticator to use - var auth = Authenticator.getAuthenticator(connectionConfig, context.getServices().ssoUrlProvider); + const auth = Authenticator.getAuthenticator(connectionConfig, context.getServices().ssoUrlProvider); try { @@ -297,7 +297,7 @@ function Connection(context) var self = this; // Get authenticator to use - var auth = Authenticator.getAuthenticator(connectionConfig, context.getHttpClient()); + const auth = Authenticator.getAuthenticator(connectionConfig, context.getHttpClient()); try { diff --git a/lib/http/base.js b/lib/http/base.js index 1793fa01b..2d92f9650 100644 --- a/lib/http/base.js +++ b/lib/http/base.js @@ -7,6 +7,7 @@ const Util = require('../util'); const Errors = require('../errors'); const Logger = require('../logger'); const axios = require('axios'); +const URL = require('node:url').URL; const DEFAULT_REQUEST_TIMEOUT = 360000; @@ -34,69 +35,9 @@ HttpClient.prototype.getConnectionConfig = function () * * @returns {Object} an object representing the request that was issued. */ -HttpClient.prototype.request = function (options) -{ - // validate input - Errors.assertInternal(Util.isObject(options)); - Errors.assertInternal(Util.isString(options.method)); - Errors.assertInternal(Util.isString(options.url)); - Errors.assertInternal(!Util.exists(options.headers) || - Util.isObject(options.headers)); - Errors.assertInternal(!Util.exists(options.json) || - Util.isObject(options.json)); - Errors.assertInternal(!Util.exists(options.callback) || - Util.isFunction(options.callback)); - - var headers; - var json; - var body; - var request; - - // normalize the headers - headers = normalizeHeaders(options.headers); - - // create a function to send the request - var sendRequest = async function sendRequest() - { - const url = options.url; - - const timeout = options.timeout || - this._connectionConfig.getTimeout() || - DEFAULT_REQUEST_TIMEOUT; - - Logger.getInstance().trace(`CALL ${options.method} with timeout ${timeout}: ${url}`); - // build the basic request options - - var requestOptions = - { - method: options.method, - url: url, - headers: headers, - gzip: options.gzip, - json: json, - body: body, - timeout: timeout, - requestOCSP: true, - rejectUnauthorized: true, - // we manually parse jsons or other structures from the server so they need to be text - responseType: 'text', - }; - - let mock; - if (this._connectionConfig.agentClass) - { - mock = { - agentClass: this._connectionConfig.agentClass - } - } - - // add the agent and proxy options - const agent = this.getAgent(url, this._connectionConfig.getProxy(), mock); - - requestOptions.data = requestOptions.body; - requestOptions.httpsAgent = agent; - requestOptions.retryDelay = this.constructExponentialBackoffStrategy(); - +HttpClient.prototype.request = function (options) { + const requestOptions = prepareRequestOptions.call(this, options); + let sendRequest = async function sendRequest() { request = axios.request(requestOptions).then(response => { if (Util.isFunction(options.callback)) { return options.callback(null, normalizeResponse(response), response.data); @@ -119,55 +60,34 @@ HttpClient.prototype.request = function (options) }; sendRequest = sendRequest.bind(this); - // if a request body is specified and compression is enabled, - // try to compress the request body before sending the request - json = body = options.json; - if (body) - { - var bufferUncompressed = Buffer.from(JSON.stringify(body), 'utf8'); - zlib.gzip(bufferUncompressed, null, function (err, bufferCompressed) - { - // if the compression was successful - if (!err) - { - json = undefined; // don't specify the 'json' option - - // use the compressed buffer as the body and - // set the appropriate content encoding - body = bufferCompressed; - headers['Content-Encoding'] = 'gzip'; - } - else - { - Logger.getInstance().warn('Could not compress request body.'); - } - - sendRequest(); - }); - } - else - { - if (body) - { - Logger.getInstance().trace('Request body compression disabled.'); - } - - process.nextTick(sendRequest); - } + Logger.getInstance().trace(`CALL ${requestOptions.method} with timeout ${requestOptions.timeout}: ${requestOptions.url}`); + process.nextTick(sendRequest); // return an externalized request object that only contains // methods we're comfortable exposing to the outside world return { - abort: function () - { - if (request) - { + abort: function () { + if (request) { request.abort(); } } }; }; +/** + * Issues an HTTP request. + * + * @param {Object} options + * + * @returns {Object} an object representing the request that was issued. + */ +HttpClient.prototype.requestAsync = async function (options) +{ + const requestOptions = prepareRequestOptions.call(this, options); + + return axios.request(requestOptions); +}; + /** * @abstract * Returns the module to use when making HTTP requests. Subclasses must override @@ -192,10 +112,61 @@ HttpClient.prototype.getAgent = function (url, proxy, mock) { return null; }; -HttpClient.prototype.post = axios.post; - module.exports = HttpClient; +function prepareRequestOptions(options) { + let headers = normalizeHeaders(options.headers) || {}; + + const timeout = options.timeout || + this._connectionConfig.getTimeout() || + DEFAULT_REQUEST_TIMEOUT; + + let data = options.data || options.json; + + if (data) { + var bufferUncompressed = Buffer.from(JSON.stringify(data), 'utf8'); + zlib.gzip(bufferUncompressed, null, function (err, bufferCompressed) { + // if the compression was successful + if (!err) { + data = bufferCompressed; + headers['Content-Encoding'] = 'gzip'; + } else { + Logger.getInstance().warn('Could not compress request data.'); + } + }); + } + let mock; + if (this._connectionConfig.agentClass) { + mock = { + agentClass: this._connectionConfig.agentClass + } + } + const backoffStrategy = this.constructExponentialBackoffStrategy(); + const requestOptions = { + method: options.method, + url: options.url, + headers: headers, + data: data, + timeout: timeout, + requestOCSP: true, + retryDelay: backoffStrategy, + rejectUnauthorized: true, + // we manually parse jsons or other structures from the server so they need to be text + responseType: options.responseType || 'text', + } + + const url = new URL(options.url); + const isHttps = url.protocol === 'https:'; + const agent = this.getAgent(url, this._connectionConfig.getProxy(), mock); + if (isHttps) { + requestOptions.httpsAgent = agent; + } else { + requestOptions.httpAgent = agent; + } + + return requestOptions; +} + /** * Normalizes a request headers object so that we get the same behavior * regardless of whether we're using request.js or browser-request.js. diff --git a/lib/http/node.js b/lib/http/node.js index 57f873d9f..44febaf2f 100644 --- a/lib/http/node.js +++ b/lib/http/node.js @@ -2,7 +2,6 @@ * Copyright (c) 2015-2019 Snowflake Computing Inc. All rights reserved. */ -const Url = require('url'); const Util = require('../util'); const Base = require('./base'); const HttpsAgent = require('../agent/https_ocsp_agent'); @@ -10,6 +9,7 @@ const HttpsProxyAgent = require('../agent/https_proxy_ocsp_agent'); const HttpAgent = require('http').Agent; const GlobalConfig = require('../../lib/global_config'); const Logger = require('../logger'); +const Url = require("url"); /** * Returns the delay time calculated by exponential backoff with @@ -94,16 +94,15 @@ function isBypassProxy(proxy, url, bypassProxy) { /** * @inheritDoc */ -NodeHttpClient.prototype.getAgent = function (url, proxy, mock) { +NodeHttpClient.prototype.getAgent = function (parsedUrl, proxy, mock) { const agentOptions = {keepAlive: GlobalConfig.getKeepAlive()}; if (mock) { return mock.agentClass(agentOptions); } - const parsedUrl = Url.parse(url); - const isHttps = parsedUrl.protocol === 'https:'; - const bypassProxy = isBypassProxy(proxy, url); + const bypassProxy = isBypassProxy(proxy, parsedUrl.href); let agent = {}; + const isHttps = parsedUrl.protocol === 'https:'; if (isHttps) { if (proxy && !bypassProxy) { diff --git a/test/integration/ocsp_mock/https_ocsp_mock_agent.js b/test/integration/ocsp_mock/https_ocsp_mock_agent.js index 2a3593feb..017c8a0aa 100644 --- a/test/integration/ocsp_mock/https_ocsp_mock_agent.js +++ b/test/integration/ocsp_mock/https_ocsp_mock_agent.js @@ -3,7 +3,6 @@ */ const HttpsAgent = require('https').Agent; -const Util = require('../../../lib/util'); const SocketUtil = require('../../../lib/agent/socket_util'); const Errors = require('../../../lib/errors'); const ErrorCodes = Errors.codes; diff --git a/test/integration/ocsp_mock/testConnectionOcspMock.js b/test/integration/ocsp_mock/testConnectionOcspMock.js index d9b78aef2..4809112e6 100644 --- a/test/integration/ocsp_mock/testConnectionOcspMock.js +++ b/test/integration/ocsp_mock/testConnectionOcspMock.js @@ -9,6 +9,7 @@ const connOption = require('../connectionOptions'); const Errors = require('../../../lib/errors'); const ErrorCodes = Errors.codes; const HttpsMockAgent = require('./https_ocsp_mock_agent'); +const Logger = require("../../../lib/logger"); function cloneConnOption(connOption) { @@ -20,9 +21,12 @@ function cloneConnOption(connOption) return ret; } -describe('Connection test with OCSP Mock', function () +describe.only('Connection test with OCSP Mock', function () { const valid = cloneConnOption(connOption.valid); + + console.log(connOption.valid); + const isHttps = valid.accessUrl.startsWith("https"); function connect(errcode, connection, callback) @@ -32,10 +36,8 @@ describe('Connection test with OCSP Mock', function () if (isHttps) { assert.equal(err.cause.code, errcode); - } - else - { - assert.ok(!err, JSON.stringify(err)); + } else { + Logger.getInstance().info('Test can be run only for https protocol'); } callback(); }) diff --git a/test/unit/authentication/authentication_test.js b/test/unit/authentication/authentication_test.js index add03a563..83ed9990e 100644 --- a/test/unit/authentication/authentication_test.js +++ b/test/unit/authentication/authentication_test.js @@ -13,7 +13,6 @@ var auth_keypair = require('./../../../lib/authentication/auth_keypair'); var auth_oauth = require('./../../../lib/authentication/auth_oauth'); var auth_okta = require('./../../../lib/authentication/auth_okta'); var authenticationTypes = require('./../../../lib/authentication/authentication').authenticationTypes; -const HttpAgent = require('https').Agent; var MockTestUtil = require('./../mock/mock_test_util'); @@ -92,11 +91,7 @@ describe('external browser authentication', function () } }); mock('httpclient', { - getAgent: function (url, body, header) - { - return new HttpAgent(); - }, - post: async function (url, body, header) + requestAsync: async function (options) { const data = { @@ -108,7 +103,7 @@ describe('external browser authentication', function () } } } - browserRedirectPort = body['data']['BROWSER_MODE_REDIRECT_PORT']; + browserRedirectPort = options.data['data']['BROWSER_MODE_REDIRECT_PORT']; return data; } }); @@ -152,11 +147,7 @@ describe('external browser authentication', function () }); mock('httpclient', { - getAgent: function (url, body, header) - { - return new HttpAgent(); - }, - post: async function (url, body, header) + requestAsync: async function (options) { const data = { @@ -167,7 +158,7 @@ describe('external browser authentication', function () } } } - browserRedirectPort = body['data']['BROWSER_MODE_REDIRECT_PORT']; + browserRedirectPort = options.data['data']['BROWSER_MODE_REDIRECT_PORT']; return data; } });