Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Keybase integration #10297

Merged
merged 42 commits into from Mar 18, 2019
Merged
Changes from 41 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
b664211
create account_identity_proofs table
xgess Dec 27, 2018
73b9867
add endpoint for keybase to check local proofs
xgess Dec 27, 2018
be92df5
add async task to update validity and liveness of proofs from keybase
xgess Dec 28, 2018
391a714
first pass keybase proof CRUD
xgess Jan 8, 2019
5554b60
second pass keybase proof creation
xgess Jan 9, 2019
fd670b5
clean up proof list and add badges
xgess Jan 10, 2019
09886f5
add avatar url to keybase api
xgess Jan 14, 2019
90aa86b
Always highlight the “Identity Proofs” navigation item when interacti…
adamjspooner Jan 16, 2019
bed4005
Update translations.
adamjspooner Jan 16, 2019
b7af7d9
Add profile URL.
adamjspooner Jan 16, 2019
82e7a46
Reorder proofs.
adamjspooner Jan 16, 2019
cc09a00
Add proofs to bio.
adamjspooner Jan 16, 2019
3c42d2d
Update settings/identity_proofs front-end.
adamjspooner Jan 16, 2019
9451e8f
Use `link_to`.
adamjspooner Jan 16, 2019
3c7a42f
Only encode query params if they exist.
adamjspooner Jan 16, 2019
4b341b1
Only show live proofs.
adamjspooner Jan 16, 2019
5781466
change valid to active in proof list and update liveness before displ…
xgess Jan 16, 2019
c8dc078
minor fixes
xgess Jan 16, 2019
729f6ac
add keybase config at well-known path
xgess Jan 24, 2019
29c390b
extremely naive feature flagging off the identity proof UI
xgess Jan 24, 2019
6ce8f39
fixes for rubocop
xgess Feb 11, 2019
9476b28
make identity proofs page resilient to potential keybase issues
xgess Feb 11, 2019
0bf7c56
normalize i18n
xgess Feb 11, 2019
2a41def
tweaks for brakeman
xgess Feb 11, 2019
cda6394
remove two unused translations
xgess Feb 11, 2019
dc2ed71
cleanup and add more localizations
xgess Feb 13, 2019
cf214d3
make keybase_contacts an admin setting
xgess Feb 15, 2019
a93123f
fix ExternalProofService my_domain
xgess Feb 18, 2019
0b3a105
use Addressable::URI in identity proofs
xgess Feb 18, 2019
dcf94d6
use active model serializer for keybase proof config
xgess Feb 18, 2019
6f6e797
more cleanup of keybase proof config
xgess Feb 18, 2019
4c37390
rename proof is_valid and is_live to proof_valid and proof_live
xgess Feb 18, 2019
a93c2bd
cleanup
xgess Feb 18, 2019
1119574
assorted tweaks for more robust communication with keybase
xgess Feb 28, 2019
5aff20c
Merge branch 'xgess/keybase-proofs' of git://github.com/xgess/mastodo…
Gargron Mar 16, 2019
728e511
Clean up
Gargron Mar 16, 2019
de387b2
Merge branch 'master' into feature-keybase
Gargron Mar 17, 2019
b343e6a
Small fixes
Gargron Mar 17, 2019
2326745
Display verified identity identically to verified links
Gargron Mar 17, 2019
bbd36e0
Clean up unused CSS
Gargron Mar 17, 2019
baf695a
Add caching for Keybase avatar URLs
Gargron Mar 17, 2019
ec56376
Remove keybase_contacts setting
Gargron Mar 18, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+958 −2
Diff settings

Always

Just for now

@@ -27,6 +27,7 @@ class SettingsController < BaseController
preview_sensitive_media
custom_css
profile_directory
keybase_contacts
).freeze

