jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Gaul <notificati...@github.com>
Subject Re: [jclouds/jclouds] [JCLOUDS-1428] Support for SAS token based Authentication for Azure Blob Storage (#1270)
Date Wed, 30 Jan 2019 20:58:27 GMT
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
Mime
View raw message