Merge branch 'master' into 14221-oauth_pretty_consent

Makefile

@@ -257,6 +257,7 @@ SPEC_HELPER_MIN_SPECS = \
 	spec/models/carto/oauth_access_token_spec.rb \
 	spec/models/carto/oauth_app_spec.rb \
 	spec/models/carto/oauth_app_user_spec.rb \
+	spec/models/carto/oauth_app_organization_spec.rb \
 	spec/models/carto/oauth_authorization_code_spec.rb \
 	spec/models/carto/oauth_refresh_token_spec.rb \
 	spec/models/carto/overlay_spec.rb \

NEWS.md

@@ -6,14 +6,25 @@ Development
 
 ### Features
 - OAuth provider: You can authenticate an external app against CARTO using OAuth, and get an API Key for the authorized user (WIP)
-  - Redirect back to OAuth flow after login (#14236)
-  - Implicit flow (#14167)
+  - Add new design for OAuth consent screen (#14237)
 
 ### Bug fixes / enhancements
-- Fix lots of requests triggered in datasets view (https://github.com/CartoDB/cartodb/issues/14190)
-- Hide like button if the user is not logged in (https://github.com/CartoDB/cartodb/issues/13098)
-- Fix OAuth login for the organizations (#14238)
-- Add new design for OAuth consent screen (#14237)
+- None yet
+
+4.20.1 (2018-08-24)
+-------------------
+
+### Features
+* OAuth provider: You can authenticate an external app against CARTO using OAuth, and get an API Key for the authorized user (WIP)
+  * Redirect back to OAuth flow after login (#14236)
+  * Implicit flow (#14167)
+  * Allow restricting application to only a set of organizations (#14180)
+
+### Bug fixes / enhancements
+* Fix lots of requests triggered in datasets view (https://github.com/CartoDB/cartodb/issues/14190)
+* Hide like button if the user is not logged in (https://github.com/CartoDB/cartodb/issues/13098)
+* Fix OAuth login for the organizations (#14238)
+* Better OAuth error management (#14214)
 
 4.20.0 (2018-08-13)
 -------------------

app/controllers/application_controller.rb

@@ -411,6 +411,12 @@ def update_user_last_activity
     current_user.set_last_ip_address request.remote_ip
   end
 
+  def ensure_required_params(required_params)
+    params_with_value = params.reject { |_, v| v.empty? }
+    missing_params = required_params - params_with_value.keys
+    raise Carto::MissingParamsError.new(missing_params) unless missing_params.empty?
+  end
+
   protected :current_user
 
   def json_formatted_request?

app/controllers/carto/oauth_provider_controller.rb

@@ -11,11 +11,15 @@ class OauthProviderController < ApplicationController
       'authorization_code' => OauthProvider::GrantStrategies::AuthorizationCodeStrategy,
       'refresh_token' => OauthProvider::GrantStrategies::RefreshTokenStrategy
     }.freeze
+
     RESPONSE_STRATEGIES = {
       'code' => OauthProvider::ResponseStrategies::CodeStrategy,
       'token' => OauthProvider::ResponseStrategies::TokenStrategy
     }.freeze
 
+    REQUIRED_TOKEN_PARAMS = ['client_id', 'client_secret', 'grant_type'].freeze
+    REQUIRED_AUTHORIZE_PARAMS = ['client_id', 'state', 'response_type'].freeze
+
     ssl_required
 
     layout 'frontend'
@@ -25,18 +29,28 @@ class OauthProviderController < ApplicationController
 
     before_action :login_required_any_user, only: [:consent, :authorize]
     before_action :set_redirection_error_handling, only: [:consent, :authorize]
+    before_action :ensure_required_token_params, only: [:token]
     before_action :load_oauth_app, :verify_redirect_uri
-    before_action :validate_response_type, :validate_scopes, :ensure_state, only: [:consent, :authorize]
+    before_action :reject_client_secret, only: [:consent, :authorize]
+    before_action :ensure_required_authorize_params, only: [:consent, :authorize]
+    before_action :validate_response_type, :validate_scopes, :set_state, only: [:consent, :authorize]
     before_action :load_oauth_app_user, only: [:consent, :authorize]
     before_action :validate_grant_type, :verify_client_secret, only: [:token]
 
     rescue_from StandardError, with: :rescue_generic_errors
+    rescue_from Carto::MissingParamsError, with: :rescue_missing_params_error
     rescue_from OauthProvider::Errors::BaseError, with: :rescue_oauth_errors
 
     def consent
       return create_authorization_code if @oauth_app_user.try(:authorized?, @scopes)
 
-      @scopes_by_category = OauthProvider::Scopes.scopes_by_category(@scopes, @oauth_app_user.try(:scopes))
+      if @oauth_app_user
+        return create_authorization_code if @oauth_app_user.authorized?(@scopes)
+      elsif @oauth_app
+        oauth_app_user = @oauth_app.oauth_app_users.new(user_id: current_viewer.id, scopes: @scopes)
+        validate_oauth_app_user(oauth_app_user)
+        @scopes_by_category = OauthProvider::Scopes.scopes_by_category(@scopes, @oauth_app_user.try(:scopes))
+      end
     end
 
     def authorize
@@ -45,7 +59,9 @@ def authorize
       if @oauth_app_user
         @oauth_app_user.upgrade!(@scopes)
       else
-        @oauth_app_user = @oauth_app.oauth_app_users.create!(user_id: current_viewer.id, scopes: @scopes)
+        @oauth_app_user = @oauth_app.oauth_app_users.new(user_id: current_viewer.id, scopes: @scopes)
+        validate_oauth_app_user(@oauth_app_user)
+        @oauth_app_user.save!
       end
 
       create_authorization_code
@@ -95,6 +111,10 @@ def rescue_generic_errors(exception)
       rescue_oauth_errors(OauthProvider::Errors::ServerError.new)
     end
 
+    def rescue_missing_params_error(exception)
+      rescue_oauth_errors(OauthProvider::Errors::InvalidRequest.new(exception.message))
+    end
+
     def validate_response_type
       @response_type = params[:response_type]
 
@@ -116,7 +136,7 @@ def verify_redirect_uri
       @redirect_uri = params[:redirect_uri].presence
       if @redirect_uri.present? && !@oauth_app.redirect_uris.include?(@redirect_uri)
         @redirect_uri = nil
-        raise OauthProvider::Errors::InvalidRequest.new('The redirect_uri is not authorized for this application')
+        raise OauthProvider::Errors::InvalidRequest.new('The redirect_uri must match the redirect_uri param used in the authorization request')
       end
     end
 
@@ -127,9 +147,8 @@ def validate_scopes
       raise OauthProvider::Errors::InvalidScope.new(invalid_scopes) if invalid_scopes.present?
     end
 
-    def ensure_state
+    def set_state
       @state = params[:state]
-      raise OauthProvider::Errors::InvalidRequest.new('state is mandatory') unless @state.present?
     end
 
     def load_oauth_app_user
@@ -140,6 +159,26 @@ def verify_client_secret
       raise OauthProvider::Errors::InvalidClient.new unless params[:client_secret] == @oauth_app.client_secret
     end
 
+    def ensure_required_token_params
+      grant_params = grant_strategy.try(:required_params) || []
+      ensure_required_params(REQUIRED_TOKEN_PARAMS + grant_params)
+    end
+
+    def ensure_required_authorize_params
+      ensure_required_params(REQUIRED_AUTHORIZE_PARAMS)
+    end
+
+    def reject_client_secret
+      raise OauthProvider::Errors::InvalidRequest.new("The client_secret param must not be sent in the authorize request") if params[:client_secret].present?
+    end
+
+    def validate_oauth_app_user(oauth_app_user)
+      unless oauth_app_user.valid?
+        errors = oauth_app_user.errors.full_messages_for(:user)
+        raise OauthProvider::Errors::AccessDenied.new(errors.join(', ')) if errors.present?
+      end
+    end
+
     def grant_strategy
       GRANT_STRATEGIES[params[:grant_type]]
     end

app/models/carto/oauth_app.rb

@@ -8,13 +8,15 @@ class OauthApp < ActiveRecord::Base
 
     belongs_to :user, inverse_of: :oauth_apps
     has_many :oauth_app_users, inverse_of: :oauth_app, dependent: :destroy
+    has_many :oauth_app_organizations, inverse_of: :oauth_app, dependent: :destroy
 
     validates :user, presence: true
     validates :name, presence: true
     validates :client_id, presence: true
     validates :client_secret, presence: true
     validates :redirect_uris, presence: true
     validates :icon_url, presence: true
+    validates :oauth_app_organizations, absence: true, unless: :restricted?
     validate :validate_uris
 
     before_validation :ensure_keys_generated

app/models/carto/oauth_app_organization.rb

@@ -0,0 +1,16 @@
+# encoding: utf-8
+
+module Carto
+  class OauthAppOrganization < ActiveRecord::Base
+    belongs_to :organization, inverse_of: :oauth_app_organizations
+    belongs_to :oauth_app, inverse_of: :oauth_app_organizations
+
+    validates :organization, presence: true, uniqueness: { scope: :oauth_app }
+    validates :oauth_app, presence: true
+    validates :seats, presence: true, numericality: { only_integer: true, greater_than: 0 }
+
+    def open_seats?
+      oauth_app.oauth_app_users.joins(:user).where(users: { organization_id: organization.id }).count < seats
+    end
+  end
+end

app/models/carto/oauth_app_user.rb

@@ -1,6 +1,7 @@
 # encoding: utf-8
 
 require_dependency 'carto/oauth_provider/scopes'
+require_dependency 'carto/oauth_provider/errors'
 
 module Carto
   class OauthAppUser < ActiveRecord::Base
@@ -15,6 +16,7 @@ class OauthAppUser < ActiveRecord::Base
     validates :user, presence: true, uniqueness: { scope: :oauth_app }
     validates :oauth_app, presence: true
     validates :scopes, scopes: true
+    validate  :validate_user_authorizable, on: :create
 
     def authorized?(requested_scopes)
       (requested_scopes - scopes).empty?
@@ -23,5 +25,21 @@ def authorized?(requested_scopes)
     def upgrade!(requested_scopes)
       update!(scopes: scopes | requested_scopes)
     end
+
+    private
+
+    def validate_user_authorizable
+      return unless oauth_app.try(:restricted?)
+
+      organization = user.organization
+      return errors.add(:user, 'is not part of an organization') unless user.organization
+
+      org_authorization = oauth_app.oauth_app_organizations.where(organization: organization).first
+      unless org_authorization
+        return errors.add(:user, 'is part of an organization which is not allowed access to this application')
+      end
+
+      errors.add(:user, 'does not have an available seat to use this application') unless org_authorization.open_seats?
+    end
   end
 end

app/models/carto/organization.rb

@@ -20,6 +20,7 @@ class Organization < ActiveRecord::Base
     has_many :assets, class_name: Carto::Asset, dependent: :destroy, inverse_of: :organization
     has_many :notifications, -> { order('created_at DESC') }, dependent: :destroy
     has_many :connector_configurations, inverse_of: :organization, dependent: :destroy
+    has_many :oauth_app_organizations, inverse_of: :oauth_app, dependent: :destroy
 
     before_destroy :destroy_groups_with_extension
 

db/migrate/20180821144500_create_oauth_app_organizations.rb

@@ -0,0 +1,23 @@
+require 'carto/db/migration_helper'
+
+include Carto::Db::MigrationHelper
+
+migration(
+  Proc.new do
+    create_table :oauth_app_organizations do
+      Uuid        :id, primary_key: true, default: 'uuid_generate_v4()'.lit
+      foreign_key :oauth_app_id, :oauth_apps, type: :uuid, null: false, index: true, on_delete: :cascade
+      foreign_key :organization_id, :organizations, type: :uuid, null: false, index: true, on_delete: :cascade
+      Integer     :seats, null: false
+      DateTime    :created_at, null: false, default: Sequel::CURRENT_TIMESTAMP
+      DateTime    :updated_at, null: false, default: Sequel::CURRENT_TIMESTAMP
+    end
+
+    add_column :oauth_apps, :restricted, :boolean, null: false, default: false
+  end,
+  Proc.new do
+    drop_column :oauth_apps, :restricted
+
+    drop_table :oauth_app_organizations
+  end
+)

lib/carto/errors.rb

@@ -58,4 +58,10 @@ def initialize(message, status = 422)
       super(message, status)
     end
   end
+
+  class MissingParamsError < CartoError
+    def initialize(missing_params, status: 400)
+      super("The following required params are missing: #{missing_params.join(', ')}", status)
+    end
+  end
 end

lib/carto/oauth_provider/errors.rb

@@ -40,8 +40,8 @@ def initialize(scopes, message: nil)
       end
 
       class AccessDenied < BaseError
-        def initialize
-          super('access_denied', 'The user rejected the authentication request')
+        def initialize(message = 'The user rejected the authentication request')
+          super('access_denied', message)
         end
       end
 

lib/carto/oauth_provider/grant_strategies.rb

@@ -15,6 +15,10 @@ def self.authorize!(oauth_app, params)
         rescue ActiveRecord::RecordNotFound
           raise OauthProvider::Errors::InvalidGrant.new
         end
+
+        def self.required_params
+          ['code']
+        end
       end
 
       module RefreshTokenStrategy
@@ -30,6 +34,10 @@ def self.authorize!(oauth_app, params)
         rescue ActiveRecord::RecordNotFound
           raise OauthProvider::Errors::InvalidGrant.new
         end
+
+        def self.required_params
+          ['refresh_token']
+        end
       end
     end
   end

spec/factories/organizations.rb

@@ -57,5 +57,25 @@
         }.stringify_keys
       end
     end
+
+    factory :carto_organization, class: Carto::Organization do
+      to_create(&:save!)
+
+      name { unique_name('organization') }
+      seats 10
+      quota_in_bytes 100.megabytes
+      geocoding_quota 1000
+      here_isolines_quota 1000
+      obs_snapshot_quota 1000
+      obs_general_quota 1000
+      map_view_quota 100000
+      website 'carto.com'
+      description 'Lorem ipsum dolor sit amet'
+      display_name 'Vizzuality Inc'
+      discus_shortname 'cartodb'
+      twitter_username 'cartodb'
+      location 'Madrid'
+      builder_enabled false # Most tests still assume editor
+    end
   end
 end

spec/factories/organizations_contexts.rb

@@ -60,6 +60,7 @@ def test_organization
     @organization.reload
 
     @carto_organization = Carto::Organization.find(@organization.id)
+    @carto_organization_2 = Carto::Organization.find(@organization_2.id)
     @carto_org_user_owner = Carto::User.find(@org_user_owner.id)
     @carto_org_user_1 = Carto::User.find(@org_user_1.id)
     @carto_org_user_2 = Carto::User.find(@org_user_2.id)