diff --git a/Changelog.md b/Changelog.md
index 77376c5d0a1644b7bd49ecbcd80633342d96dc7a..e82e1dacde637077e039491d8e5c5b6b87f917ed 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -3,12 +3,14 @@
**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
* 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)
* Convert all ActivityStreams::Photo to StatusMessages and drop ActivityStreams::Photo [#4144](https://github.com/diaspora/diaspora/issues/4144)
## Bug fixes
+* Check twitter write access before adding/authorizing it for a user.
* 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)
diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb
index 6ded9b740e0476c88261347dca5bbd1d279a60a1..b6b3e191d66ce9ceecee859f44e7e58e7f39fc4c 100644
--- a/app/controllers/services_controller.rb
+++ b/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
- auth = request.env['omniauth.auth']
-
- toke = auth['credentials']['token']
- 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?
+ def create
+ service = Service.initialize_from_omniauth( omniauth_hash )
+
+ if current_user.services << service
+ current_user.update_profile_with_omniauth( service.info )
- 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
+ fetch_photo(service) if no_profile_image?
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
diff --git a/app/models/profile.rb b/app/models/profile.rb
index 5a8073d3b965f7d4a37b57b3337afbfb62bc6ac2..e9a9b8cc360548e383cc278fa4e318d0491c6663 100644
--- a/app/models/profile.rb
+++ b/app/models/profile.rb
@@ -85,7 +85,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
diff --git a/app/models/service.rb b/app/models/service.rb
index fd5a1e21532e6246c4ee87ce1f4b5835ca3d9eea..2620fecf648bc9e9fd8551bc2a74fc3feab3da1d 100644
--- a/app/models/service.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index a53bfe4888b960b6dfc3fa02324cece110108f8a..1d4491097fcdca7f3913d27843c45d8d4f331e16 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -350,6 +350,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
diff --git a/config/locales/diaspora/en.yml b/config/locales/diaspora/en.yml
index ed7e43c6e77ce9e7b0f58acb094a2aa7e54e1320..a24facc0eac06129c3791ffb1d7ed5ccc60d3915 100644
--- a/config/locales/diaspora/en.yml
+++ b/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:
diff --git a/spec/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb
index 3446b1730af863aaefccb8612e27ce8885711de2..70b73e6ec442adf239aed98fc02ae29d1b64b6b3 100644
--- a/spec/controllers/services_controller_spec.rb
+++ b/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) {
+ let(:omniauth_auth) do
{ '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
- @controller.stub!(:current_user).and_return(@user)
- mock_access_token.stub!(:token => "12345", :secret => "56789")
+ sign_in :user, user
+ @controller.stub!(:current_user).and_return(user)
end
describe '#index' do
- it 'displays all connected serivices for a user' do
- 4.times do
- FactoryGirl.create(:service, :user => @user)
- end
+ before do
+ FactoryGirl.create(:service, user: user)
+ 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
- request.env['omniauth.auth'] = omniauth_auth
- end
+ before do
+ request.env['omniauth.auth'] = omniauth_auth
+ request.env['omniauth.origin'] = root_url
+ end
+
+ 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
- it 'creates a new OmniauthService' do
+ 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'
- }.to change(@user.services, :count).by(1)
+ post :create, :provider => 'twitter'
+ }.to_not change(Service, :count).by(1)
+ end
+
+ it 'flashes an already_authorized error with the diaspora handle for the user' do
+ post :create, :provider => '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 'creates a twitter service' do
- Service.delete_all
- @user.getting_started = false
+ it 'doesnt create a new service' do
+ expect {
post :create, :provider => 'twitter'
- @user.reload.services.first.class.name.should == "Services::Twitter"
+ }.to_not change(Service, :count).by(1)
end
- it 'returns error if the user already a service with that uid' do
- Services::Twitter.create!(:nickname => omniauth_auth["info"]['nickname'],
- :access_token => omniauth_auth['credentials']['token'],
- :access_secret => omniauth_auth['credentials']['secret'],
- :uid => omniauth_auth['uid'],
- :user => bob)
+ it 'flashes an read-only access error' do
post :create, :provider => 'twitter'
- flash[:error].include?(bob.person.profile.diaspora_handle).should be_true
+ 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
- profile[:image_url] = "/non/default/image.jpg"
- profile.save
+ @controller.stub!(:no_profile_image?).and_return false
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
- profile[:image_url] = nil
- profile.save
+ @controller.stub!(:no_profile_image?).and_return true
- 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
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 49d2a72e014702e48acdffd422adbfa7dbe6f3f9..93607d9234718c975f39c6e61bb59a07fd77bc1f 100644
--- a/spec/models/service_spec.rb
+++ b/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