GeoServer

Broken File/URL conversions in security

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 2.2.x, 2.2-beta2
  • Component/s: Security
  • Labels:
    None
  • Environment:
    Maven 3
  • Number of attachments :
    1

Description

The new security password provider is broken in any data directory that contains special characters such as spaces, causing the build to fail:

Tests in error: 
  testEncryption(org.geoserver.security.password.URLMasterPasswordProviderTest): /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/target/passwd1191987093163736111tmp (No such file or directory)
  testMasterPasswordChange(org.geoserver.security.password.MasterPasswordChangeTest): java.io.FileNotFoundException: /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/./target/mock2049237527189537102data/security/mpw1.properties (No such file or directory)

Deployments will also be affected. The underlying problem is code like:

File f = new File(url.getFile());

in URLMasterpasswordProvider that does not use DataUtilities as required by the project conventions:

http://docs.geotools.org/latest/developer/conventions/code/url.html

  1. Text File
    GEOS-5041.patch
    09/Apr/12 11:54 PM
    5 kB
    Ben Caradoc-Davies

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Ben Caradoc-Davies added a comment -

Justin, there was a time when you configured Hudson to build in a path with spaces. I think that was a great way of catching problems like this.

Show
Ben Caradoc-Davies added a comment - Justin, there was a time when you configured Hudson to build in a path with spaces. I think that was a great way of catching problems like this.
Ben Caradoc-Davies made changes -
Field Original Value New Value
Assignee Andrea Aime [ aaime ] Justin Deoliveira [ jdeolive ]
Hide
Permalink
Ben Caradoc-Davies added a comment -

Justin, please find attached a patch that fixes these failures.

Note that I may have been a bit aggressive in my use of getCanonicalFile in MasterPasswordChangeTest (the test passes both with and without them); this may reduce your test coverage of relative file URLs for password provider configuration. I am not quite sure of your intent in these tests. By comparison, in URLMasterPasswordProviderTest you explicitly canonicalise the file path before using it. Different code paths will be used for relative and absolute paths, with and without a protocol.

Show
Ben Caradoc-Davies added a comment - Justin, please find attached a patch that fixes these failures. Note that I may have been a bit aggressive in my use of getCanonicalFile in MasterPasswordChangeTest (the test passes both with and without them); this may reduce your test coverage of relative file URLs for password provider configuration. I am not quite sure of your intent in these tests. By comparison, in URLMasterPasswordProviderTest you explicitly canonicalise the file path before using it. Different code paths will be used for relative and absolute paths, with and without a protocol.
Ben Caradoc-Davies made changes -
Attachment GEOS-5041.patch [ 59513 ]
Hide
Permalink
Ben Caradoc-Davies added a comment -

Even with this patch I also see multiple test failures in jdbc-sec. Might need another Jira issue:

Results :

Failed tests:
testConfiguration(org.geoserver.security.jdbc.H2UserDetailsServiceTest): org.h2.jdbc.JdbcSQLException: Table USERS already exists; SQL statement:
create table users(name varchar(128) not null,password varchar(254), enabled char(1) not null, primary key(name)) [42101-119]

