pivotal-jbarrett commented on a change in pull request #6792:
URL: https://github.com/apache/geode/pull/6792#discussion_r694337581
##########
File path:
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java
##########
@@ -69,19 +69,27 @@ default void init(Properties securityProps) {}
Object authenticate(Properties credentials) throws
AuthenticationFailedException;
/**
- * Authorize the ResourcePermission for a given Principal
+ * Authorize the ResourcePermission for a given valid Principal.
+ * This method only checks permission. Principal expiration is checked by
isExpired method
*
* @param principal The principal that's requesting the permission
* @param permission The permission requested
* @return true if authorized, false if not
*
- * @throw AuthenticationExpiredException if the principal has expired.
*/
- default boolean authorize(Object principal, ResourcePermission permission)
- throws AuthenticationExpiredException {
+ default boolean authorize(Object principal, ResourcePermission permission) {
return true;
}
+ /**
+ * checks if the given principal is expired or not
+ *
+ * @return true if the given principal is expired, false if not
Review comment:
```Javadoc
@return @{code true} if the given principal is expired, otherwise @{code
false}.
```
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
##########
@@ -272,6 +260,11 @@ public void authorize(ResourcePermission context, Subject
currentUser) {
return;
}
+ Object principal = currentUser.getPrincipal();
Review comment:
Can `principal` ever be `null`?
##########
File path:
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java
##########
@@ -69,19 +69,27 @@ default void init(Properties securityProps) {}
Object authenticate(Properties credentials) throws
AuthenticationFailedException;
/**
- * Authorize the ResourcePermission for a given Principal
+ * Authorize the ResourcePermission for a given valid Principal.
+ * This method only checks permission. Principal expiration is checked by
isExpired method
*
* @param principal The principal that's requesting the permission
* @param permission The permission requested
* @return true if authorized, false if not
*
- * @throw AuthenticationExpiredException if the principal has expired.
*/
- default boolean authorize(Object principal, ResourcePermission permission)
- throws AuthenticationExpiredException {
+ default boolean authorize(Object principal, ResourcePermission permission) {
return true;
}
+ /**
+ * checks if the given principal is expired or not
Review comment:
"Checks if the given principal is expired or not."
##########
File path:
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java
##########
@@ -69,19 +69,27 @@ default void init(Properties securityProps) {}
Object authenticate(Properties credentials) throws
AuthenticationFailedException;
/**
- * Authorize the ResourcePermission for a given Principal
+ * Authorize the ResourcePermission for a given valid Principal.
+ * This method only checks permission. Principal expiration is checked by
isExpired method
*
* @param principal The principal that's requesting the permission
* @param permission The permission requested
* @return true if authorized, false if not
*
- * @throw AuthenticationExpiredException if the principal has expired.
*/
- default boolean authorize(Object principal, ResourcePermission permission)
- throws AuthenticationExpiredException {
+ default boolean authorize(Object principal, ResourcePermission permission) {
return true;
}
+ /**
+ * checks if the given principal is expired or not
+ *
+ * @return true if the given principal is expired, false if not
+ */
+ default boolean isExpired(Object principal) {
Review comment:
```java
default boolean isExpired(@NotNull Object principal)
```
##########
File path:
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java
##########
@@ -69,19 +69,27 @@ default void init(Properties securityProps) {}
Object authenticate(Properties credentials) throws
AuthenticationFailedException;
/**
- * Authorize the ResourcePermission for a given Principal
+ * Authorize the ResourcePermission for a given valid Principal.
+ * This method only checks permission. Principal expiration is checked by
isExpired method
Review comment:
This statement is not necessary. No reason so specify what a method
isn't used for.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
##########
@@ -272,6 +260,11 @@ public void authorize(ResourcePermission context, Subject
currentUser) {
return;
}
+ Object principal = currentUser.getPrincipal();
+ if (securityManager.isExpired(principal)) {
+ throw new AuthenticationExpiredException(principal + " expired.");
Review comment:
Given that `Object.toString()` is used to more diagnostic purposes does
it make sense to include that in the message? It may result information leak
about the credential we don't want serialized. Simply throwing this exception
should be information enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
