-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Nio check access #18095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nio check access #18095
Changes from 4 commits
8d06f25
c0dc3dc
ffd7465
ba91298
1bab6e0
64e1a7b
e4d7198
22ecb7f
45c20a9
81b5ad2
bd31b45
82ddd75
5f16848
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,13 +14,15 @@ | |
| import reactor.core.publisher.Flux; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| import java.io.FileNotFoundException; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.io.UncheckedIOException; | ||
| import java.net.HttpURLConnection; | ||
| import java.net.URI; | ||
| import java.nio.channels.SeekableByteChannel; | ||
| import java.nio.file.AccessDeniedException; | ||
| import java.nio.file.AccessMode; | ||
| import java.nio.file.CopyOption; | ||
| import java.nio.file.DirectoryNotEmptyException; | ||
|
|
@@ -373,7 +375,8 @@ public OutputStream newOutputStream(Path path, OpenOption... options) throws IOE | |
| // Write and truncate must be specified | ||
| if (!optionsList.contains(StandardOpenOption.WRITE) | ||
| || !optionsList.contains(StandardOpenOption.TRUNCATE_EXISTING)) { | ||
| throw new IllegalArgumentException("Write and TruncateExisting must be specified to open an OutputStream"); | ||
| throw logger.logThrowableAsError( | ||
| new IllegalArgumentException("Write and TruncateExisting must be specified to open an OutputStream")); | ||
| } | ||
|
|
||
| AzureResource resource = new AzureResource(path); | ||
|
|
@@ -785,11 +788,14 @@ public FileStore getFileStore(Path path) throws IOException { | |
| } | ||
|
|
||
| /** | ||
| * Unsupported. | ||
| * Checks the existence, and optionally the accessibility, of a file. | ||
| * <p> | ||
| * This method may only be used to check the existence of a file. It is not possible to determine the permissions | ||
| * granted to a given client, so if any mode argument is specified, an {@link UnsupportedOperationException} will be | ||
| * thrown. | ||
| * | ||
| * @param path path | ||
| * @param accessModes accessMode | ||
| * @throws UnsupportedOperationException Operation is not supported. | ||
| * @param path the path to the file to check | ||
| * @param accessModes The access modes to check; may have zero elements | ||
| * @throws NoSuchFileException if a file does not exist | ||
| * @throws java.nio.file.AccessDeniedException the requested access would be denied or the access cannot be | ||
| * determined because the Java virtual machine has insufficient privileges or other reasons | ||
|
|
@@ -798,7 +804,23 @@ public FileStore getFileStore(Path path) throws IOException { | |
| */ | ||
| @Override | ||
| public void checkAccess(Path path, AccessMode... accessModes) throws IOException { | ||
| throw new UnsupportedOperationException(); | ||
| if (accessModes != null && accessModes.length != 0) { | ||
| throw logger.logThrowableAsError( | ||
| new AccessDeniedException("The access cannot be determined.")); | ||
| } | ||
| AzurePath.ensureFileSystemOpen(path); | ||
|
|
||
| // Read attributes already wraps BlobStorageException in an IOException. | ||
| try { | ||
| readAttributes(path, BasicFileAttributes.class); | ||
| } catch(IOException e) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a thought - should auth errors be wrapped into AccessDeniedExceptions?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I tend to think that debugging will be easier if issues with underlying blob stuff is presented differently from nio stuff. I can see an argument where that kind of breaks the point of mapping one to the other, but an AccessDeniedException seems like it would have a response of "well let me go grant access" which is impossible in this case, and an IOException caused by a BlobStorageException kind of leads me down a path of something is wrong with my account or nio configs. What do you think?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the general pattern we've aimed for with nio then I'm fine with that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if it's generalized yet haha. Still could change if you think otherwise. But we'd have to add that explicit check in a lot of different places, which I'm also not a fan of because I think we're likely to forget one. |
||
| if (e.getCause() != null && e.getCause() instanceof BlobStorageException | ||
| && BlobErrorCode.BLOB_NOT_FOUND.equals(((BlobStorageException) e.getCause()).getErrorCode())) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this also be ContainerNotFound?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once I add support for containers, yes. A good reminder that I'll have to update a lot of error handling like that |
||
| throw logger.logThrowableAsError(new NoSuchFileException(path.toString())); | ||
| } else { | ||
| throw e; | ||
|
rickle-msft marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,31 @@ | |
|
|
||
| package com.azure.storage.blob.nio | ||
|
|
||
| import com.azure.core.http.HttpHeaders | ||
| import com.azure.core.http.HttpMethod | ||
| import com.azure.core.http.HttpPipelineCallContext | ||
| import com.azure.core.http.HttpPipelineNextPolicy | ||
| import com.azure.core.http.HttpRequest | ||
| import com.azure.core.http.HttpResponse | ||
| import com.azure.core.http.policy.HttpPipelinePolicy | ||
| import com.azure.storage.blob.BlobClient | ||
| import com.azure.storage.blob.models.AccessTier | ||
| import com.azure.storage.blob.models.BlobErrorCode | ||
| import com.azure.storage.blob.models.BlobHttpHeaders | ||
| import com.azure.storage.blob.specialized.AppendBlobClient | ||
| import com.azure.storage.blob.specialized.BlockBlobClient | ||
| import reactor.core.publisher.Flux | ||
| import reactor.core.publisher.Mono | ||
| import spock.lang.Requires | ||
| import spock.lang.Unroll | ||
|
|
||
| import java.nio.ByteBuffer | ||
| import java.nio.charset.Charset | ||
| import java.nio.file.AccessDeniedException | ||
| import java.nio.file.AccessMode | ||
| import java.nio.file.DirectoryNotEmptyException | ||
| import java.nio.file.FileAlreadyExistsException | ||
| import java.nio.file.Files | ||
| import java.nio.file.FileSystem | ||
| import java.nio.file.FileSystemAlreadyExistsException | ||
| import java.nio.file.FileSystemNotFoundException | ||
|
|
@@ -1058,6 +1072,142 @@ class AzureFileSystemProviderTest extends APISpec { | |
| thrown(IOException) | ||
| } | ||
|
|
||
| def "CheckAccess"() { | ||
| setup: | ||
| def fs = createFS(config) | ||
| def path = fs.getPath(generateBlobName()) | ||
| def os = fs.provider().newOutputStream(path) | ||
| os.close() | ||
|
|
||
| when: | ||
| fs.provider().checkAccess(path) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could probably just do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's functionally the same. If all I'm doing is check the call succeeds, I usually like to make that explicit by saying I don't want an exception. |
||
|
|
||
| then: | ||
| notThrown(Exception) | ||
| } | ||
|
|
||
| @Unroll | ||
| def "CheckAccess AccessDenied"() { | ||
| setup: | ||
| def fs = createFS(config) | ||
| def path = fs.getPath(generateBlobName()) | ||
| def os = fs.provider().newOutputStream(path) | ||
| os.close() | ||
|
|
||
| when: | ||
| fs.provider().checkAccess(path, mode) | ||
|
|
||
| then: | ||
| thrown(AccessDeniedException) | ||
|
|
||
| where: | ||
| mode | _ | ||
| AccessMode.READ | _ | ||
| AccessMode.WRITE | _ | ||
| AccessMode.EXECUTE | _ | ||
| } | ||
|
|
||
| def "CheckAccess IOException"() { | ||
| setup: | ||
| HttpPipelinePolicy[] policies = new HttpPipelinePolicy[1] | ||
| policies[0] = new checkAccessIoExceptionPolicy() | ||
| config.put(AzureFileSystem.AZURE_STORAGE_HTTP_POLICIES, policies) | ||
| def fs = createFS(config) | ||
| def path = fs.getPath(generateBlobName()) | ||
| def os = fs.provider().newOutputStream(path) | ||
| os.close() | ||
|
|
||
| when: | ||
| fs.provider().checkAccess(path) | ||
|
|
||
| then: | ||
| def e = thrown(IOException) | ||
| !(e instanceof NoSuchFileException) | ||
| } | ||
|
|
||
| class checkAccessIoExceptionPolicy implements HttpPipelinePolicy { | ||
| @Override | ||
| Mono<HttpResponse> process(HttpPipelineCallContext httpPipelineCallContext, HttpPipelineNextPolicy httpPipelineNextPolicy) { | ||
| HttpRequest request = httpPipelineCallContext.getHttpRequest() | ||
| // GetProperties call to blob | ||
| if (request.getUrl().getPath().split("/").size() == 3 && request.getHttpMethod() == (HttpMethod.HEAD)) { | ||
| return Mono.just(new checkAccessIoExceptionResponse(request)) | ||
| } else { | ||
| return httpPipelineNextPolicy.process() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class checkAccessIoExceptionResponse extends HttpResponse { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: capitalize the C |
||
| protected checkAccessIoExceptionResponse(HttpRequest request) { | ||
| super(request) | ||
| } | ||
|
|
||
| @Override | ||
| int getStatusCode() { | ||
| return 403 | ||
| } | ||
|
|
||
| @Override | ||
| String getHeaderValue(String s) { | ||
| return request.getHeaders().getValue(s) | ||
| } | ||
|
|
||
| @Override | ||
| HttpHeaders getHeaders() { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.put("x-ms-error-code", BlobErrorCode.AUTHORIZATION_FAILURE.toString()) | ||
| return headers | ||
| } | ||
|
|
||
| @Override | ||
| Flux<ByteBuffer> getBody() { | ||
| return Flux.just(ByteBuffer.allocate(0)) | ||
| } | ||
|
|
||
| @Override | ||
| Mono<byte[]> getBodyAsByteArray() { | ||
| return Mono.just(new byte[0]) | ||
| } | ||
|
|
||
| @Override | ||
| Mono<String> getBodyAsString() { | ||
| return Mono.just("") | ||
| } | ||
|
|
||
| @Override | ||
| Mono<String> getBodyAsString(Charset charset) { | ||
| return Mono.just("") | ||
| } | ||
| } | ||
|
|
||
| def "CheckAccess no file"() { | ||
| setup: | ||
| def fs = createFS(config) | ||
| def path = fs.getPath(generateBlobName()) | ||
|
|
||
| when: | ||
| fs.provider().checkAccess(path) | ||
|
|
||
| then: | ||
| thrown(NoSuchFileException) | ||
| } | ||
|
|
||
| def "CheckAccess fs closed"() { | ||
| setup: | ||
| def fs = createFS(config) | ||
| def path = fs.getPath(generateBlobName()) | ||
| def os = fs.provider().newOutputStream(path) | ||
| os.close() | ||
|
|
||
| when: | ||
| fs.close() | ||
| fs.provider().checkAccess(path) | ||
|
|
||
| then: | ||
| thrown(IOException) | ||
| } | ||
|
|
||
| @Unroll | ||
| def "GetAttributeView"() { | ||
| setup: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be UnsupportedOperationException or AccessDeniedException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AccessDenied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(per the jdk docs for this method: "the requested access would be denied or the access cannot be determined because the Java virtual machine has insufficient privileges or other reasons." I think it's reasonable to put this under access cannot be determined)