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

Jump to bottom

PDO - support username & password specified in DSN #2684

Closed
wants to merge 6 commits into from
Closed

PDO - support username & password specified in DSN #2684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5748e7
PDO - support username & password specified in DSN
SjonHortensius Aug 14, 2017
5e1a99e
Extend PDO support for credentials in DSN to dblib/firebire/oci
KalleZ Aug 15, 2017
dd96d1d
Move pdo_dsn_containing_credentials from pdo_mysql to pdo
SjonHortensius Aug 16, 2017
cc3f7b1
appveyor - don't include credentials in DSN
SjonHortensius May 17, 2019
54e3c49
Merge branch 'master' into pdo_mysql_dsn_credentials_support
SjonHortensius May 17, 2019
e6e64e4
Fix test - was moved in dd96d1d8ea but not correctly refactored
SjonHortensius May 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion appveyor/test_task.bat
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ set PDO_MYSQL_TEST_USER=%MYSQL_TEST_USER%
set PDO_MYSQL_TEST_PASS=%MYSQL_PWD%
set PDO_MYSQL_TEST_HOST=%MYSQL_TEST_HOST%
set PDO_MYSQL_TEST_PORT=%MYSQL_TEST_PORT%
set PDO_MYSQL_TEST_DSN=mysql:host=%PDO_MYSQL_TEST_HOST%;port=%PDO_MYSQL_TEST_PORT%;dbname=test;user=%PDO_MYSQL_TEST_USER%;password=%MYSQL_PW%
set PDO_MYSQL_TEST_DSN=mysql:host=%PDO_MYSQL_TEST_HOST%;port=%PDO_MYSQL_TEST_PORT%;dbname=test
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be done in a separate PR and then merged to PHP-7.2 and upwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I wouldn't mind; this change isn't necessary for branches that don't parse credentials in the dsn - so it's not really needed, do you disagree? It'll require some git-fu to undo the merge and keep this branch merge-able

"C:\Program Files\MySql\MySQL Server 5.7\bin\mysql.exe" --user=%MYSQL_TEST_USER% -e "CREATE DATABASE IF NOT EXISTS test"
if %errorlevel% neq 0 exit /b 3

Expand Down
51 changes: 51 additions & 0 deletions ext/pdo/tests/pdo_dsn_containing_credentials.phpt
View file
Open in desktop
@@ -0,0 +1,51 @@
--TEST--
PDO Common: Pass credentials in dsn instead of constructor params
--SKIPIF--
<?php
if (!extension_loaded('pdo')) die('skip');
$dir = getenv('REDIR_TEST_DIR');
if (false == $dir) die('skip no driver');

$driver = substr(getenv('PDOTEST_DSN'), 0, strpos(getenv('PDOTEST_DSN'), ':'));
if (!in_array($driver, array('mssql','sybase','dblib','firebird','mysql','oci')))
SjonHortensius marked this conversation as resolved.
Show resolved Hide resolved
die('skip not supported');

require_once $dir . 'pdo_test.inc';
PDOTest::skip();
?>
--FILE--
<?php
require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';

$orgDsn = getenv('PDOTEST_DSN');
$orgUser = getenv('PDOTEST_USER');
$orgPass = getenv('PDOTEST_PASS');

try
{
putenv("PDOTEST_DSN=$orgDsn;user=$orgUser;password=$orgPass");
putenv("PDOTEST_USER");
putenv("PDOTEST_PASS");

$link = PDOTest::factory();
echo "using credentials in dsn: done\n";


// test b/c - credentials in DSN are ignored when user/pass passed as separate params
putenv("PDOTEST_DSN=$orgDsn;user=incorrect;password=ignored");
putenv("PDOTEST_USER=$orgUser");
putenv("PDOTEST_PASS=$orgPass");

$link = PDOTest::factory();
echo "ignoring credentials in dsn: done\n";
}
finally
{
putenv("PDOTEST_DSN=$orgDsn");
putenv("PDOTEST_USER=$orgUser");
putenv("PDOTEST_PASS=$orgPass");
}
?>
--EXPECTF--
using credentials in dsn: done
ignoring credentials in dsn: done
10 changes: 10 additions & 0 deletions ext/pdo_dblib/dblib_driver.c
View file
Open in desktop
Expand Up @@ -458,6 +458,8 @@ static int pdo_dblib_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
,{ "dbname", NULL, 0 }
,{ "secure", NULL, 0 } /* DBSETLSECURE */
,{ "version", NULL, 0 } /* DBSETLVERSION */
,{ "user", NULL, 0 }
,{ "password", NULL, 0 }
};

nvars = sizeof(vars)/sizeof(vars[0]);
Expand Down Expand Up @@ -519,12 +521,20 @@ static int pdo_dblib_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
}
}

if (!dbh->username && vars[6].optval) {
dbh->username = vars[6].optval;
}

if (dbh->username) {
if(FAIL == DBSETLUSER(H->login, dbh->username)) {
goto cleanup;
}
}