BOOLEAN_SETTINGS = %w(
@@ -0,0 +1,30 @@
# frozen_string_literal: true

class Api::ProofsController < Api::BaseController
before_action :set_account
before_action :set_provider
before_action :check_account_approval
before_action :check_account_suspension

def index
render json: @account, serializer: @provider.serializer_class
end

private

def set_provider
@provider = ProofProvider.find(params[:provider]) || raise(ActiveRecord::RecordNotFound)
end

def set_account
@account = Account.find_local!(params[:username])
end

def check_account_approval
not_found if @account.user_pending?
end

def check_account_suspension
gone if @account.suspended?
end
end
@@ -0,0 +1,45 @@
# frozen_string_literal: true

class Settings::IdentityProofsController < Settings::BaseController
layout 'admin'

before_action :authenticate_user!
before_action :check_required_params, only: :new

def index
@proofs = AccountIdentityProof.where(account: current_account).order(provider: :asc, provider_username: :asc)
@proofs.each(&:refresh!)

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

@xgess Is this necessary? n synchronous HTTP requests on page load is not great. I would think the worker queued after the proof is saved would take care of checking if the proof is live

This comment has been minimized.

Copy link
@ThibG

ThibG Mar 18, 2019

Collaborator

The refresh! do not seem to be synchronous (it spawns a worker?), but I question the need to trigger a refresh each time that page is visited.

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 18, 2019

Author Member

It does worker_class.new.perform(@proof) which is synchronous

This comment has been minimized.

Copy link
@xgess

xgess Mar 18, 2019

Contributor

the main reason i had to do this was that there's nothing currently built on the mastodon side to recognize remotely revoked proofs. i thought it might be weird if a user revokes a proof in keybase, then days later still sees it as live in mastodon until the first refresh. we had it as a note to talk about / build something that might let keybase inform mastodon a proof is revoked, or build a rake task for mastodon to check and invalidate revoked proofs.

i'm flexible on this though. if you want to change it to be async, i think that's reasonable.

end

def new
@proof = current_account.identity_proofs.new(
token: params[:token],
provider: params[:provider],
provider_username: params[:provider_username]

This comment has been minimized.

Copy link
@ThibG

ThibG Mar 18, 2019

Collaborator

resource_params?

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 18, 2019

Author Member

Ah no, the difference to resource_params is that these are like ?token=??? while resource_params after form submission are nested

)

render layout: 'auth'
end

def create
@proof = current_account.identity_proofs.where(provider: resource_params[:provider], provider_username: resource_params[:provider_username]).first_or_initialize(resource_params)
@proof.token = resource_params[:token]

if @proof.save
redirect_to @proof.on_success_path(params[:user_agent])
else
flash[:alert] = I18n.t('identity_proofs.errors.failed', provider: @proof.provider.capitalize)
redirect_to settings_identity_proofs_path
end
end

private

def check_required_params
redirect_to settings_identity_proofs_path unless [:provider, :provider_username, :token].all? { |k| params[k].present? }
end

def resource_params
params.require(:account_identity_proof).permit(:provider, :provider_username, :token)
end
end
@@ -0,0 +1,9 @@
# frozen_string_literal: true

module WellKnown
class KeybaseProofConfigController < ActionController::Base
def show
render json: {}, serializer: ProofProvider::Keybase::ConfigSerializer
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

This seems strange?

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

The serializer doesn't use an object, but various globals, see also: REST::InstanceSerializer

end
end
end
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 216.4144 232.00976"><path d="M107.86523 0C78.203984.2425 49.672422 3.4535937 33.044922 11.089844c0 0-32.97656262 14.752031-32.97656262 65.082031 0 11.525-.224375 25.306175.140625 39.919925 1.19750002 49.22 9.02375002 97.72843 54.53124962 109.77343 20.9825 5.55375 38.99711 6.71547 53.505856 5.91797 26.31125-1.45875 41.08203-9.38867 41.08203-9.38867l-.86914-19.08984s-18.80171 5.92758-39.91796 5.20508c-20.921254-.7175-43.006879-2.25516-46.390629-27.94141-.3125-2.25625-.46875-4.66938-.46875-7.20313 0 0 20.536953 5.0204 46.564449 6.21289 15.915.73001 30.8393-.93343 45.99805-2.74218 29.07-3.47125 54.38125-21.3818 57.5625-37.74805 5.0125-25.78125 4.59961-62.916015 4.59961-62.916015 0-50.33-32.97461-65.082031-32.97461-65.082031C166.80539 3.4535938 138.255.2425 108.59375 0h-.72852zM74.296875 39.326172c12.355 0 21.710234 4.749297 27.896485 14.248047l6.01367 10.080078 6.01563-10.080078c6.185-9.49875 15.54023-14.248047 27.89648-14.248047 10.6775 0 19.28156 3.753672 25.85156 11.076172 6.36875 7.3225 9.53907 17.218828 9.53907 29.673828v60.941408h-24.14454V81.869141c0-12.46875-5.24453-18.798829-15.73828-18.798829-11.6025 0-17.41797 7.508516-17.41797 22.353516v32.375002H96.207031V85.423828c0-14.845-5.815468-22.353515-17.417969-22.353516-10.49375 0-15.740234 6.330079-15.740234 18.798829v59.148439H38.904297V80.076172c0-12.455 3.171016-22.351328 9.541015-29.673828 6.568751-7.3225 15.172813-11.076172 25.851563-11.076172z" fill="#000"/></svg>
Binary file not shown.
@@ -801,3 +801,58 @@ code {
}
}
}

