Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use the file_download_item helper on download links #1079

Merged
merged 3 commits into from
Apr 12, 2019
Merged

use the file_download_item helper on download links #1079

merged 3 commits into from
Apr 12, 2019

Conversation

pgwillia
Copy link
Member

@pgwillia pgwillia commented Apr 11, 2019 •
edited
Loading

previously this would show the active storage blob url and not check for
authorization or update the download count.

@pbinkley
Copy link
Member

@pgwillia confirmed that downloads will still be sourced to the ActiveStorage blob and not directly to Fedora, which is what we want.

With this change in place, could we block external acess to the ActiveStorage blob urls completely? We would have to be sure to let thumbnails (and other derivatives) through. Something to think about if the authentication-check is going to be difficult to implement.

@pbinkley pbinkley requested a review from weiweishi April 11, 2019 21:18
@pbinkley
Copy link
Member

I'd like to hold off on merging this until @weiweishi has seen it, in case there's relevant history behind the use of blob urls on the search results pages that I'm not aware of.

@pgwillia
use the file_download_item helper on download links
6cf397d 
previously this would show the active storage blob url and not check for 
authorization
houndci-bot
houndci-bot reviewed Apr 11, 2019
View reviewed changes
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

cannot load such file -- rubocop-performance 
cannot load such file -- rubocop-performance
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `require'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `each'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:43:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

@weiweishi
Copy link
Contributor

@pgwillia @pbinkley this PR looks good to me. I think it's an oversight here previous to not using the proper file_download_item_url helper method to constructure the download URL. This should not be treated any differently than other download URLs.

@weiweishi
Copy link
Contributor

I wonder, here where it uses key, should we also changed it to use signed_id, just for precaution?
https://github.com/ualbertalib/jupiter/blob/master/app/controllers/active_storage/blobs_controller.rb#L8

according to the official document, key should never be revealed to the users?
https://api.rubyonrails.org/classes/ActiveStorage/Blob.html#method-i-key

We however monkey-patched it，and I can't figure out why...
https://github.com/ualbertalib/jupiter/blob/master/config/initializers/active_storage.rb#L6

@pgwillia
Copy link
Member Author

We however monkey-patched it，and I can't figure out why...
https://github.com/ualbertalib/jupiter/blob/master/config/initializers/active_storage.rb#L6

I wonder if it was for backward compatible thumbnails?

jupiter/config/routes.rb

Line 115 in e8bf60a

get '/rails/blobs/:key', to: redirect('/rails/active_storage/blobs/%{key}/thumbnail.jpg')

@pgwillia
Copy link
Member Author

pgwillia commented Apr 12, 2019 •
edited
Loading

We however monkey-patched it，and I can't figure out why...
https://github.com/ualbertalib/jupiter/blob/master/config/initializers/active_storage.rb#L6

https://github.com/ualbertalib/jupiter/pull/718/files#r185930910
#718 (comment)

@pgwillia
Copy link
Member Author

#631 (comment)

@pbinkley
Copy link
Member

So ActiveStorage wanted to meet the use-case of providing short-lived download links, which did not suit our use-case because we want thumbnails etc. to be cacheable, so we by-passed it. Except now we don't want the old ones to work any more. But we've already revealed the keys in the harvested urls. Is the key changeable?

houndci-bot
houndci-bot reviewed Apr 12, 2019
View reviewed changes
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

cannot load such file -- rubocop-performance 
cannot load such file -- rubocop-performance
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `require'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `each'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:43:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

houndci-bot
houndci-bot reviewed Apr 12, 2019
View reviewed changes
Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

cannot load such file -- rubocop-performance 
cannot load such file -- rubocop-performance
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `require'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `each'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:43:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

@pbinkley pbinkley self-requested a review April 12, 2019 19:42
pbinkley
pbinkley approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@pbinkley pbinkley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm merging this now, and we'll consider whether to cut a release and deploy early next week, or to wait for the authentication fix, based on progress made today.

@pbinkley pbinkley merged commit f410e69 into ualbertalib:master Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@pbinkley pbinkley pbinkley approved these changes

@weiweishi weiweishi Awaiting requested review from weiweishi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pgwillia @pbinkley @weiweishi @houndci-bot