Merge pull request #162 from projecthydra/s3-enhancements

Amazon S3 driver enhancements

.rubocop.yml

@@ -11,6 +11,9 @@ AllCops:
 Rails:
   Enabled: true
 
+Metrics/ClassLength:
+  Max: 130
+
 # Configuration parameters: AllowURI, URISchemes.
 Metrics/LineLength:
   Max: 400

README.md

@@ -7,7 +7,8 @@
 This Gem allows your rails application to access user files from cloud storage.
 Currently there are drivers implemented for [Dropbox](http://www.dropbox.com),
 [Skydrive](https://skydrive.live.com/), [Google Drive](http://drive.google.com),
-[Box](http://www.box.com), and a server-side directory share.
+[Box](http://www.box.com), [Amazon S3](https://aws.amazon.com/s3/),
+and a server-side directory share.
 
 The gem uses [OAuth](http://oauth.net/) to connect to a user's account and
 generate a list of single use urls that your application can then use to

lib/browse_everything/browser.rb

@@ -16,7 +16,7 @@ def initialize(opts = {})
         begin
           driver_klass = BrowseEverything::Driver.const_get((config[:driver] || driver.to_s).camelize.to_sym)
           @providers[driver] = driver_klass.new(config.merge(url_options: url_options))
-        rescue
+        rescue NameError
           Rails.logger.warn "Unknown provider: #{driver}"
         end
       end

lib/browse_everything/driver/s3.rb

@@ -3,12 +3,17 @@
 module BrowseEverything
   module Driver
     class S3 < Base
-      DEFAULTS = { signed_url: true, region: 'us-east-1' }.freeze
-      CONFIG_KEYS = [:app_key, :app_secret, :bucket].freeze
+      DEFAULTS = { response_type: :signed_url }.freeze
+      RESPONSE_TYPES = [:signed_url, :public_url, :s3_uri].freeze
+      CONFIG_KEYS = [:bucket].freeze
 
       attr_reader :entries
 
       def initialize(config, *args)
+        if config.key?(:signed_url) && config.delete(:signed_url) == false
+          warn '[DEPRECATION] Amazon S3 driver: `:signed_url` is deprecated.  Please use `:response_type` instead.'
+          config[:response_type] = :public_url
+        end
         config = DEFAULTS.merge(config)
         super
       end
@@ -18,6 +23,12 @@ def icon
       end
 
       def validate_config
+        if config.values_at(:app_key, :app_secret).compact.length == 1
+          raise BrowseEverything::InitializationError, 'Amazon S3 driver: If either :app_key or :app_secret is provided, both must be.'
+        end
+        unless RESPONSE_TYPES.include?(config[:response_type].to_sym)
+          raise BrowseEverything::InitializationError, "Amazon S3 driver: Valid response types: #{RESPONSE_TYPES.join(',')}"
+        end
         return if CONFIG_KEYS.all? { |key| config[key].present? }
         raise BrowseEverything::InitializationError, "Amazon S3 driver requires #{CONFIG_KEYS.join(',')}"
       end
@@ -32,20 +43,23 @@ def contents(path = '')
       end
 
       def generate_listing(path)
-        listing = client.list_objects(bucket: config[:bucket], delimiter: '/', prefix: path)
+        listing = client.list_objects(bucket: config[:bucket], delimiter: '/', prefix: full_path(path))
         add_directories(listing)
         add_files(listing, path)
       end
 
       def add_directories(listing)
         listing.common_prefixes.each do |prefix|
-          entries << entry_for(prefix.prefix, 0, Time.current, true)
+          entries << entry_for(from_base(prefix.prefix), 0, Time.current, true)
         end
       end
 
       def add_files(listing, path)
-        listing.contents.reject { |entry| entry.key == path }.each do |entry|
-          entries << entry_for(entry.key, entry.size, entry.last_modified, false)
+        listing.contents.each do |entry|
+          key = from_base(entry.key)
+          unless strip(key) == strip(path)
+            entries << entry_for(key, entry.size, entry.last_modified, false)
+          end
         end
       end
 
@@ -63,12 +77,8 @@ def init_entries(path)
         @entries = if path.empty?
                      []
                    else
-                     [BrowseEverything::FileEntry.new(Pathname(path).join('..'),
-                                                      '',
-                                                      '..',
-                                                      0,
-                                                      Time.current,
-                                                      true)]
+                     [BrowseEverything::FileEntry.new(Pathname(path).join('..').to_s, '', '..',
+                                                      0, Time.current, true)]
                    end
       end
 
@@ -77,23 +87,20 @@ def entry_for(name, size, date, dir)
       end
 
       def details(path)
-        entry = client.head_object(path)
+        entry = client.head_object(full_path(path))
         BrowseEverything::FileEntry.new(
-          entry.key,
-          [key, entry.key].join(':'),
-          File.basename(entry.key),
-          entry.size,
-          entry.last_modified,
-          false
+          entry.key, [key, entry.key].join(':'),
+          File.basename(entry.key), entry.size,
+          entry.last_modified, false
         )
       end
 
       def link_for(path)
-        obj = bucket.object(path)
-        if config[:signed_url]
-          obj.presigned_url(:get, expires_in: 14400)
-        else
-          obj.public_url
+        obj = bucket.object(full_path(path))
+        case config[:response_type].to_sym
+        when :signed_url then obj.presigned_url(:get, expires_in: 14400)
+        when :public_url then obj.public_url
+        when :s3_uri     then "s3://#{obj.bucket_name}/#{obj.key}"
         end
       end
 
@@ -106,8 +113,29 @@ def bucket
       end
 
       def client
-        @client ||= Aws::S3::Client.new(credentials: Aws::Credentials.new(config[:app_key], config[:app_secret]), region: config[:region])
+        @client ||= Aws::S3::Client.new(aws_config)
       end
+
+      private
+
+        def strip(path)
+          path.sub %r{^/?(.+?)/?$}, '\1'
+        end
+
+        def from_base(key)
+          Pathname.new(key).relative_path_from(Pathname.new(config[:base].to_s)).to_s
+        end
+
+        def full_path(path)
+          config[:base].present? ? File.join(config[:base], path) : path
+        end
+
+        def aws_config
+          result = {}
+          result[:credentials] = Aws::Credentials.new(config[:app_key], config[:app_secret]) if config[:app_key].present?
+          result[:region] = config[:region] if config.key?(:region)
+          result
+        end
     end
   end
 end

lib/generators/browse_everything/templates/browse_everything_providers.yml.example

@@ -12,11 +12,12 @@
 #   :client_id: YOUR_GOOGLE_API_CLIENT_ID
 #   :client_secret: YOUR_GOOGLE_API_CLIENT_SECRET
 # s3:
-#   :app_key: YOUR_AWS_S3_KEY
-#   :app_secret: YOUR_AWS_S3_SECRET
 #   :bucket: YOUR_AWS_S3_BUCKET
-#   :region: YOUR_AWS_S3_REGION # default: us-east-1
-#   :signed_url: true # set to false for public urls
+#   :response_type: :signed_url # set to :public_url for public urls or :s3_uri for an s3://BUCKET/KEY uri
+#   :app_key: YOUR_AWS_S3_KEY       # :app_key, :app_secret, and :region can be specified
+#   :app_secret: YOUR_AWS_S3_SECRET # explicitly here, or left out to use system-configured
+#   :region: YOUR_AWS_S3_REGION     # defaults.
+#   See https://aws.amazon.com/blogs/security/a-new-and-standardized-way-to-manage-credentials-in-the-aws-sdks/
 # sky_drive:
 #   :client_id: YOUR_MS_LIVE_CONNECT_CLIENT_ID
 #   :client_secret: YOUR_MS_LIVE_CONNECT_CLIENT_SECRET

spec/unit/s3_spec.rb

@@ -1,13 +1,43 @@
 include BrowserConfigHelper
 
 describe BrowseEverything::Driver::FileSystem do
-  before(:all)  { stub_configuration   }
-  after(:all)   { unstub_configuration }
-  let(:browser) { BrowseEverything::Browser.new(url_options) }
+  before(:all)   { stub_configuration   }
+  after(:all)    { unstub_configuration }
+  let(:browser)  { BrowseEverything::Browser.new(url_options) }
   let(:provider) { browser.providers['s3'] }
+  subject        { provider }
 
-  it '#validate_config' do
-    expect { BrowseEverything::Driver::S3.new({}) }.to raise_error(BrowseEverything::InitializationError)
+  describe 'defaults' do
+    its(:icon)   { is_expected.to eq('amazon')  }
+    its(:itself) { is_expected.to be_authorized }
+  end
+
+  describe 'configuration' do
+    it '#validate_config' do
+      expect { BrowseEverything::Driver::S3.new({}) }.to raise_error(BrowseEverything::InitializationError)
+    end
+
+    it 'rejects app_key if app_secret is missing' do
+      expect { BrowseEverything::Driver::S3.new(bucket: 'bucket', app_key: 'APP_KEY') }.to raise_error(BrowseEverything::InitializationError)
+    end
+
+    it 'rejects app_secret if app_key is missing' do
+      expect { BrowseEverything::Driver::S3.new(bucket: 'bucket', app_secret: 'APP_SECRET') }.to raise_error(BrowseEverything::InitializationError)
+    end
+
+    it 'accepts app_key and app_secret together' do
+      expect { BrowseEverything::Driver::S3.new(bucket: 'bucket', app_key: 'APP_KEY', app_secret: 'APP_SECRET') }.not_to raise_error
+    end
+
+    it 'rejects an invalid response type' do
+      expect { BrowseEverything::Driver::S3.new(bucket: 'bucket', response_type: :foo) }.to raise_error(BrowseEverything::InitializationError)
+    end
+
+    it 'deprecates :signed_url' do
+      driver = BrowseEverything::Driver::S3.new(bucket: 'bucket', signed_url: false)
+      expect(driver.config).not_to have_key(:signed_url)
+      expect(driver.config[:response_type]).to eq(:public_url)
+    end
   end
 
   describe '#contents' do
@@ -72,6 +102,33 @@
         its(:size)     { is_expected.to eq(1_511_860) }
         specify        { is_expected.not_to be_container }
       end
+
+      context '#link_for' do
+        subject { contents[2] }
+        before do
+          object = instance_double(Aws::S3::Object)
+          allow(object).to receive(:presigned_url).and_return('https://s3.amazonaws.com/presigned_url')
+          allow(object).to receive(:public_url).and_return('https://s3.amazonaws.com/public_url')
+          allow(object).to receive(:bucket_name).and_return('s3.bucket')
+          allow(object).to receive(:key).and_return('foo/quux.png')
+          allow(provider.bucket).to receive(:object).and_return(object)
+        end
+
+        it ':signed_url' do
+          provider.config[:response_type] = :signed_url
+          expect(provider.link_for('foo/quux.png')).to eq('https://s3.amazonaws.com/presigned_url')
+        end
+
+        it ':public_url' do
+          provider.config[:response_type] = :public_url
+          expect(provider.link_for('foo/quux.png')).to eq('https://s3.amazonaws.com/public_url')
+        end
+
+        it ':s3_uri' do
+          provider.config[:response_type] = :s3_uri
+          expect(provider.link_for('foo/quux.png')).to eq('s3://s3.bucket/foo/quux.png')
+        end
+      end
     end
   end
 end