dlmarion commented on code in PR #6423:
URL: https://github.com/apache/accumulo/pull/6423#discussion_r3413103429
##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -549,23 +548,23 @@ public Map<String,String> getNamespaceConfiguration(TInfo
tinfo, TCredentials cr
@Override
public Map<String,String> getNamespaceProperties(TInfo tinfo, TCredentials
credentials, String ns)
- throws TException {
+ throws ThriftTableOperationException, ThriftSecurityException {
NamespaceId namespaceId;
try {
namespaceId = Namespaces.getNamespaceId(context, ns);
checkNamespacePermission(credentials, namespaceId,
NamespacePermission.ALTER_NAMESPACE);
return context.getPropStore().get(NamespacePropKey.of(context,
namespaceId)).asMap();
} catch (NamespaceNotFoundException e) {
- String why = "Could not find namespace while getting configuration.";
+ String why = "Could not find namespace while getting configuration: " +
e.getMessage();
Review Comment:
```suggestion
String why = "Could not find namespace " + ns + " while getting
configuration: " + e.getMessage();
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java:
##########
@@ -368,10 +369,17 @@ public void tabletServerStopping(TInfo tinfo,
TCredentials credentials, String t
@Override
public void reportSplitExtent(TInfo info, TCredentials credentials, String
serverName,
- TabletSplit split) throws ThriftSecurityException {
- if (!manager.security.canPerformSystemActions(credentials)) {
- throw new ThriftSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED);
+ TabletSplit split) {
+ try {
+ if (!manager.security.canPerformSystemActions(credentials)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
+ }
+ } catch (ThriftSecurityException e) {
+ // this oneway method can't throw a ThriftSecurityException
+ // convert it to a runtime exception, so at least it gets logged on the
server
Review Comment:
Comment seems inaccurate
##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -109,7 +108,7 @@ public static NamespaceId checkNamespaceId(ClientContext
context, String namespa
namespaceId = Namespaces.lookupNamespaceId(context, namespaceName);
if (namespaceId == null) {
throw new ThriftTableOperationException(null, namespaceName, operation,
- TableOperationExceptionType.NAMESPACE_NOTFOUND, null);
+ TableOperationExceptionType.NAMESPACE_NOTFOUND, "Namespace does
not exist");
Review Comment:
```suggestion
TableOperationExceptionType.NAMESPACE_NOTFOUND, "Namespace " +
namespaceName + " does not exist");
```
##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -574,7 +573,7 @@ public TVersionedProperties
getVersionedNamespaceProperties(TInfo tinfo, TCreden
.map(vProps -> new TVersionedProperties(vProps.getDataVersion(),
vProps.asMap()))
.orElseThrow();
} catch (NamespaceNotFoundException e) {
- String why = "Could not find namespace while getting configuration.";
+ String why = "Could not find namespace while getting configuration: " +
e.getMessage();
Review Comment:
```suggestion
String why = "Could not find namespace " + ns + " while getting
configuration: " + e.getMessage();
```
##########
server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java:
##########
@@ -525,18 +523,19 @@ public List<TDiskUsage> getDiskUsage(Set<String> tables,
TCredentials credential
return retUsages;
} catch (TableNotFoundException e) {
- throw new TException(e);
+ throw new ThriftTableOperationException(null, e.getTableName(), null,
+ TableOperationExceptionType.NOTFOUND, e.getMessage());
}
}
@Override
public Map<String,String> getNamespaceConfiguration(TInfo tinfo,
TCredentials credentials,
- String ns) throws ThriftTableOperationException, TException {
+ String ns) throws ThriftTableOperationException, ThriftSecurityException
{
NamespaceId namespaceId;
try {
namespaceId = Namespaces.getNamespaceId(context, ns);
} catch (NamespaceNotFoundException e) {
- String why = "Could not find namespace while getting configuration.";
+ String why = "Could not find namespace while getting configuration: " +
e.getMessage();
Review Comment:
```suggestion
String why = "Could not find namespace " + ns + " while getting
configuration: " + e.getMessage();
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:
##########
@@ -1470,12 +1476,17 @@ public TExternalCompactionJob
reserveCompactionJob(TInfo tinfo, TCredentials cre
@Override
public void compactionJobFinished(TInfo tinfo, TCredentials credentials,
- String externalCompactionId, TKeyExtent extent, long fileSize, long
entries)
- throws ThriftSecurityException, TException {
+ String externalCompactionId, TKeyExtent extent, long fileSize, long
entries) {
- if (!security.canPerformSystemActions(credentials)) {
- throw new AccumuloSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED).asThriftException();
+ try {
+ if (!security.canPerformSystemActions(credentials)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
+ }
+ } catch (ThriftSecurityException e) {
+ log.warn(
+ "Received compaction job finished message (id: {}) from user ({})
with bad credentials",
+ externalCompactionId, credentials.getPrincipal());
}
Review Comment:
The error is not being logged, so we don't really need to create it.
This is a oneway method, so it needs a return when this fails.
```suggestion
if (!security.canPerformSystemActions(credentials)) {
log.warn(
"Received compaction job finished message (id: {}) from user ({})
with bad credentials",
externalCompactionId, credentials.getPrincipal());
return;
}
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:
##########
@@ -1485,10 +1496,16 @@ public void compactionJobFinished(TInfo tinfo,
TCredentials credentials,
@Override
public void compactionJobFailed(TInfo tinfo, TCredentials credentials,
- String externalCompactionId, TKeyExtent extent) throws TException {
- if (!security.canPerformSystemActions(credentials)) {
- throw new AccumuloSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED).asThriftException();
+ String externalCompactionId, TKeyExtent extent) {
+ try {
+ if (!security.canPerformSystemActions(credentials)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
+ }
+ } catch (ThriftSecurityException e) {
+ log.warn(
+ "Received compaction job failed message (id: {}) from user ({}) with
bad credentials",
+ externalCompactionId, credentials.getPrincipal());
}
Review Comment:
Apply same comment as above. No need to create exception as it's not logged.
Need to return here.
##########
server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java:
##########
@@ -391,10 +399,18 @@ public void reportSplitExtent(TInfo info, TCredentials
credentials, String serve
@Override
public void reportTabletStatus(TInfo info, TCredentials credentials, String
serverName,
- TabletLoadState status, TKeyExtent ttablet) throws
ThriftSecurityException {
- if (!manager.security.canPerformSystemActions(credentials)) {
- throw new ThriftSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED);
+ TabletLoadState status, TKeyExtent ttablet) {
+ try {
+ if (!manager.security.canPerformSystemActions(credentials)) {
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
+ }
+ } catch (ThriftSecurityException e) {
+ // this oneway method can't throw a ThriftSecurityException
+ // convert it to a runtime exception, so at least it gets logged on the
server
Review Comment:
Comment seems inaccurate
##########
server/compactor/src/main/java/org/apache/accumulo/compactor/Compactor.java:
##########
@@ -439,17 +433,19 @@ private void cancel(String externalCompactionId) throws
TException {
@Override
public void cancel(TInfo tinfo, TCredentials credentials, String
externalCompactionId)
- throws TException {
+ throws ThriftSecurityException, UnknownCompactionIdException {
TableId tableId = JOB_HOLDER.getTableId();
try {
NamespaceId nsId = getContext().getNamespaceId(tableId);
if (!getContext().getSecurityOperation().canCompact(credentials,
tableId, nsId)) {
- throw new AccumuloSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED).asThriftException();
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
}
} catch (TableNotFoundException e) {
- throw new ThriftTableOperationException(tableId.canonical(), null,
- TableOperation.COMPACT_CANCEL, TableOperationExceptionType.NOTFOUND,
e.getMessage());
+ // if the table is deleted, there is no way to check user permissions to
cancel
+ // the system should handle this automatically
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
Review Comment:
Same comment here, I think the original exception is correct.
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java:
##########
@@ -880,18 +876,20 @@ public TExternalCompactionList
getCompletedCompactions(TInfo tinfo, TCredentials
@Override
public void cancel(TInfo tinfo, TCredentials credentials, String
externalCompactionId)
- throws TException {
+ throws ThriftSecurityException {
var runningCompaction =
RUNNING_CACHE.get(ExternalCompactionId.of(externalCompactionId));
var extent = KeyExtent.fromThrift(runningCompaction.getJob().getExtent());
try {
NamespaceId nsId = getContext().getNamespaceId(extent.tableId());
if (!security.canCompact(credentials, extent.tableId(), nsId)) {
- throw new AccumuloSecurityException(credentials.getPrincipal(),
- SecurityErrorCode.PERMISSION_DENIED).asThriftException();
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
}
} catch (TableNotFoundException e) {
- throw new ThriftTableOperationException(extent.tableId().canonical(),
null,
- TableOperation.COMPACT_CANCEL, TableOperationExceptionType.NOTFOUND,
e.getMessage());
+ // if the table is deleted, there is no way to check user permissions to
cancel
+ // the system should handle this automatically
+ throw new ThriftSecurityException(credentials.getPrincipal(),
+ SecurityErrorCode.PERMISSION_DENIED);
Review Comment:
I fell like the table not found exception is the right exception here. This
isn't really a permission problem.
--
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]
