From bba1a8479323982829e101828430d968316d5801 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Tue, 1 Dec 2020 16:46:31 +0800 Subject: [PATCH 1/3] HDDS-4519. Return forbidden instead of interval server error from s3g when user doesn't have the resouce permission. --- .../hadoop/ozone/client/OzoneBucket.java | 25 +- .../apache/hadoop/ozone/om/OzoneManager.java | 4 +- .../fs/ozone/BasicOzoneClientAdapterImpl.java | 2 +- .../BasicRootedOzoneClientAdapterImpl.java | 2 +- .../hadoop/fs/ozone/OzoneClientAdapter.java | 2 +- .../ozone/s3/endpoint/BucketEndpoint.java | 49 +++- .../ozone/s3/endpoint/EndpointBase.java | 27 +- .../ozone/s3/endpoint/ObjectEndpoint.java | 15 ++ .../ozone/s3/exception/S3ErrorTable.java | 4 + .../s3/endpoint/TestPermissionCheck.java | 241 ++++++++++++++++++ 10 files changed, 334 insertions(+), 37 deletions(-) create mode 100644 hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 20a12716370f..9aa8420dbc63 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -517,7 +517,8 @@ public long getUsedBytes() { * @param keyPrefix Bucket prefix to match * @return {@code Iterator} */ - public Iterator listKeys(String keyPrefix) { + public Iterator listKeys(String keyPrefix) + throws IOException{ return listKeys(keyPrefix, null); } @@ -532,7 +533,7 @@ public Iterator listKeys(String keyPrefix) { * @return {@code Iterator} */ public Iterator listKeys(String keyPrefix, - String prevKey) { + String prevKey) throws IOException { return new KeyIterator(keyPrefix, prevKey); } @@ -760,7 +761,6 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix) private class KeyIterator implements Iterator { private String keyPrefix = null; - private Iterator currentIterator; private OzoneKey currentValue; @@ -771,7 +771,7 @@ private class KeyIterator implements Iterator { * The returned keys match key prefix. * @param keyPrefix */ - KeyIterator(String keyPrefix, String prevKey) { + KeyIterator(String keyPrefix, String prevKey) throws IOException{ this.keyPrefix = keyPrefix; this.currentValue = null; this.currentIterator = getNextListOfKeys(prevKey).iterator(); @@ -780,7 +780,11 @@ private class KeyIterator implements Iterator { @Override public boolean hasNext() { if(!currentIterator.hasNext() && currentValue != null) { - currentIterator = getNextListOfKeys(currentValue.getName()).iterator(); + try { + currentIterator = getNextListOfKeys(currentValue.getName()).iterator(); + } catch (IOException e) { + throw new RuntimeException(e); + } } return currentIterator.hasNext(); } @@ -799,13 +803,10 @@ public OzoneKey next() { * @param prevKey * @return {@code List} */ - private List getNextListOfKeys(String prevKey) { - try { - return proxy.listKeys(volumeName, name, keyPrefix, prevKey, - listCacheSize); - } catch (IOException e) { - throw new RuntimeException(e); - } + private List getNextListOfKeys(String prevKey) throws + IOException { + return proxy.listKeys(volumeName, name, keyPrefix, prevKey, + listCacheSize); } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 6229beffa265..c5b953c2f37e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -1805,7 +1805,9 @@ public boolean checkAcls(ResourceType resType, StoreType storeType, obj.getKeyName()); throw new OMException("User " + context.getClientUgi().getUserName() + " doesn't have " + context.getAclRights() + - " permission to access " + obj.getResourceType(), + " permission to access " + obj.getResourceType() + " " + + obj.getVolumeName() + " " + obj.getBucketName() + " " + + obj.getKeyName(), ResultCodes.PERMISSION_DENIED); } return false; diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java index 0b5098886f61..f9bee5400d50 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java @@ -310,7 +310,7 @@ public FileStatusAdapter getFileStatus(String key, URI uri, @Override - public Iterator listKeys(String pathKey) { + public Iterator listKeys(String pathKey) throws IOException{ incrementCounter(Statistic.OBJECTS_LIST, 1); return new IteratorAdapter(bucket.listKeys(pathKey)); } diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java index 848119d5f55c..66daed10e31c 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java @@ -615,7 +615,7 @@ public Collection getTrashRoots(boolean allUsers, } @Override - public Iterator listKeys(String pathStr) { + public Iterator listKeys(String pathStr) throws IOException { incrementCounter(Statistic.OBJECTS_LIST, 1); OFSPath ofsPath = new OFSPath(pathStr); String key = ofsPath.getKeyName(); diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java index 2b76c22c4d56..b9e288164080 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java @@ -55,7 +55,7 @@ OzoneFSOutputStream createFile(String key, short replication, boolean deleteObjects(List keyName); - Iterator listKeys(String pathKey); + Iterator listKeys(String pathKey) throws IOException; List listStatus(String keyName, boolean recursive, String startKey, long numEntries, URI uri, diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java index 789bb4511027..c9f194af4600 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java @@ -116,19 +116,26 @@ public Response list( if (startAfter == null && marker != null) { startAfter = marker; } - if (startAfter != null && continueToken != null) { - // If continuation token and start after both are provided, then we - // ignore start After - ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey()); - } else if (startAfter != null && continueToken == null) { - ozoneKeyIterator = bucket.listKeys(prefix, startAfter); - } else if (startAfter == null && continueToken != null){ - ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey()); - } else { - ozoneKeyIterator = bucket.listKeys(prefix); + try { + if (startAfter != null && continueToken != null) { + // If continuation token and start after both are provided, then we + // ignore start After + ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey()); + } else if (startAfter != null && continueToken == null) { + ozoneKeyIterator = bucket.listKeys(prefix, startAfter); + } else if (startAfter == null && continueToken != null) { + ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey()); + } else { + ozoneKeyIterator = bucket.listKeys(prefix); + } + } catch (OMException ex) { + if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + } else { + throw ex; + } } - ListObjectResponse response = new ListObjectResponse(); response.setDelimiter(delimiter); response.setName(bucketName); @@ -229,8 +236,16 @@ public Response listMultipartUploads( OzoneBucket bucket = getBucket(bucketName); - OzoneMultipartUploadList ozoneMultipartUploadList = - bucket.listMultipartUploads(prefix); + OzoneMultipartUploadList ozoneMultipartUploadList; + try { + ozoneMultipartUploadList = bucket.listMultipartUploads(prefix); + } catch (OMException exception) { + if (exception.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + prefix); + } + throw exception; + } ListMultipartUploadsResult result = new ListMultipartUploadsResult(); result.setBucket(bucketName); @@ -282,6 +297,8 @@ public Response delete(@PathParam("bucket") String bucketName) } else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable .NO_SUCH_BUCKET, bucketName); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); } else { throw ex; } @@ -315,7 +332,11 @@ public MultiDeleteResponse multiDelete(@PathParam("bucket") String bucketName, result.addDeleted(new DeletedObject(keyToDelete.getKey())); } } catch (OMException ex) { - if (ex.getResult() != ResultCodes.KEY_NOT_FOUND) { + if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + result.addError( + new Error(keyToDelete.getKey(), "PermissionDenied", + ex.getMessage())); + } else if (ex.getResult() != ResultCodes.KEY_NOT_FOUND) { result.addError( new Error(keyToDelete.getKey(), "InternalError", ex.getMessage())); diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java index b60519df0be5..7e055159f713 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java @@ -70,6 +70,8 @@ protected OzoneBucket getBucket(String bucketName) if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND || ex.getResult() == ResultCodes.VOLUME_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); } else { throw ex; } @@ -91,11 +93,13 @@ protected OzoneVolume getVolume() throws IOException { * @throws IOException */ protected String createS3Bucket(String bucketName) throws - IOException { + IOException, OS3Exception { try { client.getObjectStore().createS3Bucket(bucketName); } catch (OMException ex) { - if (ex.getResult() != ResultCodes.BUCKET_ALREADY_EXISTS) { + if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + } else if (ex.getResult() != ResultCodes.BUCKET_ALREADY_EXISTS) { // S3 does not return error for bucket already exists, it just // returns the location. throw ex; @@ -110,8 +114,15 @@ protected String createS3Bucket(String bucketName) throws * @throws IOException in case the bucket cannot be deleted. */ public void deleteS3Bucket(String s3BucketName) - throws IOException { - client.getObjectStore().deleteS3Bucket(s3BucketName); + throws IOException, OS3Exception { + try { + client.getObjectStore().deleteS3Bucket(s3BucketName); + } catch (OMException ex) { + if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, s3BucketName); + } + throw ex; + } } /** @@ -123,7 +134,7 @@ public void deleteS3Bucket(String s3BucketName) * @return {@code Iterator} */ public Iterator listS3Buckets(String prefix) - throws IOException { + throws IOException, OS3Exception { return iterateBuckets(volume -> volume.listBuckets(prefix)); } @@ -138,18 +149,20 @@ public Iterator listS3Buckets(String prefix) * @return {@code Iterator} */ public Iterator listS3Buckets(String prefix, - String previousBucket) throws IOException { + String previousBucket) throws IOException, OS3Exception { return iterateBuckets(volume -> volume.listBuckets(prefix, previousBucket)); } private Iterator iterateBuckets( Function> query) - throws IOException { + throws IOException, OS3Exception{ try { return query.apply(getVolume()); } catch (OMException e) { if (e.getResult() == ResultCodes.VOLUME_NOT_FOUND) { return Collections.emptyIterator(); + } else if (e.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, "listBuckets"); } else { throw e; } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java index 527f774b9a87..d2622dd55416 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java @@ -209,6 +209,9 @@ public Response put( " considered as Unix Paths. Path has Violated FS Semantics " + "which caused put operation to fail."); throw os3Exception; + } else if ((((OMException) ex).getResult() == + ResultCodes.PERMISSION_DENIED)) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); } } throw ex; @@ -320,6 +323,9 @@ public Response get( if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable .NO_SUCH_KEY, keyPath); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable + .PERMISSION_DENIED, keyPath); } else { throw ex; } @@ -619,6 +625,9 @@ private Response createMultipartKey(String bucket, String key, long length, if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) { throw S3ErrorTable.newError(NO_SUCH_UPLOAD, uploadID); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + bucket + "/" + key); } throw ex; } @@ -675,6 +684,9 @@ private Response listParts(String bucket, String key, String uploadID, if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) { throw S3ErrorTable.newError(NO_SUCH_UPLOAD, uploadID); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + bucket + "/" + key + "/" + uploadID); } throw ex; } @@ -760,6 +772,9 @@ private CopyObjectResponse copyObject(String copyHeader, throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_KEY, sourceKey); } else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, sourceBucket); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + destBucket + "/" + destkey); } throw ex; } finally { diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java index 432b582bc86f..ee68767a19c0 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java @@ -22,6 +22,7 @@ import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_CONFLICT; +import static java.net.HttpURLConnection.HTTP_FORBIDDEN; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import static java.net.HttpURLConnection.HTTP_SERVER_ERROR; import static org.apache.hadoop.ozone.s3.util.S3Consts.RANGE_NOT_SATISFIABLE; @@ -105,6 +106,9 @@ private S3ErrorTable() { "InternalError", "We encountered an internal error. Please try again.", HTTP_SERVER_ERROR); + public static final OS3Exception PERMISSION_DENIED = new OS3Exception( + "PermissionDenied", "User doesn't have the permission to access this " + + "resource.", HTTP_FORBIDDEN); /** * Create a new instance of Error. diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java new file mode 100644 index 000000000000..6b0be586ee3c --- /dev/null +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java @@ -0,0 +1,241 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ +package org.apache.hadoop.ozone.s3.endpoint; + +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.ozone.client.ObjectStore; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.client.OzoneClient; +import org.apache.hadoop.ozone.client.OzoneVolume; +import org.apache.hadoop.ozone.om.exceptions.OMException; +import org.apache.hadoop.ozone.s3.exception.OS3Exception; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + +import javax.ws.rs.core.Cookie; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.MultivaluedMap; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +import static java.net.HttpURLConnection.HTTP_FORBIDDEN; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; + +/** + * Test operation permission check result. + */ +public class TestPermissionCheck { + private OzoneConfiguration conf; + private OzoneClient client; + private ObjectStore objectStore; + private OzoneBucket bucket; + private OzoneVolume volume; + private OMException exception; + private HttpHeaders headers; + + @Before + public void setup() { + conf = new OzoneConfiguration(); + conf.set(OzoneConfigKeys.OZONE_S3_VOLUME_NAME, + OzoneConfigKeys.OZONE_S3_VOLUME_NAME_DEFAULT); + client = Mockito.mock(OzoneClient.class); + objectStore = Mockito.mock(ObjectStore.class); + bucket = Mockito.mock(OzoneBucket.class); + volume = Mockito.mock(OzoneVolume.class); + exception = new OMException("Permission Denied", + OMException.ResultCodes.PERMISSION_DENIED); + Mockito.when(client.getObjectStore()).thenReturn(objectStore); + Mockito.when(client.getConfiguration()).thenReturn(conf); + headers = Mockito.mock(HttpHeaders.class); + } + + /** + * Root Endpoint + */ + @Test + public void testListS3Buckets() throws IOException { + doThrow(exception).when(objectStore).getVolume(anyString()); + RootEndpoint rootEndpoint = new RootEndpoint(); + rootEndpoint.setClient(client); + + try { + rootEndpoint.get(); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + /** + * Bucket Endpoint + */ + @Test + public void testGetBucket() throws IOException { + doThrow(exception).when(objectStore).getS3Bucket(anyString()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + + try { + bucketEndpoint.head("bucketName"); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testCreateBucket() throws IOException { + Mockito.when(objectStore.getVolume(anyString())).thenReturn(volume); + doThrow(exception).when(objectStore).createS3Bucket(anyString()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + + try { + bucketEndpoint.put("bucketName", null); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testDeleteBucket() throws IOException { + doThrow(exception).when(objectStore).deleteS3Bucket(anyString()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + + try { + bucketEndpoint.delete("bucketName"); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + @Test + public void testListMultiUpload() throws IOException { + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket).listMultipartUploads(anyString()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + + try { + bucketEndpoint.listMultipartUploads("bucketName", "prefix"); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testListKey() throws IOException { + Mockito.when(objectStore.getVolume(anyString())).thenReturn(volume); + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket).listKeys(anyString()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + + try { + bucketEndpoint.list("bucketName", null, null, null, 1000, + null, null, null, null, null, null); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testDeleteKeys() throws IOException, OS3Exception { + Mockito.when(objectStore.getVolume(anyString())).thenReturn(volume); + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket).deleteKey(any()); + BucketEndpoint bucketEndpoint = new BucketEndpoint(); + bucketEndpoint.setClient(client); + MultiDeleteRequest request = new MultiDeleteRequest(); + List objectList = new ArrayList<>(); + objectList.add(new MultiDeleteRequest.DeleteObject("deleteKeyName")); + request.setQuiet(false); + request.setObjects(objectList); + + MultiDeleteResponse response = + bucketEndpoint.multiDelete("BucketName", "keyName", request); + Assert.assertTrue(response.getErrors().size() == 1); + Assert.assertTrue( + response.getErrors().get(0).getCode().equals("PermissionDenied")); + } + + /** + * Object Endpoint + */ + @Test + public void testGetKey() throws IOException { + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket).getKey(anyString()); + ObjectEndpoint objectEndpoint = new ObjectEndpoint(); + objectEndpoint.setClient(client); + objectEndpoint.setHeaders(headers); + + try { + objectEndpoint.get("bucketName", "keyPath", "uploadId", 1000, "marker", + null); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testPutKey() throws IOException { + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket) + .createKey(anyString(), anyLong(), any(), any(), any()); + ObjectEndpoint objectEndpoint = new ObjectEndpoint(); + objectEndpoint.setClient(client); + objectEndpoint.setHeaders(headers); + + try { + objectEndpoint.put("bucketName", "keyPath", 1024, 0, null, null); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } +} From 748aab07b64d917237ebec8c912df054508d34db Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 3 Dec 2020 20:28:19 +0800 Subject: [PATCH 2/3] Fix checkstyle; Add more test cases --- .../hadoop/ozone/client/OzoneBucket.java | 3 +- .../ozone/s3/endpoint/EndpointBase.java | 6 ++- .../ozone/s3/endpoint/ObjectEndpoint.java | 12 +++-- .../s3/endpoint/TestPermissionCheck.java | 53 ++++++++++++++----- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 9aa8420dbc63..02803fd69c54 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -781,7 +781,8 @@ private class KeyIterator implements Iterator { public boolean hasNext() { if(!currentIterator.hasNext() && currentValue != null) { try { - currentIterator = getNextListOfKeys(currentValue.getName()).iterator(); + currentIterator = + getNextListOfKeys(currentValue.getName()).iterator(); } catch (IOException e) { throw new RuntimeException(e); } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java index 7e055159f713..618af702c15e 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java @@ -119,7 +119,8 @@ public void deleteS3Bucket(String s3BucketName) client.getObjectStore().deleteS3Bucket(s3BucketName); } catch (OMException ex) { if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, s3BucketName); + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + s3BucketName); } throw ex; } @@ -162,7 +163,8 @@ private Iterator iterateBuckets( if (e.getResult() == ResultCodes.VOLUME_NOT_FOUND) { return Collections.emptyIterator(); } else if (e.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, "listBuckets"); + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + "listBuckets"); } else { throw e; } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java index d2622dd55416..e74208baa67f 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java @@ -324,8 +324,7 @@ public Response get( throw S3ErrorTable.newError(S3ErrorTable .NO_SUCH_KEY, keyPath); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable - .PERMISSION_DENIED, keyPath); + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); } else { throw ex; } @@ -363,6 +362,8 @@ public Response head( if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) { // Just return 404 with no content return Response.status(Status.NOT_FOUND).build(); + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); } else { throw ex; } @@ -432,6 +433,8 @@ public Response delete( } else if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) { //NOT_FOUND is not a problem, AWS doesn't throw exception for missing // keys. Just return 204 + } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); } else { throw ex; } @@ -480,9 +483,12 @@ public Response initializeMultipartUpload( return Response.status(Status.OK).entity( multipartUploadInitiateResponse).build(); - } catch (IOException ex) { + } catch (OMException ex) { LOG.error("Error in Initiate Multipart Upload Request for bucket: {}, " + "key: {}", bucket, key, ex); + if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { + throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, key); + } throw ex; } } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java index 6b0be586ee3c..1c5622e1baab 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java @@ -32,24 +32,15 @@ import org.junit.Test; import org.mockito.Mockito; -import javax.ws.rs.core.Cookie; import javax.ws.rs.core.HttpHeaders; -import javax.ws.rs.core.MediaType; -import javax.ws.rs.core.MultivaluedMap; import java.io.IOException; import java.util.ArrayList; -import java.util.Date; -import java.util.HashMap; import java.util.List; -import java.util.Locale; -import java.util.Map; import static java.net.HttpURLConnection.HTTP_FORBIDDEN; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; /** @@ -81,7 +72,7 @@ public void setup() { } /** - * Root Endpoint + * Root Endpoint. */ @Test public void testListS3Buckets() throws IOException { @@ -99,7 +90,7 @@ public void testListS3Buckets() throws IOException { } /** - * Bucket Endpoint + * Bucket Endpoint. */ @Test public void testGetBucket() throws IOException { @@ -201,7 +192,7 @@ public void testDeleteKeys() throws IOException, OS3Exception { } /** - * Object Endpoint + * Object Endpoint. */ @Test public void testGetKey() throws IOException { @@ -212,10 +203,11 @@ public void testGetKey() throws IOException { objectEndpoint.setHeaders(headers); try { - objectEndpoint.get("bucketName", "keyPath", "uploadId", 1000, "marker", + objectEndpoint.get("bucketName", "keyPath", null, 1000, "marker", null); Assert.fail("Should fail"); } catch (Exception e) { + e.printStackTrace(); Assert.assertTrue(e instanceof OS3Exception); Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); } @@ -238,4 +230,39 @@ public void testPutKey() throws IOException { Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); } } + + @Test + public void testDeleteKey() throws IOException { + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket).deleteKey(anyString()); + ObjectEndpoint objectEndpoint = new ObjectEndpoint(); + objectEndpoint.setClient(client); + objectEndpoint.setHeaders(headers); + + try { + objectEndpoint.delete("bucketName", "keyPath", null); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } + + @Test + public void testMultiUploadKey() throws IOException { + Mockito.when(objectStore.getS3Bucket(anyString())).thenReturn(bucket); + doThrow(exception).when(bucket) + .initiateMultipartUpload(anyString(), any(), any()); + ObjectEndpoint objectEndpoint = new ObjectEndpoint(); + objectEndpoint.setClient(client); + objectEndpoint.setHeaders(headers); + + try { + objectEndpoint.initializeMultipartUpload("bucketName", "keyPath"); + Assert.fail("Should fail"); + } catch (Exception e) { + Assert.assertTrue(e instanceof OS3Exception); + Assert.assertTrue(((OS3Exception) e).getHttpCode() == HTTP_FORBIDDEN); + } + } } From d4cd1f88a245a25643827628dd23f70298280fb5 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 10 Dec 2020 11:48:50 +0800 Subject: [PATCH 3/3] address comments --- .../hadoop/ozone/s3/endpoint/BucketEndpoint.java | 6 +++--- .../hadoop/ozone/s3/endpoint/EndpointBase.java | 8 ++++---- .../hadoop/ozone/s3/endpoint/ObjectEndpoint.java | 16 ++++++++-------- .../hadoop/ozone/s3/exception/S3ErrorTable.java | 4 ++-- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java index c9f194af4600..b8bed64cc830 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java @@ -130,7 +130,7 @@ public Response list( } } catch (OMException ex) { if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName); } else { throw ex; } @@ -241,7 +241,7 @@ public Response listMultipartUploads( ozoneMultipartUploadList = bucket.listMultipartUploads(prefix); } catch (OMException exception) { if (exception.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, prefix); } throw exception; @@ -298,7 +298,7 @@ public Response delete(@PathParam("bucket") String bucketName) throw S3ErrorTable.newError(S3ErrorTable .NO_SUCH_BUCKET, bucketName); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName); } else { throw ex; } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java index 618af702c15e..360f4f436aea 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java @@ -71,7 +71,7 @@ protected OzoneBucket getBucket(String bucketName) || ex.getResult() == ResultCodes.VOLUME_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName); } else { throw ex; } @@ -98,7 +98,7 @@ protected String createS3Bucket(String bucketName) throws client.getObjectStore().createS3Bucket(bucketName); } catch (OMException ex) { if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, bucketName); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName); } else if (ex.getResult() != ResultCodes.BUCKET_ALREADY_EXISTS) { // S3 does not return error for bucket already exists, it just // returns the location. @@ -119,7 +119,7 @@ public void deleteS3Bucket(String s3BucketName) client.getObjectStore().deleteS3Bucket(s3BucketName); } catch (OMException ex) { if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, s3BucketName); } throw ex; @@ -163,7 +163,7 @@ private Iterator iterateBuckets( if (e.getResult() == ResultCodes.VOLUME_NOT_FOUND) { return Collections.emptyIterator(); } else if (e.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, "listBuckets"); } else { throw e; diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java index e74208baa67f..6b4efb72e40b 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java @@ -211,7 +211,7 @@ public Response put( throw os3Exception; } else if ((((OMException) ex).getResult() == ResultCodes.PERMISSION_DENIED)) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath); } } throw ex; @@ -324,7 +324,7 @@ public Response get( throw S3ErrorTable.newError(S3ErrorTable .NO_SUCH_KEY, keyPath); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath); } else { throw ex; } @@ -363,7 +363,7 @@ public Response head( // Just return 404 with no content return Response.status(Status.NOT_FOUND).build(); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath); } else { throw ex; } @@ -434,7 +434,7 @@ public Response delete( //NOT_FOUND is not a problem, AWS doesn't throw exception for missing // keys. Just return 204 } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, keyPath); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath); } else { throw ex; } @@ -487,7 +487,7 @@ public Response initializeMultipartUpload( LOG.error("Error in Initiate Multipart Upload Request for bucket: {}, " + "key: {}", bucket, key, ex); if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, key); + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, key); } throw ex; } @@ -632,7 +632,7 @@ private Response createMultipartKey(String bucket, String key, long length, throw S3ErrorTable.newError(NO_SUCH_UPLOAD, uploadID); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucket + "/" + key); } throw ex; @@ -691,7 +691,7 @@ private Response listParts(String bucket, String key, String uploadID, throw S3ErrorTable.newError(NO_SUCH_UPLOAD, uploadID); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucket + "/" + key + "/" + uploadID); } throw ex; @@ -779,7 +779,7 @@ private CopyObjectResponse copyObject(String copyHeader, } else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) { throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, sourceBucket); } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) { - throw S3ErrorTable.newError(S3ErrorTable.PERMISSION_DENIED, + throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, destBucket + "/" + destkey); } throw ex; diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java index ee68767a19c0..a2c9f17dda14 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java @@ -106,8 +106,8 @@ private S3ErrorTable() { "InternalError", "We encountered an internal error. Please try again.", HTTP_SERVER_ERROR); - public static final OS3Exception PERMISSION_DENIED = new OS3Exception( - "PermissionDenied", "User doesn't have the permission to access this " + + public static final OS3Exception ACCESS_DENIED = new OS3Exception( + "AccessDenied", "User doesn't have the right to access this " + "resource.", HTTP_FORBIDDEN); /**