move tracking github oauth scopes out of the user model, and into Travis::Github::Oauth

lib/travis/api/app/endpoint/authorization.rb

@@ -4,6 +4,7 @@ require 'securerandom'
 require 'customerio'
 require 'travis/api/app'
 require 'travis/github/education'
+require 'travis/github/oauth'
 
 class Travis::Api::App
   class Endpoint
@@ -290,6 +291,8 @@ class Travis::Api::App
                 self.user = ::User.create! info
               end
 
+              Travis::Github::Oauth.track_scopes(user) # unless Travis.env == 'test'
+
               nullify_logins(user.github_id, user.login)
             end
 
@@ -341,7 +344,7 @@ class Travis::Api::App
         end
 
         def acceptable?(scopes, lossy = false)
-          User::Oauth.wanted_scopes.all? do |scope|
+          Travis::Github::Oauth.wanted_scopes.all? do |scope|
             acceptable_scopes_for(scope, lossy).any? { |s| scopes.include? s }
           end
         end

lib/travis/api/serialize/v2/http/user.rb

@@ -1,4 +1,5 @@
 require 'travis/api/serialize/formats'
+require 'travis/github/oauth'
 
 module Travis
   module Api
@@ -33,7 +34,7 @@ module Travis
                   'locale' => user.locale,
                   'is_syncing' => user.syncing?,
                   'synced_at' => format_date(user.synced_at),
-                  'correct_scopes' => user.correct_scopes?,
+                  'correct_scopes' => Github::Oauth.correct_scopes?(user),
                   'created_at' => format_date(user.created_at),
                   'channels' => channels
                 }

spec/unit/endpoint/authorization/user_manager_spec.rb

@@ -3,6 +3,7 @@ describe Travis::Api::App::Endpoint::Authorization::UserManager do
 
   before do
     Travis::Features.enable_for_all(:education_data_sync)
+    Travis::Github::Oauth.stubs(:track_scopes) # TODO test that scopes are being updated
   end
 
   describe '#info' do

spec/unit/endpoint/authorization_spec.rb

@@ -68,9 +68,6 @@ describe Travis::Api::App::Endpoint::Authorization do
 
       after do
         Travis.redis.srem('github:states', 'github-state')
-        # this is cached after first run, so if we change scopes, it will stay
-        # like that for the rest of the run
-        User::Oauth.instance_variable_set("@wanted_scopes", nil)
       end
 
       # in endpoint/authorization.rb 271, get_token faraday raises the exception:

spec/unit/endpoint/users_spec.rb

@@ -7,6 +7,7 @@ describe Travis::Api::App::Endpoint::Users, set_app: true do
     User.stubs(:find).returns(user)
     user.stubs(:repositories).returns(stub(administratable: stub(select: [repository])))
     user.stubs(:repository_ids).returns([1, 2, 3])
+    user.stubs(:github_scopes).returns(['public_repo', 'user:email'])
   end
 
   it 'needs to be authenticated' do

spec/unit/serialize/v2/http/user_spec.rb

@@ -4,6 +4,8 @@ describe Travis::Api::Serialize::V2::Http::User do
   let(:user) { stub_user(repository_ids: [1, 4, 8]) }
   let(:data) { described_class.new(user).data }
 
