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

Fixed issues related to previous PR regarding AAD authentication via connection string #1461

Merged

Conversation

Shin-Aska
Copy link
Contributor

This PR is aimed to fix a couple of issues with the previous PR: #1436 in attempt to accomodate issue #1400

Changes include:

  1. Authentication now uses the standard values defined on .NET Platform Extension 7
  2. Fixes the breakage of standard/basic connection which has been reported in Login failed for user ''. on v9.1.0 #1460

Copy link
Collaborator

@dhensby dhensby left a comment

Choose a reason for hiding this comment

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

Thanks - this is looking a lot better. Please can we have some tests for this, to assert the auth credentials are assigned as expected.

@Shin-Aska
Copy link
Contributor Author

I tested this using the following:

Basic SQL Login:

var sql = require("../../node-mssql/tedious")
var main = async () => {
	var server = `${process.env.server}`
	var username = `${process.env.username}`
	var database = `${process.env.database}`
	var password = `${process.env.password}`
	var table = `${process.env.table}`

	var conn_str = `Server=${server};Database=${database};User Id=${username};Password=${password};`
	console.log(conn_str)
	await sql.connect(conn_str).then(async(pool) => {
		var result = await pool.request().query(`select top 1 * from ${table}`).catch((err) => {
			console.warn(err)
			throw err
		});
		console.log(result)
	})
}

main()

image

The other one is using azure-active-directory-service-principal-secret. (Have to redact starting here)

var sql = require("../../node-mssql/tedious")
var main = async () => {

	var server = `${process.env.server}`
	var database = `${process.env.database}`
	var conn_str = `Server=${server};Database=${database};Authentication=Active Directory Integrated;client id=${process.env.AZURE_CLIENT_ID};client secret=${process.env.AZURE_CLIENT_SECRET};tenant id=${process.env.AZURE_TENANT_ID}`;
	var table = `${process.env.table}`

	await sql.connect(conn_str).then(async(pool) => {
		var result = await pool.request().query(`select top 1 * from ${table}`).catch((err) => {
			console.warn(err)
			throw err
		});
		console.log(result)
	});
}

main();

image

And lastly is azure-active-directory-access-token

var sql = require("../../node-mssql/tedious")
const { EnvironmentCredential } = require('@azure/identity')
const tokenUrl = 'https://database.windows.net/.default'

var main = async () => {
	const credential = new EnvironmentCredential()
	const { token } = await credential.getToken(tokenUrl);
	var server = `${process.env.server}`
	var database = `${process.env.database}`
	var table = `${process.env.table}`

	var conn_str = `Server=${server};Database=${database};Authentication=Active Directory Integrated;Token=${token};`;
	await sql.connect(conn_str).then(async(pool) => {
		var result = await pool.request().query(`select top 1 * from ${table}`).catch((err) => {
			console.warn(err)
			throw err
		});
		console.log(result)
	});
}

main();

image

On the basic login, I tried both SQL servers hosted elsewhere outside Azure (SQL Server 2017-2019) and Azure. While the AAD one is Azure only.

@dhensby
Copy link
Collaborator

dhensby commented Jan 22, 2023

Thanks for that. Whilst that manual testing is really helpful, what I was referring to was the automated test suite. We need some tests that the connection string gets correctly parsed and the output shape looks right for our internal config structure.

The unit tests is the best place to add this. Something like:

describe('connection string auth', () => {
  it('parses basic login', () => {
    const config = BaseConnection._parseConnectionString('test string here')
    assert.equal(config, { expected shape })
  })
  ... more tests here
})

@Shin-Aska
Copy link
Contributor Author

Ok cool, I do have some questions though, for example this connection string:

'Server=database.test.com;Database=test;User Id=admin;Password=admin'

In the BaseConnectionPool under /lib/base/connection-pool.js, the object becomes:

{
  options: { instanceName: undefined },
  pool: {},
  port: 1433,
  server: 'database.test.com',
  database: 'test',
  domain: undefined,
  user: 'test',
  password: 'admin'
}

But if it passes through /lib/tedious/connection-pool.js, the config above becomes:

{
  server: 'database.test.com',
  options: {
    encrypt: true,
    trustServerCertificate: false,
    instanceName: undefined,
    database: 'test',
    port: 1433,
    connectTimeout: 15000,
    requestTimeout: 15000,
    tdsVersion: '7_4',
    rowCollectionOnDone: false,
    rowCollectionOnRequestCompletion: false,
    useColumnNames: false,
    appName: 'node-mssql'
  },
  authentication: {
    type: 'default',
    options: {
      userName: 'admin',
      password: 'admin',
      domain: undefined,
      clientId: undefined,
      clientSecret: undefined,
      tenantId: undefined,
      token: undefined
    }
  }
}

Which one should we start testing, the base one? tedious or both? and if both or tedious is included, would it be fine if we seperate the config generation away from _poolCreate() so we can reuse them on the unit-test file?

@Shin-Aska
Copy link
Contributor Author

Hey @dhensby, I made some changes today, I decided to add both unit tests (Base and Tedious) comparison instead of just base or tedious. Seems to be a good idea to test the two in-case some changes are made either in the connection-pool of either base library or tedious, we can check if it gets affected.

@dhensby dhensby force-pushed the feature/aad-integration-conn-string branch 2 times, most recently from 7a418d1 to 7b22303 Compare August 8, 2023 11:38
@dhensby dhensby force-pushed the feature/aad-integration-conn-string branch 2 times, most recently from 1bd8942 to c772fe6 Compare September 4, 2023 21:01
@dhensby dhensby force-pushed the feature/aad-integration-conn-string branch from c772fe6 to 59aea21 Compare September 4, 2023 21:21
@dhensby dhensby merged commit b5cf976 into tediousjs:master Sep 4, 2023
47 checks passed
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

🎉 This PR is included in version 9.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tediousjs tediousjs deleted a comment from github-actions bot Sep 5, 2023
@tediousjs tediousjs deleted a comment from github-actions bot Sep 5, 2023
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