Merge pull request #4344 from oliverbarnes/4124-check-twitter-write-access-before-auth

Issue #4124 Check write access before authorizing Twitter

Changelog.md

@@ -3,6 +3,7 @@
 **Attention:** This release includes a potentially long running migration! However it should be safe to run this while keeping your application servers on.
 
 ## Refactor
+* Service and ServiceController, general code reorg to make it cleaner/+ testable/+ extensible [#4344](https://github.com/diaspora/diaspora/pull/4344)
 * Background actual mailing when sending invitations [#4069](https://github.com/diaspora/diaspora/issues/4069)
 * Set the current user on the client side through gon [#4028](https://github.com/diaspora/diaspora/issues/4028)
 * Update sign out route to a DELETE request [#4068](https://github.com/diaspora/diaspora/issues/4068)
@@ -11,6 +12,7 @@
 * Refactor left bar side menu, improve tag autosuggestion design [#4271](https://github.com/diaspora/diaspora/issues/4271), [#4316](https://github.com/diaspora/diaspora/pull/4316)
 
 ## Bug fixes
+* Check twitter write access before adding/authorizing it for a user. [#4124](https://github.com/diaspora/diaspora/issues/4124)
 * Don't focus comment form on 'show n more comments' [#4265](https://github.com/diaspora/diaspora/issues/4265)
 * Do not render mobile photo view for none-existing photos [#4194](https://github.com/diaspora/diaspora/issues/4194)
 * Render markdown content for prettier email subjects and titles [#4182](https://github.com/diaspora/diaspora/issues/4182)

app/controllers/services_controller.rb

@@ -7,8 +7,8 @@ class ServicesController < ApplicationController
   # See https://github.com/intridea/omniauth/issues/203
   # See also http://www.communityguides.eu/articles/16
   skip_before_filter :verify_authenticity_token, :only => :create
-
   before_filter :authenticate_user!
+  before_filter :abort_if_already_authorized, :abort_if_read_only_access, :only => :create
 
   respond_to :html
   respond_to :json, :only => :inviter
@@ -17,43 +17,19 @@ class ServicesController < ApplicationController
     @services = current_user.services
   end
 
-  def create
+  def create 
-    auth = request.env['omniauth.auth']
+    service = Service.initialize_from_omniauth( omniauth_hash )
+    
+    if current_user.services << service
+      current_user.update_profile_with_omniauth( service.info )
 
-    toke = auth['credentials']['token']
+      fetch_photo(service) if no_profile_image?
-    secret = auth['credentials']['secret']
-
-    provider = auth['provider']
-    user     = auth['info']
-
-    service = "Services::#{provider.camelize}".constantize.new(:nickname => user['nickname'],
-                                                               :access_token => toke,
-                                                               :access_secret => secret,
-                                                               :uid => auth['uid'])
-    current_user.services << service
-
-    if service.persisted?
-      fetch_photo = current_user.profile[:image_url].blank?
-
-      current_user.update_profile(current_user.profile.from_omniauth_hash(user))
-      Workers::FetchProfilePhoto.perform_async(current_user.id, service.id, user["image"]) if fetch_photo
 
       flash[:notice] = I18n.t 'services.create.success'
     else
       flash[:error] = I18n.t 'services.create.failure'
-
-      if existing_service = Service.where(:type => service.type.to_s, :uid => service.uid).first
-        flash[:error] <<  I18n.t('services.create.already_authorized',
-                                 :diaspora_id => existing_service.user.profile.diaspora_handle,
-                                 :service_name => provider.camelize )
-      end
-    end
-    
-    if request.env['omniauth.origin'].nil?
-      render :text => ("<script>window.close()</script>")
-    else
-      redirect_to request.env['omniauth.origin']
     end
+    redirect_to_origin
   end
 
   def failure
@@ -68,4 +44,50 @@ class ServicesController < ApplicationController
     flash[:notice] = I18n.t 'services.destroy.success'
     redirect_to services_url
   end
+
+  private
+
+  def abort_if_already_authorized
+    if service = Service.where(uid: omniauth_hash['uid']).first
+      flash[:error] =  I18n.t( 'services.create.already_authorized',
+                                  diaspora_id:  service.user.profile.diaspora_handle,
+                                  service_name: service.provider.camelize )
+      redirect_to_origin
+    end
+  end
+
+  def abort_if_read_only_access
+    if header_hash["x_access_level"] && header_hash["x_access_level"] == 'read'
+      flash[:error] =  I18n.t( 'services.create.read_only_access' )
+      redirect_to_origin
+    end
+  end
+
+  def redirect_to_origin
+    if origin 
+      redirect_to origin
+    else 
+      render(text: "<script>window.close()</script>")
+    end
+  end
+
+  def no_profile_image?
+    current_user.profile[:image_url].blank?
+  end
+
+  def fetch_photo(service)
+    Workers::FetchProfilePhoto.perform_async(current_user.id, service.id, service.info["image"])
+  end
+
+  def origin
+    request.env['omniauth.origin']
+  end
+
+  def omniauth_hash 
+    request.env['omniauth.auth']
+  end
+
+  def header_hash 
+    omniauth_hash['extra'] ? omniauth_hash['extra']['access_token']['response']['header'] : {}
+  end 
 end

app/models/profile.rb

@@ -83,7 +83,7 @@ class Profile < ActiveRecord::Base
                'location' =>  'location',
                 }
 
-    update_hash = Hash[omniauth_user_hash.map {|k, v| [mappings[k], v] }]
+    update_hash = Hash[ omniauth_user_hash.map {|k, v| [mappings[k], v] } ]
     
     self.attributes.merge(update_hash){|key, old, new| old.blank? ? new : old}
   end

app/models/service.rb

@@ -5,14 +5,12 @@
 class Service < ActiveRecord::Base
   include ActionView::Helpers::TextHelper
   include MarkdownifyHelper
+
+  attr_accessor :provider, :info, :access_level
   
   belongs_to :user
   validates_uniqueness_of :uid, :scope => :type
 
-  def self.titles(service_strings)
-    service_strings.map{|s| "Services::#{s.titleize}"}
-  end
-
   def profile_photo_url
     nil
   end
@@ -21,4 +19,46 @@ class Service < ActiveRecord::Base
     #don't do anything (should be overriden by service extensions)
   end
 
+  class << self
+
+    def titles(service_strings)
+      service_strings.map {|s| "Services::#{s.titleize}"}
+    end
+
+    def first_from_omniauth( auth_hash )
+      @@auth = auth_hash 
+      where( type: service_type, uid: options[:uid] ).first
+    end
+
+    def initialize_from_omniauth( auth_hash )
+      @@auth = auth_hash 
+      service_type.constantize.new( options )
+    end
+
+    def auth
+      @@auth
+    end
+
+    def service_type
+      "Services::#{options[:provider].camelize}"
+    end
+
+    def access_level
+      auth['extra']['access_token']['response']['header']['x_access_level'] if auth['extra']
+    end
+
+    def options
+      { 
+        nickname:      auth['info']['nickname'],
+        access_token:  auth['credentials']['token'],
+        access_secret: auth['credentials']['secret'],
+        uid:           auth['uid'],
+        provider:      auth['provider'],
+        info:          auth['info'], 
+        access_level:  access_level  
+      }
+    end
+
+    private :auth, :service_type, :access_level, :options
+  end
 end

app/models/user.rb

@@ -336,6 +336,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def update_profile_with_omniauth( user_info )
+    update_profile( self.profile.from_omniauth_hash( user_info ) )
+  end 
+
   def deliver_profile_update
     Postzord::Dispatcher.build(self, profile).post
   end

config/locales/diaspora/en.yml

@@ -760,6 +760,7 @@ en:
       success: "Authentication successful."
       failure: "Authentication failed."
       already_authorized: "A user with diaspora id %{diaspora_id} already authorized that %{service_name} account."
+      read_only_access: "Access level is read-only, please try to authorize again later"
     destroy:
       success: "Successfully deleted authentication."
     failure:

spec/controllers/services_controller_spec.rb

@@ -5,61 +5,78 @@
 require 'spec_helper'
 
 describe ServicesController do
-  let(:mock_access_token) { Object.new }
+  let(:omniauth_auth) do
-
-  let(:omniauth_auth) {
     { 'provider' => 'twitter',
       'uid'      => '2',
       'info'   => { 'nickname' => 'grimmin' },
-      'credentials' => { 'token' => 'tokin', 'secret' =>"not_so_much" }
+      'credentials' => { 'token' => 'tokin', 'secret' =>"not_so_much" }}
-      }
+    end
-  }
+  let(:user) { alice }
 
   before do
-    @user   = alice
+    sign_in :user, user
-    sign_in :user, @user
+    @controller.stub!(:current_user).and_return(user)
-    @controller.stub!(:current_user).and_return(@user)
-    mock_access_token.stub!(:token => "12345", :secret => "56789")
   end
 
   describe '#index' do
-    it 'displays all connected serivices for a user' do
+    before do
-      4.times do
+      FactoryGirl.create(:service, user: user)
-        FactoryGirl.create(:service, :user => @user)
+    end
-      end
 
+    it "displays user's connected services" do
       get :index
-      assigns[:services].should == @user.services
+      assigns[:services].should == user.services
     end
   end
 
   describe '#create' do
-    context 'when not fetching a photo' do
+    before do
-      before do
+      request.env['omniauth.auth'] = omniauth_auth
-        request.env['omniauth.auth'] = omniauth_auth
+      request.env['omniauth.origin'] = root_url
-      end
+    end
 
-      it 'creates a new OmniauthService' do
+    it 'creates a new service and associates it with the current user' do
+      expect {
+        post :create, :provider => 'twitter'
+      }.to change(user.services, :count).by(1)
+    end
+
+    it 'saves the provider' do
+      post :create, :provider => 'twitter'
+      user.reload.services.first.class.name.should == "Services::Twitter"
+    end
+
+    context 'when service exists with the same uid' do
+      before { Services::Twitter.create!(uid: omniauth_auth['uid'], user_id: user.id) }
+
+      it 'doesnt create a new service' do
         expect {
-          post :create, :provider => 'twitter'
+        post :create, :provider => 'twitter'
-        }.to change(@user.services, :count).by(1)
+      }.to_not change(Service, :count).by(1)
       end
 
-      it 'creates a twitter service' do
+      it 'flashes an already_authorized error with the diaspora handle for the user'  do
-        Service.delete_all
-        @user.getting_started = false
         post :create, :provider => 'twitter'
-        @user.reload.services.first.class.name.should == "Services::Twitter"
+        flash[:error].include?(user.profile.diaspora_handle).should be_true
+        flash[:error].include?( 'already authorized' ).should be_true
+      end
+    end
+
+    context 'when the access-level is read-only' do
+      before do 
+        access_level_hash = { 'extra' => { 'access_token' => { 'response' => { 'header' => { 'x_access_level' => 'read' }}}}}
+        request.env['omniauth.auth'] = omniauth_auth["info"].merge!( access_level_hash )
       end
 
-      it 'returns error if the user already a service with that uid' do
+      it 'doesnt create a new service' do
-        Services::Twitter.create!(:nickname => omniauth_auth["info"]['nickname'],
+        expect {
-                                  :access_token => omniauth_auth['credentials']['token'],
-                                  :access_secret => omniauth_auth['credentials']['secret'],
-                                  :uid => omniauth_auth['uid'],
-                                  :user => bob)
         post :create, :provider => 'twitter'
-        flash[:error].include?(bob.person.profile.diaspora_handle).should be_true
+      }.to_not change(Service, :count).by(1)
+      end
+
+      it 'flashes an read-only access error'  do
+        post :create, :provider => 'twitter'
+        flash[:error].include?( 'Access level is read-only' ).should be_true
       end
     end
 
@@ -72,9 +89,7 @@ describe ServicesController do
       end
 
       it 'does not queue a job if the profile photo is set' do
-        profile = @user.person.profile
+        @controller.stub!(:no_profile_image?).and_return false
-        profile[:image_url] = "/non/default/image.jpg"
-        profile.save
 
         Workers::FetchProfilePhoto.should_not_receive(:perform_async)
 
@@ -82,11 +97,9 @@ describe ServicesController do
       end
 
       it 'queues a job to save user photo if the photo does not exist' do
-        profile = @user.person.profile
+        @controller.stub!(:no_profile_image?).and_return true
-        profile[:image_url] = nil
-        profile.save
 
-        Workers::FetchProfilePhoto.should_receive(:perform_async).with(@user.id, anything(), "https://service.com/fallback_lowres.jpg")
+        Workers::FetchProfilePhoto.should_receive(:perform_async).with(user.id, anything(), "https://service.com/fallback_lowres.jpg")
 
         post :create, :provider => 'twitter'
       end
@@ -95,13 +108,13 @@ describe ServicesController do
 
   describe '#destroy' do
     before do
-      @service1 = FactoryGirl.create(:service, :user => @user)
+      @service1 = FactoryGirl.create(:service, :user => user)
     end
 
     it 'destroys a service selected by id' do
       lambda{
         delete :destroy, :id => @service1.id
-      }.should change(@user.services, :count).by(-1)
+      }.should change(user.services, :count).by(-1)
     end
   end
 end

spec/models/service_spec.rb

@@ -19,7 +19,43 @@ describe Service do
   end
 
   it 'by default has no profile photo url' do
-    Service.new.profile_photo_url.should be_nil
+    expect( described_class.new.profile_photo_url ).to be_nil
+  end
+
+  describe '.titles' do
+    it "converts passed service titles into service constants" do
+      expect( described_class.titles( ['twitter'] ) ).to eql ['Services::Twitter']
+    end
+  end
+
+  describe '.first_from_omniauth' do
+    let(:omniauth) { { 'provider' => 'facebook', 'uid' => '1', 'credentials' => {}, 'info' => {} } }
+    it 'first service by provider and uid' do
+      expect( described_class.first_from_omniauth( omniauth ) ).to eql @service
+    end
+  end
+
+  describe '.initialize_from_omniauth' do
+    let(:omniauth) do
+      { 'provider' => 'facebook',
+        'uid'      => '2',
+        'info'   => { 'nickname' => 'grimmin' },
+        'credentials' => { 'token' => 'tokin', 'secret' =>"not_so_much" },
+        'extra' => { 'access_token' => { 'response' => { 'header' => { 'x_access_level' => 'read' }}}}
+      }
+    end
+    let(:subject) { described_class.initialize_from_omniauth( omniauth ) }
+
+    it 'new service obj of type Services::Facebook' do
+      expect( subject.type ).to eql "Services::Facebook"
+    end
+
+    it 'new service obj with oauth options populated' do
+      expect( subject.uid ).to eql "2"
+      expect( subject.nickname ).to eql "grimmin"
+      expect( subject.access_token ).to eql "tokin"
+      expect( subject.access_secret ).to eql "not_so_much"
+      expect( subject.access_level ).to eql "read"
+    end
   end
-  
 end