jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds] [JCLOUDS-1428] Support for SAS token based Authentication for Azure Blob Storage (#1270)
Date Fri, 01 Feb 2019 15:18:42 GMT
nacx requested changes on this pull request.

Thanks, @ak58588!
Please take another look at fixing indentation.

>     public HttpRequest filter(HttpRequest request) throws HttpException {
+      this.sasAuthentication = authSAS(this.credential);

This could be set in the constructor instead of checking this on every request.

> +         request = filterSAS(request, this.credential);
+      } else {
+         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();
+      return  length < 150 && length > 120;    

I'm not familiar with SAS auth. Is there a stronger or more reliable way to detect the kind
of credentials being used?

I'm thinking you could create a proper `SAASCredentials` object to be used when creating the
context (via the `credentialsSupplier` attribute, then this check would be less error-prone
as you can test the object type of the credentials.

Would this make sense?

> +   /** 
+   * this filter method is applied only for the cases with SAS Authentication. 
+   * 
+   */
+   public HttpRequest filterSAS(HttpRequest request, String credential) throws HttpException
{
+      String containerName = null;
+      String blobName = null;
+      URI requestUri = request.getEndpoint();
+      try {
+         String[] parametersArray = cutUri(requestUri); 
+         containerName = parametersArray[1];
+         if (parametersArray.length == 3) {
+            blobName = parametersArray[2];
+            return removeAuthorizationHeader(replaceDateHeader(request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).appendPath(blobName).query(credential).build()).build()));
+         } 
+         return removeAuthorizationHeader(replaceDateHeader(request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).query("restype=container&"
+ credential).build()).addHeader("x-ms-version", "2018-03-28").build()));

It would be good to be able to configure the value of this header via properties. Make it
configurable with this as the default value?

> +   * 
+   */
+   public HttpRequest filterSAS(HttpRequest request, String credential) throws HttpException
{
+      String containerName = null;
+      String blobName = null;
+      URI requestUri = request.getEndpoint();
+      try {
+         String[] parametersArray = cutUri(requestUri); 
+         containerName = parametersArray[1];
+         if (parametersArray.length == 3) {
+            blobName = parametersArray[2];
+            return removeAuthorizationHeader(replaceDateHeader(request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).appendPath(blobName).query(credential).build()).build()));
+         } 
+         return removeAuthorizationHeader(replaceDateHeader(request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).query("restype=container&"
+ credential).build()).addHeader("x-ms-version", "2018-03-28").build()));
+      } catch (NullPointerException e){
+         e.printStackTrace();

Use a logger to log errors instead of printing to stdout

> +   }
+   
+   /** 
+   * this filter method is applied only for the cases with SAS Authentication. 
+   * 
+   */
+   public HttpRequest filterSAS(HttpRequest request, String credential) throws HttpException
{
+      String containerName = null;
+      String blobName = null;
+      URI requestUri = request.getEndpoint();
+      try {
+         String[] parametersArray = cutUri(requestUri); 
+         containerName = parametersArray[1];
+         if (parametersArray.length == 3) {
+            blobName = parametersArray[2];
+            return removeAuthorizationHeader(replaceDateHeader(request.toBuilder().endpoint(Uris.uriBuilder(storageUrl).appendPath(containerName).appendPath(blobName).query(credential).build()).build()));

Add some newlines here so it is easier to read

>     private final HttpUtils utils;
+   private final URI storageUrl;
+   private boolean sasAuthentication;

Make this final once it is initialized int he constructor.

> +         request.replaceHeader("x-ms-blob-content-type", contentType);
+      }
+
+      if (options != null) {
+         request.headers(options.buildRequestHeaders());
+      }
+		
+		 
+      if (method.equals("PUT")) {
+         request.replaceHeader("x-ms-blob-type", "BlockBlob");
+      }
+	 
+	  HttpRequest req = request.build();
+	  
+      return req;
+   }  

Most of this method is just a copy of the existing one. Break the original one in reusable
methods and reuse code.

> +      this.sasAuthentication = authSAS(this.credential);
+      if (this.sasAuthentication){
+         request = filterSAS(request, this.credential);
+      } else {
+         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){

Make this static

> +		 
+      if (method.equals("PUT")) {
+         request.replaceHeader("x-ms-blob-type", "BlockBlob");
+      }
+	 
+	  HttpRequest req = request.build();
+	  
+      return req;
+   }  
+   
+   /**
+   * modified sign() method, which acts depending on the Auth input. 
+   * 
+   */
+   public HttpRequest sign(String method, String container, String name, @Nullable GetOptions
options, long expires, @Nullable Long contentLength, @Nullable String contentType) {
+      boolean isSAS = auth.authSAS(this.credential);

Again, this could go in the constructor instead of resolving it every time?

-- 
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-199114854
Mime
View raw message