gaul requested changes on this pull request.
Please add unit tests. Also explain how to test this? Is it possible to write
an integration test?
> + boolean ruleSAS = (length < 150) && (length > 120);
+ boolean ruleKey = (length < 100) && (length > 70 );
+
+ if (ruleSAS) {
+ return true;
+ } else if (ruleKey) {
+ return false;
+ } else {
+ return false;
+ }
+ }
+ /**
+ * this filter method is applied only for the cases with SAS
Authentication.
+ *
+ */
+ public HttpRequest filterSAS(HttpRequest request, String credential)
throws HttpException {
Fix indentation. Note that jclouds uses 3-space indents not tabs.
>
+ //now we need the piece, situated to the right of
".net/":
+ result = centrePart.split(".net/");
+ String path = result[1];
+
+ //finally, separate container and blob names:
+ result = path.split("/");
+
+ } catch (Exception e) {
+ e.printStackTrace();
We always propagate exceptions.
>
+ //now we need the piece, situated to the right of
".net/":
+ result = centrePart.split(".net/");
+ String path = result[1];
+
+ //finally, separate container and blob names:
+ result = path.split("/");
Would `new URI(String)` solve this more cleanly?
> @Singleton
public class AzureBlobRequestSigner implements BlobRequestSigner {
- private static final int DEFAULT_EXPIRY_SECONDS = 15 * 60;
- private static final String API_VERSION = "2017-04-17";
+ private static final int DEFAULT_EXPIRY_SECONDS = 300 * 60;
+ private static final String API_VERSION = "2018-03-28";
Why do we need to change the API version?
> @@ -73,6 +77,8 @@ public HttpRequest signGetBlob(String container, String
> name) {
@Override
public HttpRequest signGetBlob(String container, String name, long
timeInSeconds) {
+ System.out.println("Method signGetBlob is called " +
container + " , " + name + " , " + timeInSeconds);
Remove unneeded logging.
> @@ -105,9 +111,28 @@ public HttpRequest signGetBlob(String container, String
> name, org.jclouds.blobst
return sign("GET", container, name,
blob2HttpGetOptions.apply(checkNotNull(options, "options")),
DEFAULT_EXPIRY_SECONDS, null, null);
}
-
- private HttpRequest sign(String method, String container, String name,
@Nullable GetOptions options, long expires, @Nullable Long contentLength,
@Nullable String contentType) {
- checkNotNull(method, "method");
+
+ /**
+ * method to parse SAS string into components like st, se, sr etc.
+ *
+ */
+ public HashMap<String, String> refineSAS(String sas) {
+ HashMap<String, String> paramsMap = new HashMap<>();
jclouds prefers `ImmutableMap` for instances and `Map` for interfaces, e.g.,
return types.
> @@ -105,9 +111,28 @@ public HttpRequest signGetBlob(String container, String
> name, org.jclouds.blobst
return sign("GET", container, name,
blob2HttpGetOptions.apply(checkNotNull(options, "options")),
DEFAULT_EXPIRY_SECONDS, null, null);
}
-
- private HttpRequest sign(String method, String container, String name,
@Nullable GetOptions options, long expires, @Nullable Long contentLength,
@Nullable String contentType) {
- checkNotNull(method, "method");
+
+ /**
+ * method to parse SAS string into components like st, se, sr etc.
+ *
+ */
+ public HashMap<String, String> refineSAS(String sas) {
+ HashMap<String, String> paramsMap = new HashMap<>();
+ String[] queryArray = sas.split("&");
How does this work exactly? `?` delimits the first parameter. Instead use
`new URI` and allow it to parse the parameters?
> + *
+ */
+ public HttpRequest filterSAS(HttpRequest request, String credential)
throws HttpException {
+ String containerName = "";
+ String blobName = "";
+ String requestLine = request.getRequestLine();
+ String[] containerBlob = cutUri(requestLine);
+ if (containerBlob[0] != null) {
+ containerName = containerBlob[0];
+ }
+ if (containerBlob[1] != null) {
+ blobName = containerBlob[1];
+ }
+ request =
request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).appendPath(blobName).query(credential).build()).build();
+ request = replaceDateHeader(request);
+ request = removeAuthorizationHeader(request);
Prefer a builder pattern, e.g.,
```java
return
request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).appendPath(blobName).query(credential).build()).build()
replaceDateHeader(request)
removeAuthorizationHeader(request);
> +
+ if (ruleSAS) {
+ return true;
+ } else if (ruleKey) {
+ return false;
+ } else {
+ return false;
+ }
+ }
+ /**
+ * this filter method is applied only for the cases with SAS
Authentication.
+ *
+ */
+ public HttpRequest filterSAS(HttpRequest request, String credential)
throws HttpException {
+ String containerName = "";
+ String blobName = "";
Are these meaningful if empty strings?
> +
+ /**
+ *
+ * this method checks the length of the authentication string, and decides,
which auth method it represents.
+ *
+ */
+ public boolean authSAS(String credential) {
+ int length = credential.length();
+
+ boolean ruleSAS = (length < 150) && (length > 120);
+ boolean ruleKey = (length < 100) && (length > 70 );
+
+ if (ruleSAS) {
+ return true;
+ } else if (ruleKey) {
+ return false;
I don't understand this logic -- why not just `return ruleSAS`?
> + request = filterKey(request);
+ }
+ utils.logRequest(signatureLog, request, "<<");
+ return request;
+ }
+
+ /**
+ *
+ * this method checks the length of the authentication string, and decides,
which auth method it represents.
+ *
+ */
+ public boolean authSAS(String credential) {
+ int length = credential.length();
+
+ boolean ruleSAS = (length < 150) && (length > 120);
+ boolean ruleKey = (length < 100) && (length > 70 );
Huh?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1270#pullrequestreview-198300324
