ACL: acl_class class vs class_id_type for BasicLookupStrategy conflict

Summary

Hi! I'm trying to use ACL with Spring Boot.

Actual Behavior

I used the schemas as specified here (in this case H2) and I try to use the BasicLookupStrategy. I tried both names, class and class_id_type for the table acl_class, neither work out of the box because BasicLookupStrategy tries to use class_id_type AND class, but the schema above specifies class for the column name. Now here's where the strategy tries to actually use both column names:

https://github.com/spring-projects/spring-security/blob/c1db1aad91837d571ab3d621c8e470d67c7635c6/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java#L96

https://github.com/spring-projects/spring-security/blob/c1db1aad91837d571ab3d621c8e470d67c7635c6/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java#L110

Is the schema missing something? Am I supposed to implement something else? I don't want to implement my own LookupStrategy just for this.

Expected Behavior

No schema issues, both calls should succeed (see sample).

Configuration

Version

5.2.0.RELEASE

Sample

Leaving the column named class causes:

org.h2.jdbc.JdbcSQLSyntaxErrorException: Column "class_id_type" not found [42122-200]
    at org.h2.message.DbException.getJdbcSQLException(DbException.java:453) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.message.DbException.getJdbcSQLException(DbException.java:429) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.message.DbException.get(DbException.java:205) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.message.DbException.get(DbException.java:181) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.jdbc.JdbcResultSet.getColumnIndex(JdbcResultSet.java:3169) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.jdbc.JdbcResultSet.get(JdbcResultSet.java:3268) ~[h2-1.4.200.jar:1.4.200]
    at org.h2.jdbc.JdbcResultSet.getString(JdbcResultSet.java:316) ~[h2-1.4.200.jar:1.4.200]
    at com.zaxxer.hikari.pool.HikariProxyResultSet.getString(HikariProxyResultSet.java) ~[HikariCP-3.4.1.jar:na]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.classIdTypeFrom(AclClassIdUtils.java:88) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.hasValidClassIdType(AclClassIdUtils.java:80) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.identifierFrom(AclClassIdUtils.java:65) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.convertCurrentResultIntoObject(BasicLookupStrategy.java:634) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.extractData(BasicLookupStrategy.java:583) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.extractData(BasicLookupStrategy.java:558) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.jdbc.core.JdbcTemplate$1.doInPreparedStatement(JdbcTemplate.java:679) ~[spring-jdbc-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.jdbc.core.JdbcTemplate.execute(JdbcTemplate.java:617) ~[spring-jdbc-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.jdbc.core.JdbcTemplate.query(JdbcTemplate.java:669) ~[spring-jdbc-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.jdbc.core.JdbcTemplate.query(JdbcTemplate.java:700) ~[spring-jdbc-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy.lookupObjectIdentities(BasicLookupStrategy.java:381) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy.readAclsById(BasicLookupStrategy.java:336) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.JdbcAclService.readAclsById(JdbcAclService.java:129) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.JdbcAclService.readAclById(JdbcAclService.java:111) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.JdbcAclService.readAclById(JdbcAclService.java:119) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at org.springframework.security.acls.jdbc.JdbcMutableAclService.createAcl(JdbcMutableAclService.java:122) ~[spring-security-acl-5.2.0.RELEASE.jar:5.2.0.RELEASE]
    at de.redacted.data.service.PermissionService.mutableAclFactory(PermissionService.java:33) ~[classes/:na]

schema:

create table acl_class
(
    id    bigint generated by default as identity (start with 100) not null primary key,
    class varchar_ignorecase(100)                                  not null,
    constraint unique_uk_2 unique (class)
);

Renaming column to class_id_type causes:

org.springframework.jdbc.BadSqlGrammarException: PreparedStatementCallback; bad SQL grammar [select acl_object_identity.object_id_identity, acl_entry.ace_order,  acl_object_identity.id as acl_id, acl_object_identity.parent_object, acl_object_identity.entries_inheriting, acl_entry.id as ace_id, acl_entry.mask,  acl_entry.granting,  acl_entry.audit_success, acl_entry.audit_failure,  acl_sid.principal as ace_principal, acl_sid.sid as ace_sid,  acli_sid.principal as acl_principal, acli_sid.sid as acl_sid, acl_class.class from acl_object_identity left join acl_sid acli_sid on acli_sid.id = acl_object_identity.owner_sid left join acl_class on acl_class.id = acl_object_identity.object_id_class   left join acl_entry on acl_object_identity.id = acl_entry.acl_object_identity left join acl_sid on acl_entry.sid = acl_sid.id  where ( (acl_object_identity.object_id_identity = ? and acl_class.class = ?)) order by acl_object_identity.object_id_identity asc, acl_entry.ace_order asc]; nested exception is org.h2.jdbc.JdbcSQLSyntaxErrorException: Column "ACL_CLASS.CLASS" not found; SQL statement:
select acl_object_identity.object_id_identity, acl_entry.ace_order,  acl_object_identity.id as acl_id, acl_object_identity.parent_object, acl_object_identity.entries_inheriting, acl_entry.id as ace_id, acl_entry.mask,  acl_entry.granting,  acl_entry.audit_success, acl_entry.audit_failure,  acl_sid.principal as ace_principal, acl_sid.sid as ace_sid,  acli_sid.principal as acl_principal, acli_sid.sid as acl_sid, acl_class.class from acl_object_identity left join acl_sid acli_sid on acli_sid.id = acl_object_identity.owner_sid left join acl_class on acl_class.id = acl_object_identity.object_id_class   left join acl_entry on acl_object_identity.id = acl_entry.acl_object_identity left join acl_sid on acl_entry.sid = acl_sid.id  where ( (acl_object_identity.object_id_identity = ? and acl_class.class = ?)) order by acl_object_identity.object_id_identity asc, acl_entry.ace_order asc [42122-200]

with code:

final ObjectIdentity objectIdentity = new ObjectIdentityImpl(targetObj.getClass(), targetObj.getId());
final MutableAcl acl = mutableAclFactory(objectIdentity);

    public MutableAcl mutableAclFactory(ObjectIdentity objectIdentity) {
        try {
            return (MutableAcl) aclService.readAclById(objectIdentity);
        } catch (final NotFoundException e) {
            return aclService.createAcl(objectIdentity);
        }
    }

schema:

create table acl_class
(
    id    bigint generated by default as identity (start with 100) not null primary key,
    class_id_type varchar_ignorecase(100)                                  not null,
    constraint unique_uk_2 unique (class_id_type)
);

That being said, AclClassIdUtils also uses class_id_type: https://github.com/spring-projects/spring-security/blob/c1db1aad91837d571ab3d621c8e470d67c7635c6/acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java#L39

Sorry to hear you are having trouble, @ChristianSch.

Since BasicLookupStrategy doesn't include class_id_type by default, I believe the correct fix here is to change BasicLookupStrategy to not collaborate with AclClassIdUtils by default.

I think these lines:

Serializable identifier = (Serializable) rs.getObject("object_id_identity");
identifier = aclClassIdUtils.identifierFrom(identifier, rs);

Are the ones in error since they assume that BasicLookupStrategy wants to work with AclClassIdUtils (and by association class_id_type) by default.

A better approach might be:

Serializable identifier;
if (usingDefaultSelectClause) {
    identifier = rs.getLong("object_id_identity");
} else {
    identifier = (Serializable) rs.getObject("object_id_identity");
    identifier = aclClassIdUtils.identifierFrom(identifier, rs);
}

Since the original behavior before #4424 was to treat the object_id_identity column as a long.

Would you be interested in submitting a PR to fix the problem?

A possible workaround in the meantime might be for you to have both class and class_id_type columns in your table.

Hi, Thanks for getting back to me. hasPermission doesn't really work for me, I don't know why yet. Neither adding both columns to the table acl_class nor your proprosal for a fix (well identifier = rs.getLong("object_id_identity");) works for me, so I can't even validate any fixes. I'm quite time constrained and need to get this done, but I am interested in a proper fix. So don't wait for me to come around, but I might if I find out why nothing works atm and get a PR done.

/edit: I found out why my implementation didn't work. First it was using the wrong beans, then I didn't implement UserDetails on my user but in a separate UserPrincipal, which was causing the Object.toString to be looked up by the lib (what horrible and bad practice imho), then I added the permission for targetObj.getClass() instead of targetObj.getClass().getSimpleName() while having hasPermission(#id, 'SimpleName', $permission). Sooo many gotchas which the documentation doesn't seem to do justice and the (few) proper reference implementations do wrong (like the baeldung one, which is over two years old tho).

I'll need to catch up on my stuff and see what I can gather from this and see if I can contribute something. But as I said, don't wait for it.

Glad you got things working, @ChristianSch.

As a side note, the reference documentation is something we're working on improving - feel free to file a ticket if you have specific recommendations. Regarding the Baeldung article, I know that they are pretty open to you opening issues on their repo so they can update their articles accordingly.

I have the same issue on Postgrees (spring-boot 2.2.2). I really like the idea, but duo to the lack of proper documentation I moved toward implementing my own PermissionEvaluator for ACL.

org.postgresql.util.PSQLException: The column name class_id_type was not found in this ResultSet.
    at org.postgresql.jdbc.PgResultSet.findColumn(PgResultSet.java:2584) ~[postgresql-42.2.8.jar:42.2.8]
    at org.postgresql.jdbc.PgResultSet.getString(PgResultSet.java:2457) ~[postgresql-42.2.8.jar:42.2.8]
    at com.zaxxer.hikari.pool.HikariProxyResultSet.getString(HikariProxyResultSet.java) ~[HikariCP-3.4.1.jar:na]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.classIdTypeFrom(AclClassIdUtils.java:88) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.hasValidClassIdType(AclClassIdUtils.java:80) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]
    at org.springframework.security.acls.jdbc.AclClassIdUtils.identifierFrom(AclClassIdUtils.java:65) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.convertCurrentResultIntoObject(BasicLookupStrategy.java:634) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.extractData(BasicLookupStrategy.java:583) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]
    at org.springframework.security.acls.jdbc.BasicLookupStrategy$ProcessResultSet.extractData(BasicLookupStrategy.java:558) ~[spring-security-acl-5.2.1.RELEASE.jar:5.2.1.RELEASE]

I think the solutions proposed above are potentially cumbersome. I was able to solve this differently.

You must have both the class and the class_id_type columns. This is missing from the DDL.

CREATE TABLE IF NOT EXISTS acl_class (
  id BIGINT NOT NULL AUTO_INCREMENT,
  class varchar(255) NOT NULL,
  class_id_type varchar(255),
  PRIMARY KEY (id),
  UNIQUE KEY unique_uk_2 (class)
);

In addition, when using a non-Long ObjectIdentity, you have to set the following:

jdbcMutableAclService.setAclClassIdSupported(true);
lookupStrategy.setAclClassIdSupported(true);

and your DDL for acl_object_identity must change to:

CREATE TABLE IF NOT EXISTS acl_object_identity (
  id BIGINT NOT NULL AUTO_INCREMENT,
  object_id_class BIGINT NOT NULL,
  object_id_identity varchar(255) NOT NULL,
  parent_object BIGINT DEFAULT NULL,
  owner_sid BIGINT DEFAULT NULL,
  entries_inheriting tinyint(1) NOT NULL,
  PRIMARY KEY (id),
  UNIQUE KEY unique_uk_3 (object_id_class,object_id_identity)
);

Reference documentation update request: (https://github.com/spring-projects/spring-security/issues/7978)

You must have both the class and the class_id_type columns.

@jonathan-graf I think updating the documentation is a good idea. I also think that remaining passive is good, too.

We didn't require two columns in the past, and we shouldn't suddenly require folks who upgrade to add a new column if we can help it. IMO, you should only need the class_id_type if you need to customize the type.

I think the solutions proposed above are potentially cumbersome.

Could you expand on this? It's okay for a framework to do cumbersome work so that the app doesn't have to. My proposal is meant to place the upgrade burden on the framework so that the application's upgrade story is simpler.

What might not be clear is that my proposal is how to change BasicLookupStrategy, not how applications should change their behavior.

Hi Josh, thanks for getting back to me. I totally agree with you. Upgrading users should not need to add a new column. Only users that want to use UUIDs as identifiers need the new column.

I think maybe I didn't describe myself well enough because I am not proposing any new solutions or changes to the framework. The framework code references the class_id_type column already. This issue description describes a database error where the column is missing. I do not believe this is a bug. This is due to outdated documentation. No framework changes need to be made in order for a new application to support UUID as ObjectIdentifier. If existing users wish to continue using Longs as identifiers, then they need not add the database column class_id_type. I have tested both scenarios in version 5.2.2.

To further clarify: the framework, as of version 5.2.2 works. I have not needed to make any changes to the framework to allow UUIDs to be stored as the identifier in the ObjectIdentity.

That's why I'm simply recommending that we update the documentation to inform the public that the framework already supports UUIDs as ObjectIdentifiers. I am not sure how changing BasicLookupStrategy offers much of an enhancement, again, because I have already successfully implemented this as-is.

spring-projects / spring-security