From a58acfc909b76fdce97ac060c9c6fd8e0b1de130 Mon Sep 17 00:00:00 2001 From: Greg Lazarev Date: Wed, 21 Jun 2017 14:52:03 -0700 Subject: [PATCH] Find the token that can reach the repo Also, rescue `Octokit::Unauthorized` when checking if config is reachable, because if the token gets removed from GitHub's side then we'll encounter the unauthorized error (i.e. 401 - Bad credentials). In `GithubApi` make `client` method private, since we shouldn't be calling it directly -- defeats the purpose of our wrapper. --- app/models/user_token.rb | 15 ++++++- app/services/build_owner_hound_config.rb | 2 +- lib/github_api.rb | 14 ++++-- spec/models/user_token_spec.rb | 54 ++++++++++++++++++++---- spec/services/build_runner_spec.rb | 1 + 5 files changed, 71 insertions(+), 15 deletions(-) diff --git a/app/models/user_token.rb b/app/models/user_token.rb index 7b181c1a2..322685624 100644 --- a/app/models/user_token.rb +++ b/app/models/user_token.rb @@ -6,12 +6,23 @@ def token end def user - @user ||= users_with_token.sample || user_with_default_token + @_user ||= users_with_token.shuffle.detect(-> { hound_user }) do |user| + can_reach_repository?(user) + end end private - def user_with_default_token + def can_reach_repository?(user) + if GithubApi.new(user.token).repository?(repo.name) + true + else + repo.remove_membership(user) + false + end + end + + def hound_user User.new(token: Hound::GITHUB_TOKEN) end diff --git a/app/services/build_owner_hound_config.rb b/app/services/build_owner_hound_config.rb index 28216b534..fc95fdd8d 100644 --- a/app/services/build_owner_hound_config.rb +++ b/app/services/build_owner_hound_config.rb @@ -31,7 +31,7 @@ def user_token def config_repo_reachable? !!github.repo(config_repo.name) - rescue Octokit::InvalidRepository, Octokit::NotFound + rescue Octokit::InvalidRepository, Octokit::NotFound, Octokit::Unauthorized false end diff --git a/lib/github_api.rb b/lib/github_api.rb index 830a03472..b73d7125e 100644 --- a/lib/github_api.rb +++ b/lib/github_api.rb @@ -14,10 +14,6 @@ def initialize(token) @file_cache = {} end - def client - @client ||= Octokit::Client.new(access_token: token, auto_paginate: true) - end - def scopes client.scopes(token).join(",") end @@ -129,8 +125,18 @@ def remove_collaborator(repo_name, username) ) end + def repository?(repo_name) + client.repository?(repo_name) + rescue Octokit::Unauthorized + false + end + private + def client + @_client ||= Octokit::Client.new(access_token: token, auto_paginate: true) + end + def create_status(repo:, sha:, state:, description:, target_url: nil) client.create_status( repo, diff --git a/spec/models/user_token_spec.rb b/spec/models/user_token_spec.rb index 47af7c4da..1ff285666 100644 --- a/spec/models/user_token_spec.rb +++ b/spec/models/user_token_spec.rb @@ -1,15 +1,45 @@ +require "octokit/error" +require "lib/github_api" require "app/models/user_token" describe UserToken do describe "#token" do context "when user with a token is found" do - it "returns user's token" do - token = "foo_bar_token" - user = instance_double("User", token: token) - repo = instance_double("Repo", users_with_token: [user]) - user_token = UserToken.new(repo) + context "and the token can reach the repo" do + it "returns user's token" do + token = "foo_bar_token" + user = instance_double("User", token: token) + repo = instance_double( + "Repo", + users_with_token: [user], + name: "foo/bar", + ) + user_token = UserToken.new(repo) + stub_github(user.token) + + expect(user_token.token).to eq token + end + end - expect(user_token.token).to eq token + context "and the token cannot reach the repo" do + it "removes that user from repo and returns user with good token" do + user_with_bad_token = instance_double("User", token: "abc123") + user_with_good_token = instance_double("User", token: "def456") + repo = instance_double( + "Repo", + name: "thoughtbot/guides", + remove_membership: nil, + users_with_token: + double(shuffle: [user_with_bad_token, user_with_good_token]), + ) + user_token = UserToken.new(repo) + stub_github("abc123", repository?: false) + stub_github("def456", repository?: true) + + expect(user_token.token).to eq user_with_good_token.token + expect(repo).to have_received(:remove_membership). + with(user_with_bad_token) + end end end @@ -27,11 +57,19 @@ describe "#user" do it "returns the user that bears the current token" do - user = instance_double("User") - repo = instance_double("Repo", users_with_token: [user]) + user = instance_double("User", token: "abcd1") + repo = instance_double("Repo", users_with_token: [user], name: "foo/bar") user_token = UserToken.new(repo) + stub_github(user.token) expect(user_token.user).to eq user end end + + def stub_github(token, options = {}) + default_options = { repository?: true } + github_stub = instance_double("GithubApi", default_options.merge(options)) + allow(GithubApi).to receive(:new).with(token).and_return(github_stub) + github_stub + end end diff --git a/spec/services/build_runner_spec.rb b/spec/services/build_runner_spec.rb index b3aec3ded..fb6ec3ca1 100644 --- a/spec/services/build_runner_spec.rb +++ b/spec/services/build_runner_spec.rb @@ -334,6 +334,7 @@ def stubbed_github_api create_pending_status: nil, create_success_status: nil, create_error_status: nil, + repository?: true, ) allow(GithubApi).to receive(:new).and_return(github_api)