if (!dbh->password && vars[7].optval) {
Copy link

@SpyroTEQ SpyroTEQ Feb 19, 2019 •
edited

Choose a reason for hiding this comment

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

This might be a dumb question (as I'm new here, still trying to make & run tests properly): how will this behave for new PDO("...;user=root;password=shouldbeoverwritten", 'overuser', '')?
As far as I get the key idea here, the 2nd and 3rd construct parameters prevail, so user should be overuser and password should be empty. But will the test here consider that since password (from 3rd param) is empty, then the one in DSN will be used and so, instead of "overuser"+"" empty password, we get "overuser"+"shouldbeoverwritten" ?

The same question goes for all other places this test is used

Copy link
Member

Choose a reason for hiding this comment

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

@SpyroTEQ Sorry for the long time between replies, as this PR seems inactive. The idea is that if a username and password is set for the constructor to PDO (argument 2 and 3), then it will override the ones sent in the DSN, this is how other PDO drivers did it so I wrote it like that (in this case for pdo_dblib) to keep the drivers somewhat consistent

Copy link

@SpyroTEQ SpyroTEQ May 28, 2019 •
edited

Choose a reason for hiding this comment

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

Got it (sorry from my side too to take so long to answer), but I was actually focusing on how you test that the password has been set in DSN and how you test that password has been set as 3rd argument. What I mean is that !dbh->password might be true if password is not set, but is it also true if password is set to empty? Same question for vars[7].optval?

In other words, these cases might be worth being tested (or must match the tests used, considering empty/not set values as different):

  • new PDO("...;password=shouldbeoverwritten", 'overuser', '') : password must be the empty one
  • new PDO("...;password=shouldbeoverwritten", 'overuser', '000') : password must be '000'
  • new PDO("...;password=shouldbeoverwritten", 'overuser', 'false') : password must be 'false' (pretty dumb password but still)
  • new PDO("...;password=", 'overuser', 'xxx') : password must be 'xxx'
  • new PDO("...;password=000", 'overuser', 'xxx') : password must be 'xxx'
  • new PDO("...;password=inuse", 'overuser') : password must be 'inuse'
  • new PDO("...;password=", 'overuser') : password must be empty one
  • new PDO("...;", 'overuser') : password must be backward compatible

Maybe this is an absurd question in C code, but in PHP code itself, it's often easy to mistake between empty string, not set string (null), 0-filled string, false literal, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SpyroTEQ while that is indeed interesting - it is currently impossible to test like that. We have one valid set of credentials for testing and I don't think it's productive to start issuing SET PASSWORD calls just for this. Since the implementation is already in C - I'm not sure testing for weird PHP behavior is productive here. I do invite you to test the examples you specified and report back if you find any discrepancies.

dbh->password = vars[7].optval;
}

if (dbh->password) {
if(FAIL == DBSETLPWD(H->login, dbh->password)) {
goto cleanup;
Expand Down
14 changes: 12 additions & 2 deletions ext/pdo_firebird/firebird_driver.c
View file
Open in desktop
Expand Up @@ -614,14 +614,24 @@ static int pdo_firebird_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /*
struct pdo_data_src_parser vars[] = {
{ "dbname", NULL, 0 },
{ "charset", NULL, 0 },
{ "role", NULL, 0 }
{ "role", NULL, 0 },
{ "user", NULL, 0 },
{ "password", NULL, 0 }
};
int i, ret = 0;
short buf_len = 256, dpb_len;

pdo_firebird_db_handle *H = dbh->driver_data = pecalloc(1,sizeof(*H),dbh->is_persistent);

php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 3);
php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 5);

if (!dbh->username && vars[3].optval) {
dbh->username = vars[3].optval;
}

if (!dbh->password && vars[4].optval) {
dbh->password = vars[4].optval;
}

do {
static char const dpb_flags[] = {
Expand Down
16 changes: 13 additions & 3 deletions ext/pdo_mysql/mysql_driver.c
View file
Open in desktop
Expand Up @@ -568,9 +568,11 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
struct pdo_data_src_parser vars[] = {
{ "charset", NULL, 0 },
{ "dbname", "", 0 },
{ "host", "localhost", 0 },
{ "port", "3306", 0 },
{ "host", "localhost", 0 },
{ "port", "3306", 0 },
{ "unix_socket", PDO_DEFAULT_MYSQL_UNIX_ADDR, 0 },
{ "user", NULL, 0 },
{ "password", NULL, 0 },
};
int connect_opts = 0
#ifdef CLIENT_MULTI_RESULTS
Expand All @@ -596,7 +598,7 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
PDO_DBG_INF("multi results");
#endif

php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 5);
php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 7);

H = pecalloc(1, sizeof(pdo_mysql_db_handle), dbh->is_persistent);

Expand Down Expand Up @@ -808,6 +810,14 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
unix_socket = vars[4].optval;
}

if (!dbh->username && vars[5].optval) {
dbh->username = vars[5].optval;
}

if (!dbh->password && vars[6].optval) {
dbh->password = vars[6].optval;
}

/* TODO: - Check zval cache + ZTS */
#ifdef PDO_USE_MYSQLND
if (dbname) {
Expand Down
14 changes: 12 additions & 2 deletions ext/pdo_oci/oci_driver.c
View file
Open in desktop
Expand Up @@ -666,10 +666,12 @@ static int pdo_oci_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{{ *
int i, ret = 0;
struct pdo_data_src_parser vars[] = {
{ "charset", NULL, 0 },
{ "dbname", "", 0 }
{ "dbname", "", 0 },
{ "user", NULL, 0 },
{ "password", NULL, 0 }
};

php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 2);
php_pdo_parse_data_source(dbh->data_source, dbh->data_source_len, vars, 4);

H = pecalloc(1, sizeof(*H), dbh->is_persistent);
dbh->driver_data = H;
Expand Down Expand Up @@ -733,6 +735,10 @@ static int pdo_oci_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{{ *
}

/* username */
if (!dbh->username && vars[2].optval) {
dbh->username = vars[2].optval;
}

if (dbh->username) {
H->last_err = OCIAttrSet(H->session, OCI_HTYPE_SESSION,
dbh->username, (ub4) strlen(dbh->username),
Expand All @@ -744,6 +750,10 @@ static int pdo_oci_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{{ *
}

/* password */
if (!dbh->password && vars[3].optval) {
dbh->password = vars[3].optval;
}

if (dbh->password) {
H->last_err = OCIAttrSet(H->session, OCI_HTYPE_SESSION,
dbh->password, (ub4) strlen(dbh->password),
Expand Down