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

[Oracle] Fix list table columns when using external or OS authentication with Oracle #2318

Closed
wants to merge 2 commits into from

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Feb 17, 2016

To use an external authentication mechanism, ef. Oracle Wallet you specify "/" us the db user and "" as the host when calling oci_connect. See http://php.net/manual/de/function.oci-connect.php#refsect1-function.oci-connect-parameters

This works as expected until you try to read the table columns with getListTableColumnsSQL().

Since $database is "/" the code that should limit the listed columns to the given user actually prevents listing anything because a user "/" never exists.

I did not find a test for getListTableColumnsSQL() but am documenting the debug session that lead to this PR at https://github.com/owncloud/core/wiki/%5BWIP%5D-Oracle-Wallet

@Ocramius
Copy link
Member

Requires tests

@butonic
Copy link
Contributor Author

butonic commented Feb 17, 2016

I am on it

@butonic butonic force-pushed the master branch 3 times, most recently from 803c0ef to 62452f3 Compare February 19, 2016 10:30
@butonic
Copy link
Contributor Author

butonic commented Feb 19, 2016

@Ocramius I copied the @group DBAL-831 ... is that correct?

@Ocramius
Copy link
Member

@butonic seems good. Assigning to @deeky666 for review.

),
);
}
/**
Copy link
Member

Choose a reason for hiding this comment

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

Empty newline to be introduced here

@deeky666
Copy link
Member

@butonic @Ocramius hmm another funny Oracle story :D The fix seems a bit akward and tends to lead to additional checks against / database name throughout the code base which sucks. As an alternative fix we could maybe simply adjust the abstract Oracle driver to return null if / is encountered in the connection parameters. This seems also more convenient than coercing / to null from given method arguments. What do you think?

$this->assertEquals($expectedSql, $this->_platform->getListTableColumnsSQL('"test"', $database));
}

public function getReturnsGetListTableColumnsSQL()
Copy link
Member

Choose a reason for hiding this comment

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

We tend to avoid testing against complete SQL strings when it comes to testing getList*() Methods as those tests are fragile and need to be adjusted on every change in SQL. I would rather do a contains check in the string to check whether all_tab_columns or user_tab_columns is present (which is the interesting difference).

Copy link
Member

Choose a reason for hiding this comment

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

Merging this regardless of the fragile test. We may simply rework the test, should some changes break this.

@deeky666 deeky666 added this to the 2.5.5 milestone Feb 19, 2016
@deeky666 deeky666 changed the title fix list table columns when using external or os authentication with oracle [Oracle] Fix list table columns when using external or OS authentication with Oracle Feb 19, 2016
@deeky666
Copy link
Member

Btw this would also automatically fix other getList*() methods which are affected by this issue, too.

@MorrisJobke
Copy link
Contributor

@deeky666 Are there any steps from our side(@butonic and mine ;)) to help out here?

@butonic
Copy link
Contributor Author

butonic commented May 9, 2016

@deeky666 could you comment hero or or on the alternate approach in #2325. What is missing?

Ocramius added a commit that referenced this pull request Sep 9, 2016
@Ocramius Ocramius closed this in a3577e4 Sep 9, 2016
Ocramius added a commit that referenced this pull request Sep 9, 2016
Ocramius added a commit that referenced this pull request Sep 9, 2016
…-or-os-authentication-on-oracledb-2.5' into 2.5

Close #2318
Close DBAL-831
@Ocramius
Copy link
Member

Ocramius commented Sep 9, 2016

Merged, thanks @butonic!

master: a3577e4
2.5: 9f8c05c

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

Successfully merging this pull request may close these issues.

4 participants