+  before { user.stubs(:github_scopes).returns(['public_repo', 'user:email']) }
+
   it 'user' do
     data['user'].should == {
       'id' => 1,

spec_core/github/oauth_spec.rb

@@ -0,0 +1,65 @@
+describe Travis::Github::Oauth do
+  let(:user) { Factory(:user, github_oauth_token: 'token', github_scopes: scopes) }
+
+  describe 'correct_scopes?' do
+    let(:scopes) { ['public_repo', 'user:email'] }
+    subject { described_class.correct_scopes?(user) }
+
+    it 'accepts correct scopes' do
+      should eq true
+    end
+
+    it 'complains about missing scopes' do
+      user.github_scopes.pop
+      should eq false
+    end
+
+    it 'accepts additional scopes' do
+      user.github_scopes << 'foo'
+      should eq true
+    end
+  end
+
+  describe 'track_scopes' do
+    before { user.reload }
+
+    describe 'the token did not change' do
+      let(:scopes) { ['public_repo', 'user:email'] }
+
+      it 'does not resolve github scopes' do
+        Travis::Github::Oauth.expects(:scopes_for).never
+        described_class.track_scopes(user)
+      end
+    end
+
+    describe 'the token has changed' do
+      let(:scopes) { ['public_repo', 'user:email'] }
+
+      before do
+        user.github_oauth_token = 'changed'
+        user.save!
+      end
+
+      it 'updates github scopes' do
+        Travis::Github::Oauth.expects(:scopes_for).returns(['foo', 'bar'])
+        described_class.track_scopes(user)
+        expect(user.reload.github_scopes).to eq ['foo', 'bar']
+      end
+    end
+
+    describe 'no scopes have been set so far' do
+      let(:scopes) { nil }
+
+      before do
+        user.github_oauth_token = 'changed'
+        user.save!
+      end
+
+      it 'updates github scopes' do
+        Travis::Github::Oauth.expects(:scopes_for).returns(['foo', 'bar'])
+        described_class.track_scopes(user)
+        expect(user.reload.github_scopes).to eq ['foo', 'bar']
+      end
+    end
+  end
+end

spec_core/model/user_spec.rb

@@ -165,34 +165,7 @@ describe User do
     end
   end
 
-  describe 'track_github_scopes' do
+  describe 'github_scopes' do
-    before { user.save! }
-
-    it "does not resolve github scopes if the token didn't change" do
-      user.github_scopes = ['public_repo', 'user:email']
-      user.save!
-      Travis::Github.expects(:scopes_for).never
-      user.save!
-    end
-
-    it "it resolves github scopes if the token did change" do
-      ENV['ENV'] = 'production' # TODO see user.rb ... move this out of the model.
-      Travis::Github.expects(:scopes_for).with(user).returns(['foo', 'bar'])
-      user.github_oauth_token = 'new_token'
-      user.save!
-      user.github_scopes.should be == ['foo', 'bar']
-      ENV['ENV'] = 'test'
-    end
-
-    it "it resolves github scopes if they haven't been resolved already" do
-      ENV['ENV'] = 'production' # TODO see user.rb ... move this out of the model.
-      Travis::Github.expects(:scopes_for).with(user).returns(['foo', 'bar'])
-      user.github_scopes = nil
-      user.save!
-      user.github_scopes.should be == ['foo', 'bar']
-      ENV['ENV'] = 'test'
-    end
-
     it 'returns an empty list if the token is missing' do
       user.github_scopes = ['foo']
       user.github_oauth_token = nil
@@ -200,27 +173,6 @@ describe User do
     end
   end
 
-  describe 'correct_scopes?' do
-    before do
-      user.github_scopes = ['public_repo', 'user:email']
-      user.save!
-    end
-
-    it "accepts correct scopes" do
-      user.should be_correct_scopes
-    end
-
-    it "complains about missing scopes" do
-      user.github_scopes.pop
-      user.should_not be_correct_scopes
-    end
-
-    it "accepts additional scopes" do
-      user.github_scopes << "foo"
-      user.should be_correct_scopes
-    end
-  end
-
   describe 'inspect' do
     context 'when user has GitHub OAuth token' do
       before :each do

vendor/travis-core/lib/travis/github.rb

@@ -1,6 +1,7 @@
 require 'gh'
 require 'core_ext/hash/compact'
 require 'travis/github/education'
+require 'travis/github/oauth'
 require 'travis/github/services'
 
 module Travis
@@ -21,16 +22,6 @@ module Travis
         fail "we don't have a github token for #{user.inspect}" if user.github_oauth_token.blank?
         GH.with(:token => user.github_oauth_token, &block)
       end
-
-      # TODO: Maybe this should move to gh?
-      def scopes_for(token)
-        token  = token.github_oauth_token if token.respond_to? :github_oauth_token
-        scopes = GH.with(token: token.to_s) { GH.head('user') }.headers['x-oauth-scopes'] if token.present?
-        scopes &&= scopes.gsub(/\s/,'').split(',')
-        Array(scopes).sort
-      rescue GH::Error
-        []
-      end
     end
   end
 end

vendor/travis-core/lib/travis/github/oauth.rb

@@ -0,0 +1,51 @@
+module Travis
+  module Github
+    module Oauth
+      class TrackScopes < Struct.new(:user)
+        def run
+          update if update?
+        end
+
+        private
+
+          def update
+            user.github_scopes = Oauth.scopes_for(user)
+            user.save!
+          end
+
+          def update?
+            github_oauth_token_changed?(user) or user.github_scopes.blank?
+          end
+
+          def github_oauth_token_changed?(user)
+            user.github_oauth_token_changed? or user.previous_changes.key?('github_oauth_token')
+          end
+      end
+
+      class << self
+        def track_scopes(user)
+          TrackScopes.new(user).run
+        end
+
+        def correct_scopes?(user)
+          missing = wanted_scopes - user.github_scopes
+          missing.empty?
+        end
+
+        def wanted_scopes
+          Travis.config.oauth2.scope.to_s.split(',').sort
+        end
+
+        # TODO: Maybe this should move to gh?
+        def scopes_for(token)
+          token  = token.github_oauth_token if token.respond_to? :github_oauth_token
+          scopes = GH.with(token: token.to_s) { GH.head('user') }.headers['x-oauth-scopes'] if token.present?
+          scopes &&= scopes.gsub(/\s/,'').split(',')
+          Array(scopes).sort
+        rescue GH::Error
+          []
+        end
+      end
+    end
+  end
+end

vendor/travis-core/lib/travis/model/user.rb

@@ -1,5 +1,6 @@
 require 'gh'
 require 'travis/model'
+require 'travis/github/oauth'
 
 class User < Travis::Model
   require 'travis/model/user/oauth'
@@ -16,9 +17,9 @@ class User < Travis::Model
   before_create :set_as_recent
   after_create :create_a_token
   after_commit :sync, on: :create
+  before_save :track_previous_changes
 
   serialize :github_scopes
-  before_save :track_github_scopes
 
   serialize :github_oauth_token, Travis::Model::EncryptedColumn.new
 
@@ -65,7 +66,7 @@ class User < Travis::Model
   end
 
   def sync
-    Travis.run_service(:sync_user, self) # TODO remove once apps use the service
+    Travis.run_service(:sync_user, self) # TODO move to the authentication endpoint, or create a separate service
   end
 
   def syncing?
@@ -102,7 +103,7 @@ class User < Travis::Model
   end
 
   def recently_signed_up?
-    @recently_signed_up || false
+    !!@recently_signed_up
   end
 
   def profile_image_hash
@@ -113,36 +114,30 @@ class User < Travis::Model
   end
 
   def github_scopes
-    return [] unless github_oauth_token
+    github_oauth_token && read_attribute(:github_scopes) || []
-    read_attribute(:github_scopes) || []
-  end
-
-  def correct_scopes?
-    missing = Oauth.wanted_scopes - github_scopes
-    missing.empty?
   end
 
   def avatar_url
     "https://0.gravatar.com/avatar/#{profile_image_hash}"
   end
 
+  def previous_changes
+    @previous_changes ||= {}
+  end
+
+  def reload
+    @previous_changes = nil
+    super
+  end
+
   def inspect
-    if github_oauth_token
+    github_oauth_token ? super.gsub(github_oauth_token, '[REDACTED]') : super
-      super.gsub(github_oauth_token, '[REDACTED]')
-    else
-      super
-    end
   end
 
   protected
 
-    # TODO this accesses GitHub during tests. should move initializing the scopes out of the model.
+    def track_previous_changes
-    def track_github_scopes
+      @previous_changes = changes
-      self.github_scopes = Travis::Github.scopes_for(self) if invalid_github_scopes?
-    end
-
-    def invalid_github_scopes?
-      Travis.env != 'test' and (github_oauth_token_changed? or github_scopes.blank?)
     end
 
     def set_as_recent

vendor/travis-core/lib/travis/model/user/oauth.rb

@@ -1,11 +1,6 @@
 class User
   module Oauth
     class << self
-      def wanted_scopes
-        return [] unless Travis.config.oauth2.scope
-        @wanted_scopes ||= Travis.config.oauth2.scope.split(',').sort
-      end
-
       def find_or_create_by(payload)
         attrs = attributes_from(payload)
         user = User.find_by_github_id(attrs['github_id'])

vendor/travis-core/lib/travis/testing/stubs.rb

@@ -290,8 +290,7 @@ module Travis
           is_syncing: false,
           synced_at: Time.now.utc - 3600,
           tokens: [stub('token', token: 'token')],
-          github_scopes: Travis.config.oauth2.to_h[:scopes].to_s.split(','),
+          github_scopes: Travis.config.oauth2.scopes.to_s.split(','),
-          correct_scopes?: true,
           created_at: Time.now.utc - 7200
         )
       end