Skip to content

Commit e7203bf

Browse files
rwinchrwinch
committed
Null safety via JSpecify spring-security-acl
Closes gh-18401
1 parent 42e1e9f commit e7203bf

18 files changed

Lines changed: 147 additions & 68 deletions

‎acl/spring-security-acl.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
plugins {
22
id 'javadoc-warnings-error'
3+
id 'security-nullability'
34
}
45

56
apply plugin: 'io.spring.convention.spring-module'

‎acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
import java.io.Serializable;
2020

21+
import org.jspecify.annotations.Nullable;
22+
2123
import org.springframework.security.acls.model.AccessControlEntry;
2224
import org.springframework.security.acls.model.Acl;
2325
import org.springframework.security.acls.model.AuditableAccessControlEntry;
@@ -36,7 +38,7 @@ public class AccessControlEntryImpl implements AccessControlEntry, AuditableAcce
3638

3739
private Permission permission;
3840

39-
private final Serializable id;
41+
private final @Nullable Serializable id;
4042

4143
private final Sid sid;
4244

@@ -46,7 +48,7 @@ public class AccessControlEntryImpl implements AccessControlEntry, AuditableAcce
4648

4749
private final boolean granting;
4850

49-
public AccessControlEntryImpl(Serializable id, Acl acl, Sid sid, Permission permission, boolean granting,
51+
public AccessControlEntryImpl(@Nullable Serializable id, Acl acl, Sid sid, Permission permission, boolean granting,
5052
boolean auditSuccess, boolean auditFailure) {
5153
Assert.notNull(acl, "Acl required");
5254
Assert.notNull(sid, "Sid required");
@@ -133,7 +135,7 @@ public Acl getAcl() {
133135
}
134136

135137
@Override
136-
public Serializable getId() {
138+
public @Nullable Serializable getId() {
137139
return this.id;
138140
}
139141

‎acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,8 @@ public void securityCheck(Acl acl, int changeType) {
9999
Authentication authentication = context.getAuthentication();
100100
// Check if authorized by virtue of ACL ownership
101101
Sid currentUser = createCurrentUser(authentication);
102-
if (currentUser.equals(acl.getOwner())
102+
Sid owner = acl.getOwner();
103+
if (owner != null && currentUser.equals(owner)
103104
&& ((changeType == CHANGE_GENERAL) || (changeType == CHANGE_OWNERSHIP))) {
104105
return;
105106
}
@@ -108,8 +109,8 @@ public void securityCheck(Acl acl, int changeType) {
108109
Collection<? extends GrantedAuthority> reachableGrantedAuthorities = this.roleHierarchy
109110
.getReachableGrantedAuthorities(authentication.getAuthorities());
110111
Set<String> authorities = AuthorityUtils.authorityListToSet(reachableGrantedAuthorities);
111-
if (acl.getOwner() instanceof GrantedAuthoritySid
112-
&& authorities.contains(((GrantedAuthoritySid) acl.getOwner()).getGrantedAuthority())) {
112+
if (owner instanceof GrantedAuthoritySid
113+
&& authorities.contains(((GrantedAuthoritySid) owner).getGrantedAuthority())) {
113114
return;
114115
}
115116

‎acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import java.util.ArrayList;
2121
import java.util.List;
2222

23+
import org.jspecify.annotations.Nullable;
24+
2325
import org.springframework.security.acls.model.AccessControlEntry;
2426
import org.springframework.security.acls.model.Acl;
2527
import org.springframework.security.acls.model.AuditableAcl;
@@ -41,7 +43,7 @@
4143
*/
4244
public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
4345

44-
private Acl parentAcl;
46+
private @Nullable Acl parentAcl;
4547

4648
private transient AclAuthorizationStrategy aclAuthorizationStrategy;
4749

@@ -54,10 +56,10 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl {
5456
private Serializable id;
5557

5658
// OwnershipAcl
57-
private Sid owner;
59+
private @Nullable Sid owner;
5860

5961
// includes all SIDs the WHERE clause covered, even if there was no ACE for a SID
60-
private List<Sid> loadedSids = null;
62+
private @Nullable List<Sid> loadedSids = null;
6163

6264
private boolean entriesInheriting = true;
6365

@@ -97,8 +99,8 @@ public AclImpl(ObjectIdentity objectIdentity, Serializable id, AclAuthorizationS
9799
* @param owner the owner (required)
98100
*/
99101
public AclImpl(ObjectIdentity objectIdentity, Serializable id, AclAuthorizationStrategy aclAuthorizationStrategy,
100-
PermissionGrantingStrategy grantingStrategy, Acl parentAcl, List<Sid> loadedSids, boolean entriesInheriting,
101-
Sid owner) {
102+
PermissionGrantingStrategy grantingStrategy, @Nullable Acl parentAcl, @Nullable List<Sid> loadedSids,
103+
boolean entriesInheriting, Sid owner) {
102104
Assert.notNull(objectIdentity, "Object Identity required");
103105
Assert.notNull(id, "Id required");
104106
Assert.notNull(aclAuthorizationStrategy, "AclAuthorizationStrategy required");
@@ -117,7 +119,7 @@ public AclImpl(ObjectIdentity objectIdentity, Serializable id, AclAuthorizationS
117119
* Private no-argument constructor for use by reflection-based persistence tools along
118120
* with field-level access.
119121
*/
120-
@SuppressWarnings("unused")
122+
@SuppressWarnings({ "unused", "NullAway.Init" })
121123
private AclImpl() {
122124
}
123125

@@ -199,7 +201,7 @@ public boolean isGranted(List<Permission> permission, List<Sid> sids, boolean ad
199201
}
200202

201203
@Override
202-
public boolean isSidLoaded(List<Sid> sids) {
204+
public boolean isSidLoaded(@Nullable List<Sid> sids) {
203205
// If loadedSides is null, this indicates all SIDs were loaded
204206
// Also return true if the caller didn't specify a SID to find
205207
if ((this.loadedSids == null) || (sids == null) || sids.isEmpty()) {
@@ -238,19 +240,19 @@ public void setOwner(Sid newOwner) {
238240
}
239241

240242
@Override
241-
public Sid getOwner() {
243+
public @Nullable Sid getOwner() {
242244
return this.owner;
243245
}
244246

245247
@Override
246-
public void setParent(Acl newParent) {
248+
public void setParent(@Nullable Acl newParent) {
247249
this.aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL);
248250
Assert.isTrue(newParent == null || !newParent.equals(this), "Cannot be the parent of yourself");
249251
this.parentAcl = newParent;
250252
}
251253

252254
@Override
253-
public Acl getParentAcl() {
255+
public @Nullable Acl getParentAcl() {
254256
return this.parentAcl;
255257
}
256258

‎acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
import java.io.Serializable;
2020

21+
import org.jspecify.annotations.Nullable;
22+
2123
import org.springframework.cache.Cache;
2224
import org.springframework.security.acls.model.AclCache;
2325
import org.springframework.security.acls.model.MutableAcl;
@@ -78,13 +80,13 @@ public void evictFromCache(ObjectIdentity objectIdentity) {
7880
}
7981

8082
@Override
81-
public MutableAcl getFromCache(ObjectIdentity objectIdentity) {
83+
public @Nullable MutableAcl getFromCache(ObjectIdentity objectIdentity) {
8284
Assert.notNull(objectIdentity, "ObjectIdentity required");
8385
return getFromCache((Object) objectIdentity);
8486
}
8587

8688
@Override
87-
public MutableAcl getFromCache(Serializable pk) {
89+
public @Nullable MutableAcl getFromCache(Serializable pk) {
8890
Assert.notNull(pk, "Primary key (identifier) required");
8991
return getFromCache((Object) pk);
9092
}
@@ -101,12 +103,16 @@ public void putInCache(MutableAcl acl) {
101103
this.cache.put(acl.getId(), acl);
102104
}
103105

104-
private MutableAcl getFromCache(Object key) {
106+
private @Nullable MutableAcl getFromCache(Object key) {
105107
Cache.ValueWrapper element = this.cache.get(key);
106108
if (element == null) {
107109
return null;
108110
}
109-
return initializeTransientFields((MutableAcl) element.get());
111+
Object value = element.get();
112+
if (value == null) {
113+
return null;
114+
}
115+
return initializeTransientFields((MutableAcl) value);
110116
}
111117

112118
private MutableAcl initializeTransientFields(MutableAcl value) {

‎acl/src/main/java/org/springframework/security/acls/domain/package-info.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,7 @@
1717
/**
1818
* Basic implementation of access control lists (ACLs) interfaces.
1919
*/
20+
@NullMarked
2021
package org.springframework.security.acls.domain;
22+
23+
import org.jspecify.annotations.NullMarked;

‎acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import org.apache.commons.logging.Log;
2525
import org.apache.commons.logging.LogFactory;
26+
import org.jspecify.annotations.Nullable;
2627

2728
import org.springframework.core.convert.ConversionFailedException;
2829
import org.springframework.core.convert.ConversionService;
@@ -67,10 +68,10 @@ class AclClassIdUtils {
6768
* @return The identifier in the appropriate target Java type. Typically Long or UUID.
6869
* @throws SQLException
6970
*/
70-
Serializable identifierFrom(Serializable identifier, ResultSet resultSet) throws SQLException {
71-
if (isString(identifier) && hasValidClassIdType(resultSet)
72-
&& canConvertFromStringTo(classIdTypeFrom(resultSet))) {
73-
return convertFromStringTo((String) identifier, classIdTypeFrom(resultSet));
71+
@Nullable Serializable identifierFrom(Serializable identifier, ResultSet resultSet) throws SQLException {
72+
Class<? extends Serializable> classIdType = classIdTypeFrom(resultSet);
73+
if (isString(identifier) && classIdType != null && canConvertFromStringTo(classIdType)) {
74+
return convertFromStringTo((String) identifier, classIdType);
7475
}
7576
// Assume it should be a Long type
7677
return convertToLong(identifier);
@@ -86,11 +87,17 @@ private boolean hasValidClassIdType(ResultSet resultSet) {
8687
}
8788
}
8889

89-
private <T extends Serializable> Class<T> classIdTypeFrom(ResultSet resultSet) throws SQLException {
90-
return classIdTypeFrom(resultSet.getString(DEFAULT_CLASS_ID_TYPE_COLUMN_NAME));
90+
private <T extends Serializable> @Nullable Class<T> classIdTypeFrom(ResultSet resultSet) throws SQLException {
91+
try {
92+
return classIdTypeFrom(resultSet.getString(DEFAULT_CLASS_ID_TYPE_COLUMN_NAME));
93+
}
94+
catch (SQLException ex) {
95+
log.debug("Unable to obtain the class id type", ex);
96+
return null;
97+
}
9198
}
9299

93-
private <T extends Serializable> Class<T> classIdTypeFrom(String className) {
100+
private <T extends Serializable> @Nullable Class<T> classIdTypeFrom(String className) {
94101
if (className == null) {
95102
return null;
96103
}
@@ -107,7 +114,7 @@ private <T> boolean canConvertFromStringTo(Class<T> targetType) {
107114
return this.conversionService.canConvert(String.class, targetType);
108115
}
109116

110-
private <T extends Serializable> T convertFromStringTo(String identifier, Class<T> targetType) {
117+
private <T extends Serializable> @Nullable T convertFromStringTo(String identifier, Class<T> targetType) {
111118
return this.conversionService.convert(identifier, targetType);
112119
}
113120

@@ -121,7 +128,7 @@ private <T extends Serializable> T convertFromStringTo(String identifier, Class<
121128
* exception occurred
122129
* @throws IllegalArgumentException if targetType is null
123130
*/
124-
private Long convertToLong(Serializable identifier) {
131+
private @Nullable Long convertToLong(Serializable identifier) {
125132
if (this.conversionService.canConvert(identifier.getClass(), Long.class)) {
126133
return this.conversionService.convert(identifier, Long.class);
127134
}
@@ -140,10 +147,10 @@ void setConversionService(ConversionService conversionService) {
140147
private static class StringToLongConverter implements Converter<String, Long> {
141148

142149
@Override
143-
public Long convert(String identifierAsString) {
150+
public Long convert(@Nullable String identifierAsString) {
144151
if (identifierAsString == null) {
145152
throw new ConversionFailedException(TypeDescriptor.valueOf(String.class),
146-
TypeDescriptor.valueOf(Long.class), null, null);
153+
TypeDescriptor.valueOf(Long.class), identifierAsString, new NullPointerException());
147154

148155
}
149156
return Long.parseLong(identifierAsString);
@@ -154,10 +161,10 @@ public Long convert(String identifierAsString) {
154161
private static class StringToUUIDConverter implements Converter<String, UUID> {
155162

156163
@Override
157-
public UUID convert(String identifierAsString) {
164+
public UUID convert(@Nullable String identifierAsString) {
158165
if (identifierAsString == null) {
159166
throw new ConversionFailedException(TypeDescriptor.valueOf(String.class),
160-
TypeDescriptor.valueOf(UUID.class), null, null);
167+
TypeDescriptor.valueOf(UUID.class), identifierAsString, new NullPointerException());
161168

162169
}
163170
return UUID.fromString(identifierAsString);

‎acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131

3232
import javax.sql.DataSource;
3333

34+
import org.jspecify.annotations.Nullable;
35+
3436
import org.springframework.core.convert.ConversionException;
3537
import org.springframework.core.convert.ConversionService;
3638
import org.springframework.jdbc.core.JdbcTemplate;
@@ -224,7 +226,8 @@ private void setAces(AclImpl acl, List<AccessControlEntryImpl> aces) {
224226
* @param findNow Long-based primary keys to retrieve
225227
* @param sids
226228
*/
227-
private void lookupPrimaryKeys(final Map<Serializable, Acl> acls, final Set<Long> findNow, final List<Sid> sids) {
229+
private void lookupPrimaryKeys(final Map<Serializable, Acl> acls, final Set<Long> findNow,
230+
final @Nullable List<Sid> sids) {
228231
Assert.notNull(acls, "ACLs are required");
229232
Assert.notEmpty(findNow, "Items to find now required");
230233
String sql = computeRepeatingSql(this.lookupPrimaryKeysWhereClause, findNow.size());
@@ -264,7 +267,7 @@ private void setKeys(PreparedStatement ps, Set<Long> findNow) throws SQLExceptio
264267
* automatically create entries if required)
265268
*/
266269
@Override
267-
public final Map<ObjectIdentity, Acl> readAclsById(List<ObjectIdentity> objects, List<Sid> sids) {
270+
public final Map<ObjectIdentity, Acl> readAclsById(List<ObjectIdentity> objects, @Nullable List<Sid> sids) {
268271
Assert.isTrue(this.batchSize >= 1, "BatchSize must be >= 1");
269272
Assert.notEmpty(objects, "Objects to lookup required");
270273
// Map<ObjectIdentity,Acl>
@@ -323,7 +326,7 @@ public final Map<ObjectIdentity, Acl> readAclsById(List<ObjectIdentity> objects,
323326
* properly-configured parent ACLs.
324327
*/
325328
private Map<ObjectIdentity, Acl> lookupObjectIdentities(final Collection<ObjectIdentity> objectIdentities,
326-
List<Sid> sids) {
329+
@Nullable List<Sid> sids) {
327330
Assert.notEmpty(objectIdentities, "Must provide identities to lookup");
328331

329332
// contains Acls with StubAclParents
@@ -399,8 +402,10 @@ private AclImpl convert(Map<Serializable, Acl> inputMap, Long currentIdentity) {
399402
}
400403

401404
// Now we have the parent (if there is one), create the true AclImpl
405+
Sid owner = inputAcl.getOwner();
406+
Assert.isTrue(owner != null, "Owner is required");
402407
AclImpl result = new AclImpl(inputAcl.getObjectIdentity(), inputAcl.getId(), this.aclAuthorizationStrategy,
403-
this.grantingStrategy, parent, null, inputAcl.isEntriesInheriting(), inputAcl.getOwner());
408+
this.grantingStrategy, parent, null, inputAcl.isEntriesInheriting(), owner);
404409

405410
// Copy the "aces" from the input to the destination
406411

@@ -506,9 +511,9 @@ private class ProcessResultSet implements ResultSetExtractor<Set<Long>> {
506511

507512
private final Map<Serializable, Acl> acls;
508513

509-
private final List<Sid> sids;
514+
private final @Nullable List<Sid> sids;
510515

511-
ProcessResultSet(Map<Serializable, Acl> acls, List<Sid> sids) {
516+
ProcessResultSet(Map<Serializable, Acl> acls, @Nullable List<Sid> sids) {
512517
Assert.notNull(acls, "ACLs cannot be null");
513518
this.acls = acls;
514519
this.sids = sids; // can be null
@@ -579,6 +584,9 @@ private void convertCurrentResultIntoObject(Map<Serializable, Acl> acls, ResultS
579584
// target id type, e.g. UUID.
580585
Serializable identifier = (Serializable) rs.getObject("object_id_identity");
581586
identifier = BasicLookupStrategy.this.aclClassIdUtils.identifierFrom(identifier, rs);
587+
if (identifier == null) {
588+
throw new IllegalStateException("Identifier cannot be null");
589+
}
582590
ObjectIdentity objectIdentity = BasicLookupStrategy.this.objectIdentityGenerator
583591
.createObjectIdentity(identifier, rs.getString("class"));
584592

@@ -670,7 +678,7 @@ public boolean isGranted(List<Permission> permission, List<Sid> sids, boolean ad
670678
}
671679

672680
@Override
673-
public boolean isSidLoaded(List<Sid> sids) {
681+
public boolean isSidLoaded(@Nullable List<Sid> sids) {
674682
throw new UnsupportedOperationException("Stub only");
675683
}
676684

0 commit comments

Comments
 (0)