From a9e40a3d80435431f689b8d19005dd77a8f50224 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 22 Oct 2016 19:38:47 +0200 Subject: [PATCH] Adding OAuth access scopes, fixing OAuth authorization UI, adding rate limiting to the API --- .rubocop.yml | 54 +++++++++++- app/assets/stylesheets/forms.scss | 88 ++++++++++--------- app/channels/public_channel.rb | 2 +- app/controllers/api/v1/accounts_controller.rb | 4 +- app/controllers/api/v1/follows_controller.rb | 2 +- app/controllers/api/v1/media_controller.rb | 2 +- app/controllers/api/v1/statuses_controller.rb | 4 +- app/controllers/api_controller.rb | 22 +++++ app/controllers/home_controller.rb | 2 +- .../oauth/authorizations_controller.rb | 9 ++ app/models/feed.rb | 2 +- app/models/media_attachment.rb | 3 +- .../doorkeeper/authorizations/error.html.haml | 4 - .../doorkeeper/authorizations/new.html.haml | 26 ------ .../doorkeeper/authorizations/show.html.haml | 2 - .../oauth/authorizations/error.html.haml | 2 + app/views/oauth/authorizations/new.html.haml | 25 ++++++ app/views/oauth/authorizations/show.html.haml | 1 + config/environments/production.rb | 2 +- config/initializers/doorkeeper.rb | 4 +- config/initializers/rabl_init.rb | 2 +- config/initializers/rack-attack.rb | 18 +++- config/locales/doorkeeper.en.yml | 4 + config/routes.rb | 4 +- db/seeds.rb | 2 +- lib/tasks/mastodon.rake | 4 +- 26 files changed, 195 insertions(+), 99 deletions(-) create mode 100644 app/controllers/oauth/authorizations_controller.rb delete mode 100644 app/views/doorkeeper/authorizations/error.html.haml delete mode 100644 app/views/doorkeeper/authorizations/new.html.haml delete mode 100644 app/views/doorkeeper/authorizations/show.html.haml create mode 100644 app/views/oauth/authorizations/error.html.haml create mode 100644 app/views/oauth/authorizations/new.html.haml create mode 100644 app/views/oauth/authorizations/show.html.haml diff --git a/.rubocop.yml b/.rubocop.yml index 3a638a009..15ba678d4 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,14 +1,60 @@ Rails: Enabled: true -Metrics/LineLength: - Enabled: false - Style/PerlBackrefs: AutoCorrect: false Style/ClassAndModuleChildren: Enabled: false -Documentation: +Metrics/BlockNesting: + Max: 2 + +Metrics/LineLength: + AllowURI: true Enabled: false + +Metrics/MethodLength: + CountComments: false + Max: 10 + +Metrics/ModuleLength: + Max: 100 + +Metrics/ParameterLists: + Max: 4 + CountKeywordArgs: true + +Style/AccessModifierIndentation: + EnforcedStyle: indent + +Style/CollectionMethods: + Enabled: true + PreferredMethods: + find_all: 'select' + +Style/Documentation: + Enabled: false + +Style/DoubleNegation: + Enabled: false + +Style/FrozenStringLiteralComment: + Enabled: false + +Style/SpaceInsideHashLiteralBraces: + EnforcedStyle: space + +Style/TrailingCommaInLiteral: + EnforcedStyleForMultiline: 'comma' + +Style/RegexpLiteral: + Enabled: false + +AllCops: + TargetRubyVersion: 2.2 + Exclude: + - 'spec/**/*' + - 'db/**/*' + - 'app/views/**/*' + - 'config/**/*' diff --git a/app/assets/stylesheets/forms.scss b/app/assets/stylesheets/forms.scss index 8abbbdaee..f7677ac9d 100644 --- a/app/assets/stylesheets/forms.scss +++ b/app/assets/stylesheets/forms.scss @@ -85,18 +85,7 @@ } } - .prompt { - font-size: 16px; - color: #9baec8; - text-align: center; - - .prompt-highlight { - font-weight: 500; - color: #fff; - } - } - - code.copypasteable { + code { display: block; font-family: 'Roboto Mono', monospace; font-weight: 400; @@ -110,42 +99,42 @@ .actions { margin-top: 30px; + } - button { - display: block; - width: 100%; - border: 0; - border-radius: 4px; - background: #2b90d9; - color: #fff; - font-size: 18px; - padding: 10px; - text-transform: uppercase; - cursor: pointer; - font-weight: 500; - outline: 0; - margin-bottom: 10px; + button { + display: block; + width: 100%; + border: 0; + border-radius: 4px; + background: #2b90d9; + color: #fff; + font-size: 18px; + padding: 10px; + text-transform: uppercase; + cursor: pointer; + font-weight: 500; + outline: 0; + margin-bottom: 10px; + + &:hover { + background-color: lighten(#2b90d9, 5%); + } + + &:active, &:focus { + position: relative; + top: 1px; + background-color: darken(#2b90d9, 5%); + } + + &.negative { + background: #df405a; &:hover { - background-color: lighten(#2b90d9, 5%); + background-color: lighten(#df405a, 5%); } &:active, &:focus { - position: relative; - top: 1px; - background-color: darken(#2b90d9, 5%); - } - - &.negative { - background: #df405a; - - &:hover { - background-color: lighten(#df405a, 5%); - } - - &:active, &:focus { - background-color: darken(#df405a, 5%); - } + background-color: darken(#df405a, 5%); } } } @@ -180,3 +169,18 @@ } } +.oauth-prompt { + margin-bottom: 30px; + text-align: center; + color: #9baec8; + + h2 { + font-size: 16px; + margin-bottom: 30px; + } + + strong { + color: #d9e1e8; + font-weight: 500; + } +} diff --git a/app/channels/public_channel.rb b/app/channels/public_channel.rb index 5d7fadc1d..578cdc001 100644 --- a/app/channels/public_channel.rb +++ b/app/channels/public_channel.rb @@ -1,7 +1,7 @@ # Be sure to restart your server when you modify this file. Action Cable runs in a loop that does not support auto reloading. class PublicChannel < ApplicationCable::Channel def subscribed - stream_from 'timeline:public', -> (encoded_message) do + stream_from 'timeline:public', lambda do |encoded_message| message = ActiveSupport::JSON.decode(encoded_message) status = Status.find_by(id: message['id']) diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 2669315e2..bb3e54a89 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,5 +1,7 @@ class Api::V1::AccountsController < ApiController - before_action :doorkeeper_authorize! + before_action -> { doorkeeper_authorize! :read }, except: [:follow, :unfollow, :block, :unblock] + before_action -> { doorkeeper_authorize! :follow }, only: [:follow, :unfollow, :block, :unblock] + before_action :set_account, except: [:verify_credentials, :suggestions] respond_to :json diff --git a/app/controllers/api/v1/follows_controller.rb b/app/controllers/api/v1/follows_controller.rb index 739ac1fb1..9181cd077 100644 --- a/app/controllers/api/v1/follows_controller.rb +++ b/app/controllers/api/v1/follows_controller.rb @@ -1,5 +1,5 @@ class Api::V1::FollowsController < ApiController - before_action :doorkeeper_authorize! + before_action -> { doorkeeper_authorize! :follow } respond_to :json def create diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index 7efe38bd8..dffc797fe 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -1,5 +1,5 @@ class Api::V1::MediaController < ApiController - before_action :doorkeeper_authorize! + before_action -> { doorkeeper_authorize! :write } respond_to :json def create diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index a7305233e..b02b7bb57 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -1,5 +1,7 @@ class Api::V1::StatusesController < ApiController - before_action :doorkeeper_authorize! + before_action -> { doorkeeper_authorize! :read }, except: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite] + before_action -> { doorkeeper_authorize! :write }, only: [:create, :destroy, :reblog, :unreblog, :favourite, :unfavourite] + respond_to :json def show diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index e29892cbe..0776f4ce8 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -1,7 +1,10 @@ class ApiController < ApplicationController protect_from_forgery with: :null_session + skip_before_action :verify_authenticity_token + before_action :set_rate_limit_headers + rescue_from ActiveRecord::RecordInvalid do |e| render json: { error: e.to_s }, status: 422 end @@ -22,8 +25,27 @@ class ApiController < ApplicationController render json: { error: 'Remote SSL certificate could not be verified' }, status: 503 end + def doorkeeper_unauthorized_render_options(*) + { json: { error: 'Not authorized' } } + end + + def doorkeeper_forbidden_render_options(*) + { json: { error: 'This action is outside the authorized scopes' } } + end + protected + def set_rate_limit_headers + return if request.env['rack.attack.throttle_data'].nil? + + now = Time.now.utc + match_data = request.env['rack.attack.throttle_data']['api'] + + response.headers['X-RateLimit-Limit'] = match_data[:limit].to_s + response.headers['X-RateLimit-Remaining'] = (match_data[:limit] - match_data[:count]).to_s + response.headers['X-RateLimit-Reset'] = (now + (match_data[:period] - now.to_i % match_data[:period])).to_s + end + def current_resource_owner User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token end diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index 4e6b2a879..8ed88d074 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -15,6 +15,6 @@ class HomeController < ApplicationController end def find_or_create_access_token - Doorkeeper::AccessToken.find_or_create_for(Doorkeeper::Application.where(superapp: true).first, current_user.id, nil, Doorkeeper.configuration.access_token_expires_in, Doorkeeper.configuration.refresh_token_enabled?) + Doorkeeper::AccessToken.find_or_create_for(Doorkeeper::Application.where(superapp: true).first, current_user.id, 'read write follow', Doorkeeper.configuration.access_token_expires_in, Doorkeeper.configuration.refresh_token_enabled?) end end diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb new file mode 100644 index 000000000..f5f05814e --- /dev/null +++ b/app/controllers/oauth/authorizations_controller.rb @@ -0,0 +1,9 @@ +class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController + before_action :store_current_location + + private + + def store_current_location + store_location_for(:user, request.url) + end +end diff --git a/app/models/feed.rb b/app/models/feed.rb index 2bc9e980a..211d64638 100644 --- a/app/models/feed.rb +++ b/app/models/feed.rb @@ -7,7 +7,7 @@ class Feed def get(limit, max_id = nil, since_id = nil) max_id = '+inf' if max_id.blank? since_id = '-inf' if since_id.blank? - unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).collect(&:last).map(&:to_i) + unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).map(&:last).map(&:to_i) # If we're after most recent items and none are there, we need to precompute the feed if unhydrated.empty? && max_id == '+inf' && since_id == '-inf' diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index ab6c91efb..008147a08 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -34,7 +34,8 @@ class MediaAttachment < ApplicationRecord image? ? 'image' : 'video' end -private + private + def self.file_styles(f) if f.instance.image? { diff --git a/app/views/doorkeeper/authorizations/error.html.haml b/app/views/doorkeeper/authorizations/error.html.haml deleted file mode 100644 index cb97ae170..000000000 --- a/app/views/doorkeeper/authorizations/error.html.haml +++ /dev/null @@ -1,4 +0,0 @@ -.prompt= t('doorkeeper.authorizations.error.title') - -#error_explanation - = @pre_auth.error_response.body[:error_description] diff --git a/app/views/doorkeeper/authorizations/new.html.haml b/app/views/doorkeeper/authorizations/new.html.haml deleted file mode 100644 index 91d71cc01..000000000 --- a/app/views/doorkeeper/authorizations/new.html.haml +++ /dev/null @@ -1,26 +0,0 @@ -.prompt= raw t('.prompt', client_name: "#{ @pre_auth.client.name }") - -/- if @pre_auth.scopes.count > 0 -/ .scope-permission-prompt -/ %p= t('.able_to') - -/ %ul.scope-permissions -/ - @pre_auth.scopes.each do |scope| -/ %li= t scope, scope: [:doorkeeper, :scopes] - -.actions - = form_tag oauth_authorization_path, method: :post do - = hidden_field_tag :client_id, @pre_auth.client.uid - = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri - = hidden_field_tag :state, @pre_auth.state - = hidden_field_tag :response_type, @pre_auth.response_type - = hidden_field_tag :scope, @pre_auth.scope - = button_tag t('doorkeeper.authorizations.buttons.authorize'), type: :submit - - = form_tag oauth_authorization_path, method: :delete do - = hidden_field_tag :client_id, @pre_auth.client.uid - = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri - = hidden_field_tag :state, @pre_auth.state - = hidden_field_tag :response_type, @pre_auth.response_type - = hidden_field_tag :scope, @pre_auth.scope - = button_tag t('doorkeeper.authorizations.buttons.deny'), type: :submit, class: 'negative' diff --git a/app/views/doorkeeper/authorizations/show.html.haml b/app/views/doorkeeper/authorizations/show.html.haml deleted file mode 100644 index 44638318b..000000000 --- a/app/views/doorkeeper/authorizations/show.html.haml +++ /dev/null @@ -1,2 +0,0 @@ -.prompt= t('.title') -%code.copypasteable= params[:code] diff --git a/app/views/oauth/authorizations/error.html.haml b/app/views/oauth/authorizations/error.html.haml new file mode 100644 index 000000000..ee72d9740 --- /dev/null +++ b/app/views/oauth/authorizations/error.html.haml @@ -0,0 +1,2 @@ +.flash-message#error_explanation + = @pre_auth.error_response.body[:error_description] diff --git a/app/views/oauth/authorizations/new.html.haml b/app/views/oauth/authorizations/new.html.haml new file mode 100644 index 000000000..ba5d426f5 --- /dev/null +++ b/app/views/oauth/authorizations/new.html.haml @@ -0,0 +1,25 @@ +.oauth-prompt + %h2 + Application + %strong=@pre_auth.client.name + requests access to your account + + %p + It will be able to + = @pre_auth.scopes.map { |scope| t(scope, scope: [:doorkeeper, :scopes]) }.map { |s| "#{s}"}.to_sentence.html_safe + += form_tag oauth_authorization_path, method: :post, class: 'simple_form' do + = hidden_field_tag :client_id, @pre_auth.client.uid + = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri + = hidden_field_tag :state, @pre_auth.state + = hidden_field_tag :response_type, @pre_auth.response_type + = hidden_field_tag :scope, @pre_auth.scope + = button_tag t('doorkeeper.authorizations.buttons.authorize'), type: :submit + += form_tag oauth_authorization_path, method: :delete, class: 'simple_form' do + = hidden_field_tag :client_id, @pre_auth.client.uid + = hidden_field_tag :redirect_uri, @pre_auth.redirect_uri + = hidden_field_tag :state, @pre_auth.state + = hidden_field_tag :response_type, @pre_auth.response_type + = hidden_field_tag :scope, @pre_auth.scope + = button_tag t('doorkeeper.authorizations.buttons.deny'), type: :submit, class: 'negative' diff --git a/app/views/oauth/authorizations/show.html.haml b/app/views/oauth/authorizations/show.html.haml new file mode 100644 index 000000000..d1a3e1f81 --- /dev/null +++ b/app/views/oauth/authorizations/show.html.haml @@ -0,0 +1 @@ +%code= params[:code] diff --git a/config/environments/production.rb b/config/environments/production.rb index 8ac857a19..b90505f68 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -12,7 +12,7 @@ Rails.application.configure do # Full error reports are disabled and caching is turned on. config.consider_all_requests_local = false - config.action_controller.perform_caching = false + config.action_controller.perform_caching = true # Disable serving static files from the `/public` folder by default since # Apache or NGINX already handles this. diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 97eaea678..16297456e 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -50,8 +50,8 @@ Doorkeeper.configure do # Define access token scopes for your provider # For more information go to # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes - # default_scopes :public - # optional_scopes :write, :follow + default_scopes :read + optional_scopes :write, :follow # Change the way client credentials are retrieved from the request object. # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then diff --git a/config/initializers/rabl_init.rb b/config/initializers/rabl_init.rb index d14f8c54e..325bf0c78 100644 --- a/config/initializers/rabl_init.rb +++ b/config/initializers/rabl_init.rb @@ -1,5 +1,5 @@ Rabl.configure do |config| - config.cache_all_output = true + config.cache_all_output = false config.cache_sources = !!Rails.env.production? config.include_json_root = false config.view_paths = [Rails.root.join('app/views')] diff --git a/config/initializers/rack-attack.rb b/config/initializers/rack-attack.rb index fb447685b..6d9286e66 100644 --- a/config/initializers/rack-attack.rb +++ b/config/initializers/rack-attack.rb @@ -1,9 +1,19 @@ class Rack::Attack - throttle('get-req/ip', limit: 300, period: 5.minutes) do |req| - req.ip if req.get? + # Rate limits for the API + throttle('api', limit: 150, period: 5.minutes) do |req| + req.ip if req.path.match(/\A\/api\//) end - throttle('post-req/ip', limit: 100, period: 5.minutes) do |req| - req.ip if req.post? + self.throttled_response = lambda do |env| + now = Time.now.utc + match_data = env['rack.attack.match_data'] + + headers = { + 'X-RateLimit-Limit' => match_data[:limit].to_s, + 'X-RateLimit-Remaining' => '0', + 'X-RateLimit-Reset' => (now + (match_data[:period] - now.to_i % match_data[:period])).to_s + } + + [429, headers, [{ error: 'Throttled' }.to_json]] end end diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index 7d2d215da..e4444c972 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -15,6 +15,10 @@ en: secured_uri: 'must be an HTTPS/SSL URI.' doorkeeper: + scopes: + read: read your account's data + write: post on your behalf + follow: follow, block, unblock and unfollow accounts applications: confirmations: destroy: 'Are you sure?' diff --git a/config/routes.rb b/config/routes.rb index f3708938a..b5abd56af 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -7,7 +7,9 @@ Rails.application.routes.draw do mount Sidekiq::Web => '/sidekiq' end - use_doorkeeper + use_doorkeeper do + controllers authorizations: 'oauth/authorizations' + end get '.well-known/host-meta', to: 'xrd#host_meta', as: :host_meta get '.well-known/webfinger', to: 'xrd#webfinger', as: :webfinger diff --git a/db/seeds.rb b/db/seeds.rb index 7e8ee8e44..a3e75f274 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -1,2 +1,2 @@ -web_app = Doorkeeper::Application.new(name: 'Web', superapp: true, redirect_uri: Doorkeeper.configuration.native_redirect_uri) +web_app = Doorkeeper::Application.new(name: 'Web', superapp: true, redirect_uri: Doorkeeper.configuration.native_redirect_uri, scopes: 'read write follow') web_app.save! diff --git a/lib/tasks/mastodon.rake b/lib/tasks/mastodon.rake index 78def7b51..94da0904c 100644 --- a/lib/tasks/mastodon.rake +++ b/lib/tasks/mastodon.rake @@ -2,9 +2,7 @@ namespace :mastodon do namespace :media do desc 'Removes media attachments that have not been assigned to any status for longer than a day' task clear: :environment do - MediaAttachment.where(status_id: nil).where('created_at < ?', 1.day.ago).find_each do |m| - m.destroy - end + MediaAttachment.where(status_id: nil).where('created_at < ?', 1.day.ago).find_each(&:destroy) end end