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

Add basic tests for SSPI authentication. #557

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@chrullrich
Contributor

chrullrich commented May 10, 2016

  • Both successful and unsuccessful authentication is tested, the latter to ensure that a configuration mistake (such as a "trust" line left in pg_hba.conf) has not caused both tests to succeed when they should have failed.
  • Setting up to run these tests is not entirely (or at all) trivial; it requires running the database server as an account that is capable of SSPI authentication (such as a virtual service account, e.g. "NT SERVICE\PostgreSQL") on both domain member and non-member systems, or a domain user account.
  • Additionally, both pg_hba.conf and, in most cases, pg_ident.conf must be configured. In particular, the user account that runs the tests must be permitted to authenticate as the database role configured in build.properties.
  • The tests are not run when Waffle is disabled. I would have preferred to have a separate option to turn them off even when building with Waffle because the setup is so difficult. I could not think of a way to make Maven do this, mostly because profiles cannot be chained, and profile activation cannot use two variables, for example (!enableWaffle || disableSSPITests).
  • There is an intermittent problem where testUnauthorized() fails because it gets the wrong exception: It expects SQLSTATE 28000 from the server, but sometimes it gets 08001 generated internally in the driver. No idea what causes that. I did not want to blindly accept any error as proof of failed authentication.
@chrullrich

This comment has been minimized.

Contributor

chrullrich commented May 10, 2016

OK, that went as well as could be expected. There clearly needs to be a way to turn the SSPI tests off separately if the Travis checks are ever going to be successful. I just have no idea how to do that.

@vlsi

This comment has been minimized.

Member

vlsi commented May 10, 2016

@chrullrich , do you know if https://www.appveyor.com/ can be used to test SSPI?

@chrullrich

This comment has been minimized.

Contributor

chrullrich commented May 10, 2016

I have never used them, but from reading the documentation it looks like it could work. The build script would have to do the required changes to pg_hba.conf and pg_ident.conf.

They have JDK 1.8 installed, and PostgreSQL 9.[345], but not Maven. There is a sample configuration to install it at http://www.yegor256.com/2015/01/10/windows-appveyor-maven.html .

@codecov-io

This comment has been minimized.

codecov-io commented May 10, 2016

Current coverage is 69.46%

Merging #557 into master will decrease coverage by -0.99%

  1. 4 files (not in diff) in ...va/org/postgresql/xa were modified. more
    • Misses +94
    • Hits -105
  2. 8 files (not in diff) in .../org/postgresql/util were modified. more
    • Misses +62
    • Hits -68
  3. 2 files (not in diff) in .../org/postgresql/sspi were modified. more
    • Misses -2
  4. 2 files (not in diff) in ...postgresql/ssl/jdbc4 were modified. more
    • Misses -12
    • Hits -2
  5. 3 files (not in diff) in ...a/org/postgresql/ssl were modified. more
    • Misses -13
  6. 2 files (not in diff) in .../org/postgresql/osgi were modified. more
    • Misses -1
    • Hits -4
  7. 16 files (not in diff) in .../org/postgresql/jdbc were modified. more
    • Misses -96
    • Hits -264
  8. 3 files (not in diff) in ...postgresql/geometric were modified. more
    • Misses +28
    • Hits -32
  9. 2 files (not in diff) in ...postgresql/ds/common were modified. more
    • Misses -1
    • Hits -6
  10. 2 files (not in diff) in ...va/org/postgresql/ds were modified. more
    • Misses -1
    • Hits -6
@@             master       #557   diff @@
==========================================
  Files           147        147          
  Lines         15498      14930   -568   
  Methods           0          0          
  Messages          0          0          
  Branches       3059       2963    -96   
==========================================
- Hits          10919      10371   -548   
+ Misses         4579       4559    -20   
  Partials          0          0          

Powered by Codecov. Last updated by 766f806...9d12247

Make checkstyle happy.
No idea what the deal is with the list of tests in the class annotation, but
checkstyle would apparently like to see:

@Suite.SuiteClasses({
    SSPITest.class
    })
public class ...
@chrullrich

This comment has been minimized.

Contributor

chrullrich commented May 10, 2016

Travis is now green, but only because it does not run the tests anymore; getting them to run still requires Windows.

In other news, in the initial comment to the PR I claimed that SSPI could only be tested against a server that runs as some service account. This, as it turns out, is not true; it works perfectly fine when the server runs in a console window, as a regular user.

import java.util.Properties;
/*
* These tests require a working SSPI authentication setup

This comment has been minimized.

@vlsi

vlsi May 11, 2016

Member

Javadoc should start with /**.

This comment has been minimized.

@chrullrich

chrullrich May 11, 2016

Contributor

It's not meant to be JavaDoc. Should it be?

This comment has been minimized.

@vlsi

vlsi May 11, 2016

Member

Well, it looks odd when a non-javadoc comment is present in a javadoc location.

It is not that I'm going to read those javadocs in html format. Having them in javadoc format would help to {@link OtherClasses}, keep those in sync with code when refactoring (e.g. from IDE), etc, etc.

* to authenticate as the "sspiusername" in the build
* configuration.
*/
public class SSPITest {

This comment has been minimized.

@vlsi

vlsi May 14, 2016

Member

How about extends org.postgresql.test.jdbc2.BaseTest4 ?

@vlsi vlsi closed this in 16c27b9 May 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment