diff --git a/lib/travis/api/app/endpoint/authorization.rb b/lib/travis/api/app/endpoint/authorization.rb index bfc4f1e5..f5ccc09d 100644 --- a/lib/travis/api/app/endpoint/authorization.rb +++ b/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 diff --git a/lib/travis/api/serialize/v2/http/user.rb b/lib/travis/api/serialize/v2/http/user.rb index e7bbbf55..e414d961 100644 --- a/lib/travis/api/serialize/v2/http/user.rb +++ b/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 } diff --git a/spec/unit/endpoint/authorization/user_manager_spec.rb b/spec/unit/endpoint/authorization/user_manager_spec.rb index 0dd28728..0954c131 100644 --- a/spec/unit/endpoint/authorization/user_manager_spec.rb +++ b/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 diff --git a/spec/unit/endpoint/authorization_spec.rb b/spec/unit/endpoint/authorization_spec.rb index 5ca8b830..7d33ef64 100644 --- a/spec/unit/endpoint/authorization_spec.rb +++ b/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: diff --git a/spec/unit/endpoint/users_spec.rb b/spec/unit/endpoint/users_spec.rb index 140c0119..7b4d5770 100644 --- a/spec/unit/endpoint/users_spec.rb +++ b/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 diff --git a/spec/unit/serialize/v2/http/user_spec.rb b/spec/unit/serialize/v2/http/user_spec.rb index 45b58fca..b0ccc157 100644 --- a/spec/unit/serialize/v2/http/user_spec.rb +++ b/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, diff --git a/spec_core/github/oauth_spec.rb b/spec_core/github/oauth_spec.rb new file mode 100644 index 00000000..eb7fe169 --- /dev/null +++ b/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 diff --git a/spec_core/model/user_spec.rb b/spec_core/model/user_spec.rb index ebc8fb97..c3b653d5 100644 --- a/spec_core/model/user_spec.rb +++ b/spec_core/model/user_spec.rb @@ -165,34 +165,7 @@ describe User do end end - describe 'track_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 - + describe 'github_scopes' do 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 diff --git a/vendor/travis-core/lib/travis/github.rb b/vendor/travis-core/lib/travis/github.rb index 78ea5050..3afc1d72 100644 --- a/vendor/travis-core/lib/travis/github.rb +++ b/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 diff --git a/vendor/travis-core/lib/travis/github/oauth.rb b/vendor/travis-core/lib/travis/github/oauth.rb new file mode 100644 index 00000000..969152a4 --- /dev/null +++ b/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 diff --git a/vendor/travis-core/lib/travis/model/user.rb b/vendor/travis-core/lib/travis/model/user.rb index 5a44a7ef..89a2ec9c 100644 --- a/vendor/travis-core/lib/travis/model/user.rb +++ b/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 - read_attribute(:github_scopes) || [] - end - - def correct_scopes? - missing = Oauth.wanted_scopes - github_scopes - missing.empty? + github_oauth_token && read_attribute(:github_scopes) || [] 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 - super.gsub(github_oauth_token, '[REDACTED]') - else - super - end + github_oauth_token ? super.gsub(github_oauth_token, '[REDACTED]') : super end protected - # TODO this accesses GitHub during tests. should move initializing the scopes out of the model. - def track_github_scopes - 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?) + def track_previous_changes + @previous_changes = changes end def set_as_recent diff --git a/vendor/travis-core/lib/travis/model/user/oauth.rb b/vendor/travis-core/lib/travis/model/user/oauth.rb index 17ccf6d9..d8a9e5ab 100644 --- a/vendor/travis-core/lib/travis/model/user/oauth.rb +++ b/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']) diff --git a/vendor/travis-core/lib/travis/testing/stubs.rb b/vendor/travis-core/lib/travis/testing/stubs.rb index f4b08611..5a8fe51a 100644 --- a/vendor/travis-core/lib/travis/testing/stubs.rb +++ b/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(','), - correct_scopes?: true, + github_scopes: Travis.config.oauth2.scopes.to_s.split(','), created_at: Time.now.utc - 7200 ) end