2755: incorporate code review

* lib/locator.rb renamed => app/models/locator.rb
* Relaxed Locator.parse! handling of hint content.
* Locator.parse() rescues only from ArgumentError.
* Removed blob_signing_ttl from application.yml.example.
* Collections.show only matches locators that are preceded by a space,
  when parsing manifest_text.

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 2844cb4c7a0f0afc8bcf40c5b3fb0b9f9b9d53b2..1982a525f7c03366a5174e10d39e9e0a53bd96d0 100644 (file)
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -1,5 +1,3 @@
-require 'locator'
-
 class Arvados::V1::CollectionsController < ApplicationController
   def create
     # Collections are owned by system_user. Creating a collection has
@@ -24,8 +22,8 @@ class Arvados::V1::CollectionsController < ApplicationController
     }
     resource_attrs[:manifest_text].lines.each do |entry|
       entry.split[1..-1].each do |tok|
-        # TODO(twp): fail the request if this match fails.
-        # Add in Phase 4 (see #2755)
+        # TODO(twp): in Phase 4, fail the request if the locator
+        # lacks a permission signature. (see #2755)
         loc = Locator.parse(tok)
         if loc and loc.signature
           if !api_token
@@ -44,10 +42,10 @@ class Arvados::V1::CollectionsController < ApplicationController
 
     # Remove any permission signatures from the manifest.
     resource_attrs[:manifest_text]
-      .gsub!(/[[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) { |word|
+      .gsub!(/ [[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) { |word|
       loc = Locator.parse(word)
       if loc
-        loc.without_signature.to_s
+        " " + loc.without_signature.to_s
       else
         word
       end
@@ -95,10 +93,10 @@ class Arvados::V1::CollectionsController < ApplicationController
         ttl: Rails.configuration.blob_signing_ttl,
       }
       @object[:manifest_text]
-        .gsub!(/[[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) { |word|
+        .gsub!(/ [[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) { |word|
         loc = Locator.parse(word)
         if loc
-          Blob.sign_locator(word, signing_opts)
+          " " + Blob.sign_locator(word, signing_opts)
         else
           word
         end

diff --git a/services/api/app/models/locator.rb b/services/api/app/models/locator.rb
new file mode 100644 (file)
index 0000000..39d7da9
--- /dev/null
+++ b/services/api/app/models/locator.rb
@@ -0,0 +1,84 @@
+# A Locator is used to parse and manipulate Keep locator strings.
+#
+# Locators obey the following syntax:
+#
+#   locator      ::= address hint*
+#   address      ::= digest size-hint
+#   digest       ::= <32 hexadecimal digits>
+#   size-hint    ::= "+" [0-9]+
+#   hint         ::= "+" hint-type hint-content
+#   hint-type    ::= [A-Z]
+#   hint-content ::= [A-Za-z0-9@_-]+
+#
+# Individual hints may have their own required format:
+# 
+#   sign-hint      ::= "+A" <40 lowercase hex digits> "@" sign-timestamp
+#   sign-timestamp ::= <8 lowercase hex digits>
+
+class Locator
+  def initialize(hasharg, sizearg, hintarg)
+    @hash = hasharg
+    @size = sizearg
+    @hints = hintarg
+  end
+
+  # Locator.parse returns a Locator object parsed from the string tok.
+  # Returns nil if tok could not be parsed as a valid locator.
+  def self.parse(tok)
+    begin
+      Locator.parse!(tok)
+    rescue ArgumentError => e
+      nil
+    end
+  end
+
+  # Locator.parse! returns a Locator object parsed from the string tok,
+  # raising an ArgumentError if tok cannot be parsed.
+  def self.parse!(tok)
+    m = /^([[:xdigit:]]{32})(\+([[:digit:]]+))?(\+([[:upper:]][[:alnum:]+@_-]*))?$/.match(tok.strip)
+    unless m
+      raise ArgumentError.new "could not parse #{tok}"
+    end
+
+    tokhash, _, toksize, _, trailer = m[1..5]
+    tokhints = []
+    if trailer
+      trailer.split('+').each do |hint|
+        if hint =~ /^[[:upper:]][[:alnum:]@_-]+$/
+          tokhints.push(hint)
+        else
+          raise ArgumentError.new "unknown hint #{hint}"
+        end
+      end
+    end
+
+    Locator.new(tokhash, toksize, tokhints)
+  end
+
+  # Returns the signature hint supplied with this locator,
+  # or nil if the locator was not signed.
+  def signature
+    @hints.grep(/^A/).first
+  end
+
+  # Returns an unsigned Locator.
+  def without_signature
+    Locator.new(@hash, @size, @hints.reject { |o| o.start_with?("A") })
+  end
+
+  def hash
+    @hash
+  end
+
+  def size
+    @size
+  end
+
+  def hints
+    @hints
+  end
+
+  def to_s
+    [ @hash, @size, *@hints ].join('+')
+  end
+end

diff --git a/services/api/config/application.yml.example b/services/api/config/application.yml.example
index e0420ce076145a8a776aa7dbc4c27085a0cc94f0..030e23894f00a5436354d36cf946272bdea616a2 100644 (file)
--- a/services/api/config/application.yml.example
+++ b/services/api/config/application.yml.example
@@ -52,6 +52,3 @@ common:
   #git_repositories_dir: /var/cache/git
   #git_internal_dir: /var/cache/arvados/internal.git
 
-  # Amount of time (in seconds) for which a blob permission signature
-  # remains valid.  Default: 2 weeks (1209600 seconds)
-  #blob_signing_ttl: 1209600

diff --git a/services/api/lib/locator.rb b/services/api/lib/locator.rb
deleted file mode 100644 (file)
index 0ec3f62..0000000
--- a/services/api/lib/locator.rb
+++ /dev/null
@@ -1,63 +0,0 @@
-class Locator
-  # This regex will match a word that appears to be a locator.
-  @@pattern = %r![[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)?!
-
-  def initialize(hasharg, sizearg, optarg)
-    @hash = hasharg
-    @size = sizearg
-    @options = optarg
-  end
-
-  def self.parse(tok)
-    Locator.parse!(tok) rescue nil
-  end
-
-  def self.parse!(tok)
-    m = /^([[:xdigit:]]{32})(\+([[:digit:]]+))?(\+.*)?$/.match(tok)
-    unless m
-      raise ArgumentError.new "could not parse #{tok}"
-    end
-
-    tokhash, _, toksize, trailer = m[1..4]
-    tokopts = []
-    while m = /^\+[[:upper:]][^\s+]+/.match(trailer)
-      opt = m.to_s
-      if opt =~ /^\+A[[:xdigit:]]+@[[:xdigit:]]{8}$/ or
-          opt =~ /\+K@[[:alnum:]]+$/
-        tokopts.push(opt)
-        trailer = m.post_match
-      else
-        raise ArgumentError.new "unknown option #{opt}"
-      end
-    end
-    if trailer and !trailer.empty?
-      raise ArgumentError.new "unrecognized trailing chars #{trailer}"
-    end
-
-    Locator.new(tokhash, toksize, tokopts)
-  end
-
-  def signature
-    @options.grep(/^\+A/).first
-  end
-
-  def without_signature
-    Locator.new(@hash, @size, @options.reject { |o| o.start_with?("+A") })
-  end
-
-  def hash
-    @hash
-  end
-
-  def size
-    @size
-  end
-
-  def options
-    @options
-  end
-
-  def to_s
-    [ @hash + "+", @size, *@options].join
-  end
-end