Closed taylorthurlow closed 6 years ago
Closed my pull request. I had incorrectly assumed that symbolize_keys
was a ruby thing, not an ActiveSupport
thing.
I'm curious about others' opinions here. Is it a better idea to only allow symbol keys? Using symbol keys definitely feels more correct, but I can still define a hash using string keys.
Hey @taylorthurlow. Sorry I've been MIA. I've got a lot on my plate right now, and maintenance on the Ruby and Rails SDK has had to wait. I think it's okay to change the README and define that only symbol keys are supported. But I would also welcome the input of others as to whether or not that specification will be too restrictive in production.
@staturecrane Don't worry about it. I don't have particularly strong feelings either way. I'm sure changing any available documentation, and perhaps throwing a small disclaimer in would be enough to prevent this sort of problem in the future.
If you'd like to close this that's fine, otherwise I'll leave it open in case anyone else wants to voice an opinion.
@taylorthurlow the symbolize_keys
method is very small and can be copied from ActiveSupport as something like lib/filestack/core_ext/hash.rb
with:
module Filestack
class Hash
def self.symbolize_keys
# Contents here
end
end
end
and then called when the config is initialized as: Filestack::Hash.symbolize_keys(config_hash)
or whatever.
@jfelchner I've opened a pull request with some changes following your suggestion. I went for a far simpler implementation of symbolize_keys
, mostly because transform_keys
isn't a thing until Ruby 2.5, and the Rails 4.x implementation is all tied up with the Key
object.
@taylorthurlow - The issue is solved in current released version of Ruby SDK.
@gabifiolek solved how exactly? 😂 A link to a commit or CHANGELOG entry would be nice.
@jfelchner So... it looks like my pull request was closed and after an essentially identical commit was made to fix the issue. Maybe there's something I'm not seeing here but it looks like my changes were just copied.
@gabifiolek ?
Pull request in question is here: https://github.com/filestack/filestack-ruby/pull/31
Hey don't mean to bother you @staturecrane as I believe you handed off this repo to someone else, but not sure who else to ping about this. See my previous comment about my commits seemingly being copied without credit.
@taylorthurlow I understand your frustration, but I no longer work for this company nor contribute to this project and so I have no more information than you about this.
Hi, I submitted an issue in the filestack-rails repo (https://github.com/filestack/filestack-rails/issues/175), which after some poking around I have solved and found that it's a small issue with this gem instead. I'll be submitting a pull request shortly, but I'd like to outline my original issue and proposed changes.
Using
filestack-rails
, I set up a security policy this way:This hash is handled by the
filestack-rails
gem and passed directly into a newFilestackSecurity
object. The issue lies in howFilestackSecurity
handles it's input. Here's a short debugger session to detail the issue.The
expiry_timestamp
function adds a duplicate key to the options hash. It does this because it incorrectly uses a symbolized key:expiry
instead of the string key'expiry'
.expiry_timestamp
checks the wrong key, and thinks that there was no expiry key passed in to begin with, and offers the default expiry length of 1 hour. Using string keys instead of symbol keys increate_policy_string
andexpiry_timestamp
fixes the problem. My solution will probably be to call.symbolize_keys
on the options hash, ensuring that it doesn't matter whether the end user configures the gem using string keys or symbol keys.All that said, I don't normally do pull requests or dig through gem source, so I'm not really sure what the process is for getting this change made in the
filestack
gem and then gettingfilestack-rails
to accept the new version as a dependency - currently it requires2.2.0
. I'll submit a pull request. If i've done anything incorrectly, sorry!