Policy Condition failed: ["starts-with", "$key", "uploads"] when updating existing image #74

Closed
kikito opened this Issue Apr 2, 2013 · 16 comments

Projects

None yet

6 participants

@kikito
Contributor
kikito commented Apr 2, 2013

Hello there,

I'm able to upload images with carrierwave_direct the first time. But if I try to update them later on (change the image for another one), I get this S3 error:

<Error>
  <Code>AccessDenied</Code>
  <Message>
    Invalid according to Policy: Policy Condition failed: ["starts-with", "$key", "uploads"]
  </Message>
  <RequestId>[...]</RequestId>
  <HostId>[...]</HostId>
</Error>

My current setup is:

rails (3.2.12)
carrierwave (0.8.0)
carrierwave_direct (0.0.11)
fog (1.10.0)

Models:

class User < ActiveRecord::Base
  has_one :avatar, :as => :avatar_owner
end

class Avatar < ActiveRecord::Base
  belongs_to :avatar_owner, :polymorphic => true
  mount_uploader :image, AvatarUploader
end

Uploader (simplified):

class AvatarUploader < CarrierWave::Uploader::Base
  include CarrierWaveDirect::Uploader
  include Sprockets::Helpers::RailsHelper
  include Sprockets::Helpers::IsolatedHelper
  process :set_content_type
  def extension_white_list
    %w(jpg jpeg gif png)
  end
end

Avatar form (I only allow creating avatars for existing users; when a user is created its avatar is nil):

<%= direct_upload_form_for @uploader do |f| %>
  <%= f.file_field :image %>
<% end %>

Avatars controller:

class AvatarController
  before_filter get_avatar

  def edit
    @uploader = @avatar.image
    @uploader.success_action_redirect = url_for(['update_image', @avatar_owner,'avatar'])
  end

  # successful uploads redirect here
  def update_image
    @owner.build_avatar if @owner.avatar.nil?
    @owner.avatar.key = params[:key] if params[:key]
    redirect_to :owner
  end

  private
  def get_avatar
    owner_type    = params[:type]
    owner_model   = owner_type.constantize
    @avatar_owner = owner_model.find params[:id]
    @avatar       = @avatar_owner.avatar || Avatar.new(:avatar_owner_id => @avatar_owner.id, :avatar_owner_type => owner_type)
  end
end

As a reminder: my problem is that I can upload an avatar image the first time, but any subsequent attempts at updating the image give me an error. I'd appreciate any help.

Thanks a lot!

@kikito
Contributor
kikito commented Apr 3, 2013

I've been studying this in more detail.

  • On "new" images, the key attribute looks like this in the network payload (some numbers have been zeroed just in case):

    Content-Disposition: form-data; name="key"
    uploads/5ee72a20-7e78-0000-0000-7cd1c3de5b0b/${filename}

  • On "existing" images, the key attribute looks like this (some numbers have been zeroed):

    Content-Disposition: form-data; name="key"
    /uploads/d9a75a40-7d23-0000-0000-7cd1c3de5b0b/matz-slide.jpg

The second one makes S3 fail.

I strongly suspect this is due to the fact that the second one starts with "/", althought the fact that ${filename} is missing could also play a role here.

Has anyone had similar issues to this one before?

@kikito
Contributor
kikito commented Apr 3, 2013

I suspect this is related with #73 , which fix "extra slashes added sometimes to paths".

Since it's already merged, could you guys please release a new gem version?

Thanks a lot!

@colinyoung
Contributor

Sure, @kikito. I won't close this one, yet, though, in case it is an issue with ${filename}.

@kikito
Contributor
kikito commented Apr 3, 2013

Actually, hold your horses. I tried that fix locally and it didn't help. But I'm investigating this a bit further. If you give me 20 minutes I can get back to you.

EDIT: I think I've got it. Unfortunately github seems to have decided to start failing just now, and I can't clone the repo to run the tests.

The trouble seems to be in the way the key is calculated on existing files:

module CarrierWaveDirect
  module Uploader
    def key
      return @key if @key.present?
      if url.present?
        self.key = URI.parse(URI.encode(url)).path[1 .. -1] # explicitly set key minus initial slash
      else
        @key = "#{store_dir}/#{guid}/#{FILENAME_WILDCARD}"
      end
      @key
    end
  end
end

All I did was adding the [1..-1] at the end, to remove the initial slash. I have tried it locally in an initializer on my rails app, and I can update files now. I'd like to run the gem tests with this patch to see if they work fine too, but I can't clone the repo :/

@colinyoung
Contributor

When GH comes back up and you're able to clone, let me know -- it's ok if 0.0.12 doesn't include this fix. We can release again.

@kikito
Contributor
kikito commented Apr 3, 2013

I have run the tests by cloning the (read-only) repo and doing the changes locally. Unfortunately they don't pass. They seem to assume that the "initial push" begins with "uploads/...", but from that moment on they begin with "/uploads/...".

I don't know what to do then :/ Things seem to be working on my end, but I'd hate to break the gem for everyone else. Any ideas?

Failures:

1) CarrierWaveDirect::Uploader#key where the key is not set but the uploaders url is 'https://s3-bucket.s3.amazonaws.com/store_dir/guid/filename.avi' should return '/store_dir/guid/filename.avi'
 Failure/Error: mounted_subject.key.should == "/#{sample(:s3_key)}"
   expected: "/store_dir/guid/filename.avi"
        got: "store_dir/guid/filename.avi" (using ==)
 # ./spec/uploader_spec.rb:127:in `block (5 levels) in <top (required)>'

2) CarrierWaveDirect::Uploader#key where the key is not set but the uploaders url is 'https://s3-bucket.s3.amazonaws.com/store_dir/guid/filename.avi' should set the key explicitly in order to update the version keys
 Failure/Error: mounted_subject.key
   https://s3-bucket.s3.amazonaws.com/store_dir/guid/filename.avi received :key= with unexpected arguments
     expected: ("/store_dir/guid/filename.avi")
          got: ("store_dir/guid/filename.avi")
 # ./lib/carrierwave_direct/uploader.rb:45:in `key'
 # ./spec/uploader_spec.rb:132:in `block (5 levels) in <top (required)>'
@kikito kikito added a commit to kikito/carrierwave_direct that referenced this issue Apr 4, 2013
@kikito kikito remove initial slash from existing paths. References #74 6bf18af
@i0n
i0n commented Apr 10, 2013

I have the same issue, any news on merging the pull request?

@renanmzmendes

I'm getting this error ( Policy Condition failed: ["starts-with", "$key", "uploads"] ) even when uploading for the first time. I'm following the railscast http://railscasts.com/episodes/383-uploading-to-amazon-s3 and it doesn't work. Do I need to configure my bucket's policy? I'm a little lost in all of this...

@renanmzmendes

Looking at the developer tools I saw that the value of key was fallback/company_photo/default.png, as you can see in the html below:

<input id="company_photo_uploader_key" name="key" type="hidden" value="fallback/company_photo/default.png">

This value is actually my default path. How can I change this?

@nicholasmartin

I'm getting this error when I try to upload a new image on an existing image. It works first time when I upload, but if I go and update my record and add a new file, it fails with this message.. bummer.

@nddeluca
Collaborator
nddeluca commented Jul 6, 2013

@renanmzmendes Can you post your uploader code? You're key doesn't look right to me.

@lifestylenetworker It appears the pull request referenced in this thread never made it into version 0.0.12. I'll get a new gem version released as soon I can.

But, for now you can monkey patch the uploader using:

module CarrierWaveDirect
  module Uploader
    def key
      return @key if @key.present?
      if url.present?
        self.key = URI.parse(URI.encode(url)).path[1..-1]
      else
        @key = "#{store_dir}/#{guid}/#{FILENAME_WILDCARD}"
      end
      @key
    end
  end
end

As stated in @kikito's comment above.

@renanmzmendes

@nddeluca Actually, I got it to work when I used the master branch instead of the released gem. Could you tell us in this thread when the new version will be available?

@nicholasmartin

Thanks that worked perfectly!

@nicholasmartin

Ah this only worked on my local development platform. As soon as I push to Heroku, the error is back again. How would I go about patching this so that it works on Heroku?

@nicholasmartin

So I managed to install the gem from master with:

gem 'carrierwave_direct', :git => 'https://github.com/dwilkie/carrierwave_direct', :branch => 'master'

The error message is gone, but my file is not uploaded/updated. This is my controller:

def upload
    @issue = Issue.find(params[:id])
    @uploader = @issue.hpub
    @uploader.success_action_redirect = preview_issue_url

    respond_to do |format|
      format.html # index.html.erb
      format.json { render json: @issues }
    end
  end

  def preview
    @issue = Issue.find(params[:id])
    @issue.update_attribute :key, params[:key]
    @issue.save

    respond_to do |format|
      format.html # index.html.erb
      format.json { render json: @issues }
    end
  end

My form uploader:

<%= direct_upload_form_for @uploader, :html => { :class => "form-inline" } do |f| %>
  <%= f.file_field :hpub %>
  <%= f.submit "Upload New Issue", class: "btn btn-large btn-primary"%>
<% end %>

My uploader class is straight out the box, with no store_dir (commented out) and it's mounted on the hpub field of my model.

Any idea why it's not re-uploading a new file to S3? It seems to be uploading as the browser is waiting while uploading, but I can't find the new file in S3 and Carrierwave_direct just returns the same old key to the model/object.

@nddeluca
Collaborator
nddeluca commented Jul 7, 2013

@renanmzmendes and @lifestylenetworker - Just released version 0.0.13, which includes the fix for uploading existing files. Closing the issue.

@lifestylenetworker Thats odd - can you create a new issue with some steps so I can replicate the problem? I don't see any problems with your controller or form.

@nddeluca nddeluca closed this Jul 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment