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

Conversation

Projects
None yet
8 participants
@Gargron
Copy link
Member

Gargron commented Mar 16, 2019

Allow users to setup a cryptographic connection between their Mastodon profile and their Keybase profile. Keybase is extracted into a polymorphous ProofProvider interface to potentially allow other providers to be supported and minimize "hardcoding" a specific provider.

image

image

image

Performance notes:

  • One synchronous HTTP request is performed when proof is being saved to check if the signature hash is actually valid on Keybase
  • One asynchronous HTTP request is queued right after the proof is created to check if the proof went live on Keybase (and can therefore be showcased)
  • One synchronous HTTP request is performed to retrieve the avatar URL from Keybase for each proof, both on the proof index as well as on new proof page
  • One synchronous HTTP request is performed to check if the proof is still live and valid whenever the proof index is loaded

Resolve #10013


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 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.

@Gargron Gargron force-pushed the feature-keybase branch from 9477644 to baf695a Mar 17, 2019

@ThibG
Copy link
Collaborator

ThibG left a comment

This review doesn't really cover the correctness of the implementation wrt. keybase's protocol itself, as I have not read it, but only code quality and some concerns.

It seems fine overall, but I'm a bit concerned with the various HTTP calls, some of which synchronous. This shouldn't be a major issue, but still.

Also, will this work for any instance, or do instance admins still need to get approved by keybase?

Wouldn't it make sense to have a setting to enable or disable this feature? (Especially if the admin needs to take action wrt. keybase for it to be usable)

Show resolved Hide resolved config/settings.yml Outdated
@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


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

Show resolved Hide resolved app/views/accounts/_bio.html.haml Outdated

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

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.

@Gargron

This comment has been minimized.

Copy link
Member Author

Gargron commented Mar 18, 2019

Also, will this work for any instance, or do instance admins still need to get approved by keybase?

Wouldn't it make sense to have a setting to enable or disable this feature? (Especially if the admin needs to take action wrt. keybase for it to be usable)

I was assured no proactive action would be necessary on the part of the instance admin

@Gargron Gargron merged commit 9c4cbdb into master Mar 18, 2019

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details

@Gargron Gargron deleted the feature-keybase branch Mar 18, 2019

@witcheslive

This comment has been minimized.

Copy link

witcheslive commented Mar 19, 2019

I have updated witches.live to the master branch which has support for this, do you need any help testing it, and if so how could I go about doing that?

@xgess xgess referenced this pull request Mar 19, 2019

Closed

Add Keybase Proofs #1

@cnf

This comment has been minimized.

Copy link

cnf commented Mar 23, 2019

Is there any documentation on how to use this as a user? I can't seem to find it.

@Gargron

This comment has been minimized.

Copy link
Member Author

Gargron commented Mar 24, 2019

@cnf It is not live on Keybase's side yet. Other than that, there is nothing to do on Mastodon, you initiate the process from Keybase.

@cnf

This comment has been minimized.

Copy link

cnf commented Mar 24, 2019

Right, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.