.connection-prompt {
margin-bottom: 25px;

.fa-link {
background-color: darken($ui-base-color, 4%);
border-radius: 100%;
font-size: 24px;
padding: 10px;
}

&__column {
align-items: center;
display: flex;
flex: 1;
flex-direction: column;
flex-shrink: 1;

&-sep {
flex-grow: 0;
overflow: visible;
position: relative;
z-index: 1;
}
}

.account__avatar {
margin-bottom: 20px;
}

&__connection {
background-color: lighten($ui-base-color, 8%);
box-shadow: 0 0 15px rgba($base-shadow-color, 0.2);
border-radius: 4px;
padding: 25px 10px;
position: relative;
text-align: center;

&::after {
background-color: darken($ui-base-color, 4%);
content: '';
display: block;
height: 100%;
left: 50%;
position: absolute;
width: 1px;
}
}

&__row {
align-items: center;
display: flex;
flex-direction: row;
}
}
@@ -0,0 +1,12 @@
# frozen_string_literal: true
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

Why app/lib and not app/providers? The latter allows you to avoid namespacing, sets you up to mime others by creating ann ApplicationProvider superclass, fits in with the rails loader api, and matches other practices in rails.

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

The ActivityPub and OStatus classes are likewise in app/lib, so I followed a precedent. Also, the namespacing is intentional to compartmentalize the proprietary vendor in such a way as to make removing it or swapping it out easier (everything is loaded more or less based on the provider value in AccountIdentityProof model)

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 18, 2019

Member

Yeah, seems like these should be a ProofProvider type of class, IMO, but I can definitely understand the current implementation.


module ProofProvider
SUPPORTED_PROVIDERS = %w(keybase).freeze

def self.find(identifier, proof = nil)
case identifier
when 'keybase'
ProofProvider::Keybase.new(proof)
end
end
end
@@ -0,0 +1,59 @@
# frozen_string_literal: true

class ProofProvider::Keybase
BASE_URL = 'https://keybase.io'

class Error < StandardError; end

class ExpectedProofLiveError < Error; end

class UnexpectedResponseError < Error; end
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

One practice I've taken up is having a global exception directory (and error): app/errors and app/exceptions, so that errors can be shared and to deliniate between business problems and developer problems (example: ArgumentError vs NotEnoughFundsException

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