Tests in error:
testWrapRoleService(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: PRIMARY_KEY_7 ON PUBLIC.GROUPS(NAME); SQL statement:
insert into groups(name ,enabled) values (?,?) [23001-119]
testWrapUserGroupService(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testHideAdminRole(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testHideGroups(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testRoleServiceReadOnly(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testCreateNewUser(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testAssignUserToGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testRemoveUserInGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testRemoveUserNotInGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists
testRoleDatabaseSetup(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testRemove(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testInsert(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testModify(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testIsModified(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testUserGroupDatabaseSetup(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testRemove(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testInsert(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testModify(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testIsModified(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
testEmptyPassword(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity

Show
Ben Caradoc-Davies added a comment - Even with this patch I also see multiple test failures in jdbc-sec. Might need another Jira issue: Results : Failed tests: testConfiguration(org.geoserver.security.jdbc.H2UserDetailsServiceTest): org.h2.jdbc.JdbcSQLException: Table USERS already exists; SQL statement: create table users(name varchar(128) not null,password varchar(254), enabled char(1) not null, primary key(name)) [42101-119] Tests in error: testWrapRoleService(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: PRIMARY_KEY_7 ON PUBLIC.GROUPS(NAME); SQL statement: insert into groups(name ,enabled) values (?,?) [23001-119] testWrapUserGroupService(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testHideAdminRole(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testHideGroups(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testRoleServiceReadOnly(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testCreateNewUser(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testAssignUserToGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testRemoveUserInGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testRemoveUserNotInGroup(org.geoserver.security.jdbc.JDBCGroupAdminServiceTest): User/group service gaugs already exists testRoleDatabaseSetup(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testRemove(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testInsert(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testModify(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testIsModified(org.geoserver.security.jdbc.DB2RoleServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testUserGroupDatabaseSetup(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testRemove(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testInsert(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testModify(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testIsModified(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity testEmptyPassword(org.geoserver.security.jdbc.PostGisUserGroupServiceTest): org.geoserver.data.test.LiveData cannot be cast to org.geoserver.security.jdbc.LiveDbmsDataSecurity
Ben Caradoc-Davies made changes -
Description Th new security password provider is broken in any data directory that contains special characters such as spaces, causing the build to fail:

{noformat}
Tests in error:
  testEncryption(org.geoserver.security.password.URLMasterPasswordProviderTest): /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/target/passwd1191987093163736111tmp (No such file or directory)
  testMasterPasswordChange(org.geoserver.security.password.MasterPasswordChangeTest): java.io.FileNotFoundException: /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/./target/mock2049237527189537102data/security/mpw1.properties (No such file or directory)
{noformat}

Deployments will also be affected. The underlying problem is code like:

{noformat}
File f = new File(url.getFile());
{noformat}

in URLMasterpasswordProvider that does not use DataUtilities as required by the project conventions:

http://docs.geotools.org/latest/developer/conventions/code/url.html 		The new security password provider is broken in any data directory that contains special characters such as spaces, causing the build to fail:

{noformat}
Tests in error:
  testEncryption(org.geoserver.security.password.URLMasterPasswordProviderTest): /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/target/passwd1191987093163736111tmp (No such file or directory)
  testMasterPasswordChange(org.geoserver.security.password.MasterPasswordChangeTest): java.io.FileNotFoundException: /home/car605/geoserver/src%20with%20spaces/geoserver-trunk/src/main/./target/mock2049237527189537102data/security/mpw1.properties (No such file or directory)
{noformat}

Deployments will also be affected. The underlying problem is code like:

{noformat}
File f = new File(url.getFile());
{noformat}

in URLMasterpasswordProvider that does not use DataUtilities as required by the project conventions:

http://docs.geotools.org/latest/developer/conventions/code/url.html
Hide
Permalink
Justin Deoliveira added a comment -

Looks good to me Ben, commit these fixes as you please. Also feel free to just commit the fixes in the jdbc security module. As for the master password change test i do believe the intension is to test with a relative fiel url... so if we can maintain that it would be ideal.

Show
Justin Deoliveira added a comment - Looks good to me Ben, commit these fixes as you please. Also feel free to just commit the fixes in the jdbc security module. As for the master password change test i do believe the intension is to test with a relative fiel url... so if we can maintain that it would be ideal.
Hide
Permalink
Ben Caradoc-Davies added a comment -

OK, will do. Looks like the jdbc-sec failures are in Maven3 only (32 and 64 bit). I am running tests with Maven 2 to confirm they all pass and then will commit the patch (removing my added getCanonicalFiles in MasterPasswordChangeTest). I will raise the Maven 3 failures as a separate issue.

Show
Ben Caradoc-Davies added a comment - OK, will do. Looks like the jdbc-sec failures are in Maven3 only (32 and 64 bit). I am running tests with Maven 2 to confirm they all pass and then will commit the patch (removing my added getCanonicalFiles in MasterPasswordChangeTest). I will raise the Maven 3 failures as a separate issue.
Hide
Permalink
Ben Caradoc-Davies added a comment -

Committed in r16912 on trunk (with the two added getCanonicalFiles in MasterPasswordChangeTest omitted as requested).

Show
Ben Caradoc-Davies added a comment - Committed in r16912 on trunk (with the two added getCanonicalFiles in MasterPasswordChangeTest omitted as requested).
Hide
Permalink
Ben Caradoc-Davies added a comment -

Fixed.

Show
Ben Caradoc-Davies added a comment - Fixed.
Ben Caradoc-Davies made changes -
Status Open [ 1 ] Resolved [ 5 ]
Resolution Fixed [ 1 ]
Hide
Permalink
Ben Caradoc-Davies added a comment -

The sec-jdbc failures are GEOS-5043.

Show
Ben Caradoc-Davies added a comment - The sec-jdbc failures are GEOS-5043 .
Ben Caradoc-Davies made changes -
Status Resolved [ 5 ] Closed [ 6 ]
Ben Caradoc-Davies made changes -
Resolution Fixed [ 1 ]
Status Closed [ 6 ] Reopened [ 4 ]
Assignee Justin Deoliveira [ jdeolive ] Ben Caradoc-Davies [ bencaradocdavies ]
Ben Caradoc-Davies made changes -
Status Reopened [ 4 ] Resolved [ 5 ]
Fix Version/s 2.2.x [ 17106 ]
Fix Version/s 2.2-beta2 [ 18437 ]
Resolution Fixed [ 1 ]
Hide
Permalink
Andrea Aime added a comment -

Switching all issues that have been in "resolved" state for more than one month without further comments to "closed" status

Show
Andrea Aime added a comment - Switching all issues that have been in "resolved" state for more than one month without further comments to "closed" status
Andrea Aime made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

  • Assignee:
    Ben Caradoc-Davies
    Reporter:
    Ben Caradoc-Davies
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: