Commit ed1d4fa4 authored by Stan Hu's avatar Stan Hu

Remove user OAuth tokens stored in database for Bitbucket, GitHub, and GitLab

and request them each session. Pass these tokens to the project import data.

This prevents the need to encrypt these tokens and clear them in case they
expire or get revoked.

For example, if you deleted and re-created OAuth2 keys for Bitbucket, you would get
an Error 500 with no way to recover:

```
Started GET "/import/bitbucket/status" for x.x.x.x at 2015-08-07 05:24:10 +0000
Processing by Import::BitbucketController#status as HTML
Completed 500 Internal Server Error in 607ms (ActiveRecord: 2.3ms)

NameError (uninitialized constant Import::BitbucketController::Unauthorized):
  app/controllers/import/bitbucket_controller.rb:77:in `rescue in go_to_bitbucket_for_permissions'
  app/controllers/import/bitbucket_controller.rb:74:in `go_to_bitbucket_for_permissions'
  app/controllers/import/bitbucket_controller.rb:86:in `bitbucket_unauthorized'
```

Closes #1871
parent 97cc91d2
Please view this file on the master branch, on stable branches it's out of date.
v 8.0.0 (unreleased)
- Remove user OAuth tokens from the database and request new tokens each session (Stan Hu)
- Only show recent push event if the branch still exists or a recent merge request has not been created (Stan Hu)
- Remove satellites
- Better performance for web editor (switched from satellites to rugged)
......
......@@ -13,10 +13,9 @@ class Import::BitbucketController < Import::BaseController
access_token = client.get_token(request_token, params[:oauth_verifier], callback_import_bitbucket_url)
current_user.bitbucket_access_token = access_token.token
current_user.bitbucket_access_token_secret = access_token.secret
session[:bitbucket_access_token] = access_token.token
session[:bitbucket_access_token_secret] = access_token.secret
current_user.save
redirect_to status_import_bitbucket_url
end
......@@ -46,19 +45,20 @@ class Import::BitbucketController < Import::BaseController
namespace = get_or_create_namespace || (render and return)
unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user).execute
unless Gitlab::BitbucketImport::KeyAdder.new(repo, current_user, access_params).execute
@access_denied = true
render
return
end
@project = Gitlab::BitbucketImport::ProjectCreator.new(repo, namespace, current_user).execute
@project = Gitlab::BitbucketImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute
end
private
def client
@client ||= Gitlab::BitbucketImport::Client.new(current_user.bitbucket_access_token, current_user.bitbucket_access_token_secret)
@client ||= Gitlab::BitbucketImport::Client.new(session[:bitbucket_access_token],
session[:bitbucket_access_token_secret])
end
def verify_bitbucket_import_enabled
......@@ -66,7 +66,7 @@ class Import::BitbucketController < Import::BaseController
end
def bitbucket_auth
if current_user.bitbucket_access_token.blank?
if session[:bitbucket_access_token].blank?
go_to_bitbucket_for_permissions
end
end
......@@ -81,4 +81,13 @@ class Import::BitbucketController < Import::BaseController
def bitbucket_unauthorized
go_to_bitbucket_for_permissions
end
private
def access_params
{
bitbucket_access_token: session[:bitbucket_access_token],
bitbucket_access_token_secret: session[:bitbucket_access_token_secret]
}
end
end
......@@ -5,9 +5,7 @@ class Import::GithubController < Import::BaseController
rescue_from Octokit::Unauthorized, with: :github_unauthorized
def callback
token = client.get_token(params[:code])
current_user.github_access_token = token
current_user.save
session[:github_access_token] = client.get_token(params[:code])
redirect_to status_import_github_url
end
......@@ -39,13 +37,13 @@ class Import::GithubController < Import::BaseController
namespace = get_or_create_namespace || (render and return)
@project = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, current_user).execute
@project = Gitlab::GithubImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute
end
private
def client
@client ||= Gitlab::GithubImport::Client.new(current_user.github_access_token)
@client ||= Gitlab::GithubImport::Client.new(session[:github_access_token])
end
def verify_github_import_enabled
......@@ -53,7 +51,7 @@ class Import::GithubController < Import::BaseController
end
def github_auth
if current_user.github_access_token.blank?
if session[:github_access_token].blank?
go_to_github_for_permissions
end
end
......@@ -65,4 +63,10 @@ class Import::GithubController < Import::BaseController
def github_unauthorized
go_to_github_for_permissions
end
private
def access_params
{ github_access_token: session[:github_access_token] }
end
end
......@@ -5,9 +5,7 @@ class Import::GitlabController < Import::BaseController
rescue_from OAuth2::Error, with: :gitlab_unauthorized
def callback
token = client.get_token(params[:code], callback_import_gitlab_url)
current_user.gitlab_access_token = token
current_user.save
session[:gitlab_access_token] = client.get_token(params[:code], callback_import_gitlab_url)
redirect_to status_import_gitlab_url
end
......@@ -36,13 +34,13 @@ class Import::GitlabController < Import::BaseController
namespace = get_or_create_namespace || (render and return)
@project = Gitlab::GitlabImport::ProjectCreator.new(repo, namespace, current_user).execute
@project = Gitlab::GitlabImport::ProjectCreator.new(repo, namespace, current_user, access_params).execute
end
private
def client
@client ||= Gitlab::GitlabImport::Client.new(current_user.gitlab_access_token)
@client ||= Gitlab::GitlabImport::Client.new(session[:gitlab_access_token])
end
def verify_gitlab_import_enabled
......@@ -50,7 +48,7 @@ class Import::GitlabController < Import::BaseController
end
def gitlab_auth
if current_user.gitlab_access_token.blank?
if session[:gitlab_access_token].blank?
go_to_gitlab_for_permissions
end
end
......@@ -62,4 +60,10 @@ class Import::GitlabController < Import::BaseController
def gitlab_unauthorized
go_to_gitlab_for_permissions
end
private
def access_params
{ gitlab_access_token: session[:gitlab_access_token] }
end
end
......@@ -25,9 +25,10 @@ class RepositoryImportWorker
end
return project.import_fail unless data_import_result
Gitlab::BitbucketImport::KeyDeleter.new(project).execute if project.import_type == 'bitbucket'
project.import_finish
project.save
ProjectCacheWorker.perform_async(project.id)
Gitlab::BitbucketImport::KeyDeleter.new(project).execute if project.import_type == 'bitbucket'
end
end
class RemoveOauthTokensFromUsers < ActiveRecord::Migration
def change
remove_column :users, :github_access_token, :string
remove_column :users, :gitlab_access_token, :string
remove_column :users, :bitbucket_access_token, :string
remove_column :users, :bitbucket_access_token_secret, :string
end
end
......@@ -526,13 +526,9 @@ ActiveRecord::Schema.define(version: 20150818213832) do
t.string "unconfirmed_email"
t.boolean "hide_no_ssh_key", default: false
t.string "website_url", default: "", null: false
t.string "github_access_token"
t.string "gitlab_access_token"
t.string "notification_email"
t.boolean "hide_no_password", default: false
t.boolean "password_automatically_set", default: false
t.string "bitbucket_access_token"
t.string "bitbucket_access_token_secret"
t.string "location"
t.string "encrypted_otp_secret"
t.string "encrypted_otp_secret_iv"
......
......@@ -5,7 +5,10 @@ module Gitlab
def initialize(project)
@project = project
@client = Client.new(project.creator.bitbucket_access_token, project.creator.bitbucket_access_token_secret)
import_data = project.import_data.try(:data)
bb_session = import_data["bb_session"] if import_data
@client = Client.new(bb_session["bitbucket_access_token"],
bb_session["bitbucket_access_token_secret"])
@formatter = Gitlab::ImportFormatter.new
end
......@@ -16,12 +19,12 @@ module Gitlab
#Issues && Comments
issues = client.issues(project_identifier)
issues["issues"].each do |issue|
body = @formatter.author_line(issue["reported_by"]["username"], issue["content"])
comments = client.issue_comments(project_identifier, issue["local_id"])
if comments.any?
body += @formatter.comments_header
end
......@@ -31,13 +34,13 @@ module Gitlab
end
project.issues.create!(
description: body,
description: body,
title: issue["title"],
state: %w(resolved invalid duplicate wontfix).include?(issue["status"]) ? 'closed' : 'opened',
author_id: gl_user_id(project, issue["reported_by"]["username"])
)
end
true
end
......
......@@ -3,14 +3,15 @@ module Gitlab
class KeyAdder
attr_reader :repo, :current_user, :client
def initialize(repo, current_user)
def initialize(repo, current_user, access_params)
@repo, @current_user = repo, current_user
@client = Client.new(current_user.bitbucket_access_token, current_user.bitbucket_access_token_secret)
@client = Client.new(access_params[:bitbucket_access_token],
access_params[:bitbucket_access_token_secret])
end
def execute
return false unless BitbucketImport.public_key.present?
project_identifier = "#{repo["owner"]}/#{repo["slug"]}"
client.add_deploy_key(project_identifier, BitbucketImport.public_key)
......
......@@ -6,12 +6,15 @@ module Gitlab
def initialize(project)
@project = project
@current_user = project.creator
@client = Client.new(current_user.bitbucket_access_token, current_user.bitbucket_access_token_secret)
import_data = project.import_data.try(:data)
bb_session = import_data["bb_session"] if import_data
@client = Client.new(bb_session["bitbucket_access_token"],
bb_session["bitbucket_access_token_secret"])
end
def execute
return false unless BitbucketImport.public_key.present?
client.delete_deploy_key(project.import_source, BitbucketImport.public_key)
true
......
module Gitlab
module BitbucketImport
class ProjectCreator
attr_reader :repo, :namespace, :current_user
attr_reader :repo, :namespace, :current_user, :session_data
def initialize(repo, namespace, current_user)
def initialize(repo, namespace, current_user, session_data)
@repo = repo
@namespace = namespace
@current_user = current_user
@session_data = session_data
end
def execute
::Projects::CreateService.new(current_user,
project = ::Projects::CreateService.new(current_user,
name: repo["name"],
path: repo["slug"],
description: repo["description"],
......@@ -18,8 +19,11 @@ module Gitlab
visibility_level: repo["is_private"] ? Gitlab::VisibilityLevel::PRIVATE : Gitlab::VisibilityLevel::PUBLIC,
import_type: "bitbucket",
import_source: "#{repo["owner"]}/#{repo["slug"]}",
import_url: "ssh://git@bitbucket.org/#{repo["owner"]}/#{repo["slug"]}.git"
import_url: "ssh://git@bitbucket.org/#{repo["owner"]}/#{repo["slug"]}.git",
).execute
project.create_import_data(data: { "bb_session" => session_data } )
project
end
end
end
......
......@@ -5,7 +5,9 @@ module Gitlab
def initialize(project)
@project = project
@client = Client.new(project.creator.github_access_token)
import_data = project.import_data.try(:data)
github_session = import_data["github_session"] if import_data
@client = Client.new(github_session["github_access_token"])
@formatter = Gitlab::ImportFormatter.new
end
......
module Gitlab
module GithubImport
class ProjectCreator
attr_reader :repo, :namespace, :current_user
attr_reader :repo, :namespace, :current_user, :session_data
def initialize(repo, namespace, current_user)
def initialize(repo, namespace, current_user, session_data)
@repo = repo
@namespace = namespace
@current_user = current_user
@session_data = session_data
end
def execute
::Projects::CreateService.new(current_user,
project = ::Projects::CreateService.new(
current_user,
name: repo.name,
path: repo.name,
description: repo.description,
......@@ -18,8 +20,11 @@ module Gitlab
visibility_level: repo.private ? Gitlab::VisibilityLevel::PRIVATE : Gitlab::VisibilityLevel::PUBLIC,
import_type: "github",
import_source: repo.full_name,
import_url: repo.clone_url.sub("https://", "https://#{current_user.github_access_token}@")
import_url: repo.clone_url.sub("https://", "https://#{@session_data[:github_access_token]}@")
).execute
project.create_import_data(data: { "github_session" => session_data } )
project
end
end
end
......
......@@ -5,7 +5,9 @@ module Gitlab
def initialize(project)
@project = project
@client = Client.new(project.creator.gitlab_access_token)
import_data = project.import_data.try(:data)
gitlab_session = import_data["gitlab_session"] if import_data
@client = Client.new(gitlab_session["gitlab_access_token"])
@formatter = Gitlab::ImportFormatter.new
end
......@@ -14,12 +16,12 @@ module Gitlab
#Issues && Comments
issues = client.issues(project_identifier)
issues.each do |issue|
body = @formatter.author_line(issue["author"]["name"], issue["description"])
comments = client.issue_comments(project_identifier, issue["id"])
if comments.any?
body += @formatter.comments_header
end
......@@ -29,13 +31,13 @@ module Gitlab
end
project.issues.create!(
description: body,
description: body,
title: issue["title"],
state: issue["state"],
author_id: gl_user_id(project, issue["author"]["id"])
)
end
true
end
......
module Gitlab
module GitlabImport
class ProjectCreator
attr_reader :repo, :namespace, :current_user
attr_reader :repo, :namespace, :current_user, :session_data
def initialize(repo, namespace, current_user)
def initialize(repo, namespace, current_user, session_data)
@repo = repo
@namespace = namespace
@current_user = current_user
@session_data = session_data
end
def execute
::Projects::CreateService.new(current_user,
project = ::Projects::CreateService.new(current_user,
name: repo["name"],
path: repo["path"],
description: repo["description"],
......@@ -18,8 +19,11 @@ module Gitlab
visibility_level: repo["visibility_level"],
import_type: "gitlab",
import_source: repo["path_with_namespace"],
import_url: repo["http_url_to_repo"].sub("://", "://oauth2:#{current_user.gitlab_access_token}@")
import_url: repo["http_url_to_repo"].sub("://", "://oauth2:#{@session_data[:gitlab_access_token]}@")
).execute
project.create_import_data(data: { "gitlab_session" => session_data } )
project
end
end
end
......
......@@ -4,7 +4,15 @@ require_relative 'import_spec_helper'
describe Import::BitbucketController do
include ImportSpecHelper
let(:user) { create(:user, bitbucket_access_token: 'asd123', bitbucket_access_token_secret: "sekret") }
let(:user) { create(:user) }
let(:token) { "asdasd12345" }
let(:secret) { "sekrettt" }
let(:access_params) { { bitbucket_access_token: token, bitbucket_access_token_secret: secret } }
def assign_session_tokens
session[:bitbucket_access_token] = token
session[:bitbucket_access_token_secret] = secret
end
before do
sign_in(user)
......@@ -17,8 +25,6 @@ describe Import::BitbucketController do
end
it "updates access token" do
token = "asdasd12345"
secret = "sekrettt"
access_token = double(token: token, secret: secret)
allow_any_instance_of(Gitlab::BitbucketImport::Client).
to receive(:get_token).and_return(access_token)
......@@ -26,8 +32,8 @@ describe Import::BitbucketController do
get :callback
expect(user.reload.bitbucket_access_token).to eq(token)
expect(user.reload.bitbucket_access_token_secret).to eq(secret)
expect(session[:bitbucket_access_token]).to eq(token)
expect(session[:bitbucket_access_token_secret]).to eq(secret)
expect(controller).to redirect_to(status_import_bitbucket_url)
end
end
......@@ -35,6 +41,7 @@ describe Import::BitbucketController do
describe "GET status" do
before do
@repo = OpenStruct.new(slug: 'vim', owner: 'asd')
assign_session_tokens
end
it "assigns variables" do
......@@ -73,17 +80,18 @@ describe Import::BitbucketController do
before do
allow(Gitlab::BitbucketImport::KeyAdder).
to receive(:new).with(bitbucket_repo, user).
to receive(:new).with(bitbucket_repo, user, access_params).
and_return(double(execute: true))
stub_client(user: bitbucket_user, project: bitbucket_repo)
assign_session_tokens
end
context "when the repository owner is the Bitbucket user" do
context "when the Bitbucket user and GitLab user's usernames match" do
it "takes the current user's namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, user.namespace, user).
to receive(:new).with(bitbucket_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -95,7 +103,7 @@ describe Import::BitbucketController do
it "takes the current user's namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, user.namespace, user).
to receive(:new).with(bitbucket_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -116,7 +124,7 @@ describe Import::BitbucketController do
context "when the namespace is owned by the GitLab user" do
it "takes the existing namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, existing_namespace, user).
to receive(:new).with(bitbucket_repo, existing_namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -150,7 +158,7 @@ describe Import::BitbucketController do
it "takes the new namespace" do
expect(Gitlab::BitbucketImport::ProjectCreator).
to receive(:new).with(bitbucket_repo, an_instance_of(Group), user).
to receive(:new).with(bitbucket_repo, an_instance_of(Group), user, access_params).
and_return(double(execute: true))
post :create, format: :js
......
......@@ -4,7 +4,13 @@ require_relative 'import_spec_helper'
describe Import::GithubController do
include ImportSpecHelper
let(:user) { create(:user, github_access_token: 'asd123') }
let(:user) { create(:user) }
let(:token) { "asdasd12345" }
let(:access_params) { { github_access_token: token } }
def assign_session_token
session[:github_access_token] = token
end
before do
sign_in(user)
......@@ -20,7 +26,7 @@ describe Import::GithubController do
get :callback
expect(user.reload.github_access_token).to eq(token)
expect(session[:github_access_token]).to eq(token)
expect(controller).to redirect_to(status_import_github_url)
end
end
......@@ -30,6 +36,7 @@ describe Import::GithubController do
@repo = OpenStruct.new(login: 'vim', full_name: 'asd/vim')
@org = OpenStruct.new(login: 'company')
@org_repo = OpenStruct.new(login: 'company', full_name: 'company/repo')
assign_session_token
end
it "assigns variables" do
......@@ -66,13 +73,14 @@ describe Import::GithubController do
before do
stub_client(user: github_user, repo: github_repo)
assign_session_token
end
context "when the repository owner is the GitHub user" do
context "when the GitHub user and GitLab user's usernames match" do
it "takes the current user's namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, user.namespace, user).
to receive(:new).with(github_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -84,7 +92,7 @@ describe Import::GithubController do
it "takes the current user's namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, user.namespace, user).
to receive(:new).with(github_repo, user.namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -97,6 +105,7 @@ describe Import::GithubController do
before do
github_repo.owner = OpenStruct.new(login: other_username)
assign_session_token
end
context "when a namespace with the GitHub user's username already exists" do
......@@ -105,7 +114,7 @@ describe Import::GithubController do
context "when the namespace is owned by the GitLab user" do
it "takes the existing namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, existing_namespace, user).
to receive(:new).with(github_repo, existing_namespace, user, access_params).
and_return(double(execute: true))
post :create, format: :js
......@@ -139,7 +148,7 @@ describe Import::GithubController do
it "takes the new namespace" do
expect(Gitlab::GithubImport::ProjectCreator).
to receive(:new).with(github_repo, an_instance_of(Group), user).
to receive(:new).with(github_repo, an_instance_of(Group), user, access_params).
and_return(double(execute: true))
post :create, format: :js
......
......@@ -4,7 +4,13 @@ require_relative 'import_spec_helper'
describe Import::GitlabController do
include ImportSpecHelper
let(:user) { create(:user, gitlab_access_token: 'asd123') }
let(:user) { create(:user) }
let(:token) { "asdasd12345" }
let(:access_params) { { gitlab_access_token: token } }
def assign_session_token
session[:gitlab_access_token] = token
end
before do
sign_in(user)
......@@ -13,14 +19,13 @@ describe Import::GitlabController do
describe "GET callback" do
it "updates access token" do
token = "asdasd12345"
allow_any_instance_of(Gitlab::GitlabImport::Client).
to receive(:get_token).and_return(token)
stub_omniauth_provider('gitlab')
get :callback
expect(user.reload.gitlab_access_token).to eq(token)
expect(session[:gitlab_access_token]).to eq(token)
expect(</