We do have an app/lib/errors.rb file with that sort of thing. My intention here is to contain Keybase-specific errors in the Keybase-specific area of the code--the Rails controllers/models should not receive these exceptions.


def initialize(proof = nil)
@proof = proof
end

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

Looks like this could benefit from being an ActiveModel::Model?


def serializer_class
ProofProvider::Keybase::Serializer
end

def worker_class
ProofProvider::Keybase::Worker
end
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

I feel like this should be derived rather than explicit? AKA ProofProvider.const_get(self.class.name)::Worker

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

I was considering using metaprogramming, and this might be a fair critique, but since it's just two methods that do this, I opted to just write them out. I am generally more comfortable with less metaprogramming involved because it simplifies finding errors and searching code for usage.

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 18, 2019

Member

Understandable.


def validate!
unless @proof.token&.size == 66
@proof.errors.add(:base, I18n.t('identity_proofs.errors.keybase.invalid_token'))
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

It feels really strange to reach into @proof to modify it's errorset. Why not keep errors here?

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

AccountIdentityProof calls this method from validate. It is essentially an ActiveRecord validator. I was thinking whether it should be its own class or not, but it's not too many lines, so I just put it here.

One problem is that I have to use validate with a method/lambda that calls provider_instance, instead of validates_with SomeValidatorClass because that class would depend on the dynamic provider value. Of course I could do something like validates_with ProofProvider::Validator, which then in its own validate_each method would resolve the right class and call it, but why do all those extra steps when this method works?

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 18, 2019

Member

I guess. This violates the Law of Demeter, though, and IMO will create annoyances down the line.

return
end

return if @proof.provider_username.blank?
This conversation was marked as resolved by krainboltgreene

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 17, 2019

Member

This means we'll return nil, instead of a Boolean.

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

This method does not have a return value, it just fills @proof.errors during @proof's validation phase.

This comment has been minimized.

Copy link
@krainboltgreene

if verifier.valid?
@proof.verified = true
@proof.live = false

This comment has been minimized.

Copy link
@ThibG

ThibG Mar 18, 2019

Collaborator

Should ActiveRecord validation really have side-effects?

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 18, 2019

Author Member

I suppose you could argue that verified could be set before_save which would not happen if validation doesn't pass, but I am not too unhappy with how it is

else
@proof.errors.add(:base, I18n.t('identity_proofs.errors.keybase.verification_failed', kb_username: @proof.provider_username))
end
end

def refresh!
worker_class.new.perform(@proof)
rescue ProofProvider::Keybase::Error
nil
end

def on_success_path(user_agent = nil)
verifier.on_success_path(user_agent)
end

def badge
@badge ||= ProofProvider::Keybase::Badge.new(@proof.account.username, @proof.provider_username, @proof.token)
end

private

def verifier
@verifier ||= ProofProvider::Keybase::Verifier.new(@proof.account.username, @proof.provider_username, @proof.token)
end
end
@@ -0,0 +1,48 @@
# frozen_string_literal: true

class ProofProvider::Keybase::Badge
include RoutingHelper

def initialize(local_username, provider_username, token)
@local_username = local_username
@provider_username = provider_username
@token = token
end

def proof_url
"#{ProofProvider::Keybase::BASE_URL}/#{@provider_username}/sigchain\##{@token}"
end

def profile_url
"#{ProofProvider::Keybase::BASE_URL}/#{@provider_username}"
end

def icon_url
"#{ProofProvider::Keybase::BASE_URL}/#{@provider_username}/proof_badge/#{@token}?username=#{@local_username}&domain=#{domain}"
end

def avatar_url
Rails.cache.fetch("proof_providers/keybase/#{@provider_username}/avatar_url", expires_in: 5.minutes) { remote_avatar_url } || default_avatar_url
end

private

def remote_avatar_url
request = Request.new(:get, "#{ProofProvider::Keybase::BASE_URL}/_/api/1.0/user/pic_url.json", params: { username: @provider_username })

request.perform do |res|
json = Oj.load(res.body_with_limit, mode: :strict)
json['pic_url'] if json.is_a?(Hash)
end
rescue Oj::ParseError, HTTP::Error, OpenSSL::SSL::SSLError
nil
end

def default_avatar_url
asset_pack_path('media/images/proof_providers/keybase.png')
end

def domain
Rails.configuration.x.local_domain
end
end
@@ -0,0 +1,70 @@
# frozen_string_literal: true

class ProofProvider::Keybase::ConfigSerializer < ActiveModel::Serializer
include RoutingHelper

attributes :version, :domain, :display_name, :username,
:brand_color, :logo, :description, :prefill_url,
:profile_url, :check_url, :check_path, :avatar_path,
:contact

def version
1
end

def domain
Rails.configuration.x.local_domain
end

def display_name
Setting.site_title
end

def logo
{ svg_black: full_asset_url(asset_pack_path('media/images/logo_transparent_black.svg')), svg_full: full_asset_url(asset_pack_path('media/images/logo.svg')) }
end

def brand_color
'#282c37'
end

def description
Setting.site_short_description.presence || Setting.site_description.presence || I18n.t('about.about_mastodon_html')
end

def username
{ min: 1, max: 30, re: Account::USERNAME_RE.inspect }
end

def prefill_url
params = {
provider: 'keybase',
token: '%{sig_hash}',
provider_username: '%{kb_username}',
username: '%{username}',
user_agent: '%{kb_ua}',
}
This conversation was marked as resolved by Gargron

This comment has been minimized.

Copy link
@BenLubar

BenLubar Mar 17, 2019

Contributor

The validator wants there to be a username parameter (presumably, Mastodon should check to make sure this matches the logged-in user's name).

{
  "status": {
    "code": 100,
    "desc": "missing or invalid inputs {\"prefill_url\":\"Error: Input https://mastodon.example.com/settings/identity_proofs/new?provider=keybase&provider_username=%{kb_username}&token=%{sig_hash}&user_agent=%{kb_ua} missing %{username}\"}",
    "name": "INPUT_ERROR"
  }
}
Suggested change
}
username: '%{username}',
}

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

@xgess This wasn't in the original PR--has the validation on your side changed? We don't use the username param on that page on our side...

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 18, 2019

Author Member

I added the param anyway for the time being


CGI.unescape(new_settings_identity_proof_url(params))
end

def profile_url
CGI.unescape(short_account_url('%{username}')) # rubocop:disable Style/FormatStringToken
end

def check_url
CGI.unescape(api_proofs_url(username: '%{username}', provider: 'keybase'))
end

def check_path
['signatures']
end

def avatar_path
['avatar']
end

def contact
Setting.keybase_contacts.split(/\s+,\s+/)
This conversation was marked as resolved by Gargron

This comment has been minimized.

Copy link
@Gargron

Gargron Mar 17, 2019

Author Member

@xgess Are you sure you don't want Setting.site_contact_email here, which will likely be set in more cases than specifically Setting.keybase_contacts?

This comment has been minimized.

Copy link
@xgess

xgess Mar 18, 2019

Contributor

i trust your judgment on this

This comment has been minimized.

Copy link
@krainboltgreene

krainboltgreene Mar 18, 2019

Member

Also, is it more likely that you want /\s*,\s*/ to allow for no or any spaces?

end
end
@@ -0,0 +1,25 @@
# frozen_string_literal: true

class ProofProvider::Keybase::Serializer < ActiveModel::Serializer
include RoutingHelper

attribute :avatar

has_many :identity_proofs, key: :signatures

def avatar
full_asset_url(object.avatar_original_url)
end

class AccountIdentityProofSerializer < ActiveModel::Serializer
attributes :sig_hash, :kb_username

def sig_hash
object.token
end

def kb_username
object.provider_username
end
end
end
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.