From 871c0d251a6d27c4591785ae446738a8d6c553ab Mon Sep 17 00:00:00 2001 From: Colin Mitchell Date: Tue, 22 Aug 2017 12:33:57 -0400 Subject: [PATCH] Application prefs section (#2758) * Add code for creating/managing apps to settings section * Add specs for app changes * Fix controller spec * Fix view file I pasted over by mistake * Add locale strings. Add 'my apps' to nav * Add Client ID/Secret to App page. Add some visual separation * Fix rubocop warnings * Fix embarrassing typo I lost an `end` statement while fixing a merge conflict. * Add code for creating/managing apps to settings section - Add specs for app changes - Add locale strings. Add 'my apps' to nav - Add Client ID/Secret to App page. Add some visual separation - Fix some bugs/warnings * Update to match code standards * Trigger notification * Add warning about not sharing API secrets * Tweak spec a bit * Cleanup fixture creation by using let! * Remove unused key * Add foreign key for application<->user --- .../settings/applications_controller.rb | 65 +++++++ app/models/user.rb | 13 ++ .../settings/applications/_fields.html.haml | 4 + .../settings/applications/index.html.haml | 20 +++ app/views/settings/applications/new.html.haml | 9 + .../settings/applications/show.html.haml | 28 +++ config/initializers/doorkeeper.rb | 2 +- config/locales/doorkeeper.en.yml | 7 +- config/locales/en.yml | 11 ++ config/navigation.rb | 1 + config/routes.rb | 5 + ...70427011934_re_add_owner_to_application.rb | 8 + db/schema.rb | 7 +- .../settings/applications_controller_spec.rb | 166 ++++++++++++++++++ spec/models/user_spec.rb | 20 +++ 15 files changed, 362 insertions(+), 4 deletions(-) create mode 100644 app/controllers/settings/applications_controller.rb create mode 100644 app/views/settings/applications/_fields.html.haml create mode 100644 app/views/settings/applications/index.html.haml create mode 100644 app/views/settings/applications/new.html.haml create mode 100644 app/views/settings/applications/show.html.haml create mode 100644 db/migrate/20170427011934_re_add_owner_to_application.rb create mode 100644 spec/controllers/settings/applications_controller_spec.rb diff --git a/app/controllers/settings/applications_controller.rb b/app/controllers/settings/applications_controller.rb new file mode 100644 index 0000000000..b8f114455d --- /dev/null +++ b/app/controllers/settings/applications_controller.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class Settings::ApplicationsController < ApplicationController + layout 'admin' + + before_action :authenticate_user! + + def index + @applications = current_user.applications.page(params[:page]) + end + + def new + @application = Doorkeeper::Application.new( + redirect_uri: Doorkeeper.configuration.native_redirect_uri, + scopes: 'read write follow' + ) + end + + def show + @application = current_user.applications.find(params[:id]) + end + + def create + @application = current_user.applications.build(application_params) + if @application.save + redirect_to settings_applications_path, notice: I18n.t('application.created') + else + render :new + end + end + + def update + @application = current_user.applications.find(params[:id]) + if @application.update_attributes(application_params) + redirect_to settings_applications_path, notice: I18n.t('generic.changes_saved_msg') + else + render :show + end + end + + def destroy + @application = current_user.applications.find(params[:id]) + @application.destroy + redirect_to settings_applications_path, notice: t('application.destroyed') + end + + def regenerate + @application = current_user.applications.find(params[:application_id]) + @access_token = current_user.token_for_app(@application) + @access_token.destroy + + redirect_to settings_application_path(@application), notice: t('access_token.regenerated') + end + + private + + def application_params + params.require(:doorkeeper_application).permit( + :name, + :redirect_uri, + :scopes, + :website + ) + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 96a2d09b77..02b1b26ee2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -46,6 +46,8 @@ class User < ApplicationRecord belongs_to :account, inverse_of: :user, required: true accepts_nested_attributes_for :account + has_many :applications, class_name: 'Doorkeeper::Application', as: :owner + validates :locale, inclusion: I18n.available_locales.map(&:to_s), if: :locale? validates_with BlacklistedEmailValidator, if: :email_changed? @@ -108,6 +110,17 @@ class User < ApplicationRecord settings.noindex end + def token_for_app(a) + return nil if a.nil? || a.owner != self + Doorkeeper::AccessToken + .find_or_create_by(application_id: a.id, resource_owner_id: id) do |t| + + t.scopes = a.scopes + t.expires_in = Doorkeeper.configuration.access_token_expires_in + t.use_refresh_token = Doorkeeper.configuration.refresh_token_enabled? + end + end + def activate_session(request) session_activations.activate(session_id: SecureRandom.hex, user_agent: request.user_agent, diff --git a/app/views/settings/applications/_fields.html.haml b/app/views/settings/applications/_fields.html.haml new file mode 100644 index 0000000000..9dbe23466a --- /dev/null +++ b/app/views/settings/applications/_fields.html.haml @@ -0,0 +1,4 @@ += f.input :name, hint: t('activerecord.attributes.doorkeeper/application.name') += f.input :website, hint: t('activerecord.attributes.doorkeeper/application.website') += f.input :redirect_uri, hint: t('activerecord.attributes.doorkeeper/application.redirect_uri') += f.input :scopes, hint: t('activerecord.attributes.doorkeeper/application.scopes') diff --git a/app/views/settings/applications/index.html.haml b/app/views/settings/applications/index.html.haml new file mode 100644 index 0000000000..17035f96cf --- /dev/null +++ b/app/views/settings/applications/index.html.haml @@ -0,0 +1,20 @@ +- content_for :page_title do + = t('doorkeeper.applications.index.title') + +%table.table + %thead + %tr + %th= t('doorkeeper.applications.index.application') + %th= t('doorkeeper.applications.index.scopes') + %th= t('doorkeeper.applications.index.created_at') + %th + %tbody + - @applications.each do |application| + %tr + %td= link_to application.name, settings_application_path(application) + %th= application.scopes.map { |scope| t(scope, scope: [:doorkeeper, :scopes]) }.join('
').html_safe + %td= l application.created_at + %td= table_link_to 'show', t('doorkeeper.applications.index.show'), settings_application_path(application) + %td= table_link_to 'times', t('doorkeeper.applications.index.delete'), settings_application_path(application), method: :delete, data: { confirm: t('doorkeeper.applications.confirmations.destroy') } += paginate @applications += link_to t('add_new'), new_settings_application_path, class: 'button' diff --git a/app/views/settings/applications/new.html.haml b/app/views/settings/applications/new.html.haml new file mode 100644 index 0000000000..61406a31f5 --- /dev/null +++ b/app/views/settings/applications/new.html.haml @@ -0,0 +1,9 @@ +- content_for :page_title do + = t('doorkeeper.applications.new.title') + +.form-container + = simple_form_for @application, url: settings_applications_path do |f| + = render 'fields', f:f + + .actions + = f.button :button, t('.create'), type: :submit diff --git a/app/views/settings/applications/show.html.haml b/app/views/settings/applications/show.html.haml new file mode 100644 index 0000000000..9f1a119861 --- /dev/null +++ b/app/views/settings/applications/show.html.haml @@ -0,0 +1,28 @@ +- content_for :page_title do + = t('doorkeeper.applications.show.title', name: @application.name) + + +%p.hint= t('application.warning') + +%div + %h3= t('application.uid') + %code= @application.uid + +%div + %h3= t('application.secret') + %code= @application.secret + +%div + %h3= t('access_token.your_token') + %code= current_user.token_for_app(@application).token + += link_to t('access_token.regenerate'), settings_application_regenerate_path(@application), method: :put, class: 'button' + +%hr + += simple_form_for @application, url: settings_application_path(@application), method: :put do |f| + = render 'fields', f:f + + .actions + = f.button :button, t('generic.save_changes'), type: :submit + diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 056a3651a6..689e2ac4a5 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -50,7 +50,7 @@ Doorkeeper.configure do # Optional parameter :confirmation => true (default false) if you want to enforce ownership of # a registered application # Note: you must also run the rails g doorkeeper:application_owner generator to provide the necessary support - # enable_application_owner :confirmation => true + enable_application_owner # Define access token scopes for your provider # For more information go to diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index 6412b8b48b..fa0a7babf1 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -3,8 +3,10 @@ en: activerecord: attributes: doorkeeper/application: - name: Name + name: Application Name + website: Application Website redirect_uri: Redirect URI + scopes: Scopes errors: models: doorkeeper/application: @@ -37,9 +39,12 @@ en: name: Name new: New Application title: Your applications + show: Show + delete: Delete new: title: New Application show: + title: 'Application: %{name}' actions: Actions application_id: Application Id callback_urls: Callback urls diff --git a/config/locales/en.yml b/config/locales/en.yml index 97f46c3af0..fbcef03bde 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -33,6 +33,10 @@ en: user_count_after: users user_count_before: Home to what_is_mastodon: What is Mastodon? + access_token: + your_token: Your Access Token + regenerate: Regenerate Access Token + regenerated: Access Token Regenerated accounts: follow: Follow followers: Followers @@ -226,6 +230,12 @@ en: settings: 'Change e-mail preferences: %{link}' signature: Mastodon notifications from %{instance} view: 'View:' + application: + created: Application Created + destroyed: Application Destroyed + uid: Client ID + secret: Client Secret + warning: Be very careful with this data. Never share it with anyone other than authorized applications! applications: invalid_url: The provided URL is invalid auth: @@ -423,6 +433,7 @@ en: preferences: Preferences settings: Settings two_factor_authentication: Two-factor Authentication + your_apps: Your applications statuses: open_in_web: Open in web over_character_limit: character limit of %{max} exceeded diff --git a/config/navigation.rb b/config/navigation.rb index 535d033f50..6e04843eca 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -12,6 +12,7 @@ SimpleNavigation::Configuration.run do |navigation| settings.item :import, safe_join([fa_icon('cloud-upload fw'), t('settings.import')]), settings_import_url settings.item :export, safe_join([fa_icon('cloud-download fw'), t('settings.export')]), settings_export_url settings.item :authorized_apps, safe_join([fa_icon('list fw'), t('settings.authorized_apps')]), oauth_authorized_applications_url + settings.item :your_apps, safe_join([fa_icon('list fw'), t('settings.your_apps')]), settings_applications_url settings.item :follower_domains, safe_join([fa_icon('users fw'), t('settings.followers')]), settings_follower_domains_url end diff --git a/config/routes.rb b/config/routes.rb index 1a39dfeac7..e8bc968f4e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -79,6 +79,11 @@ Rails.application.routes.draw do end resource :follower_domains, only: [:show, :update] + + resources :applications do + put :regenerate + end + resource :delete, only: [:show, :destroy] resources :sessions, only: [:destroy] diff --git a/db/migrate/20170427011934_re_add_owner_to_application.rb b/db/migrate/20170427011934_re_add_owner_to_application.rb new file mode 100644 index 0000000000..a41d71d2a1 --- /dev/null +++ b/db/migrate/20170427011934_re_add_owner_to_application.rb @@ -0,0 +1,8 @@ +class ReAddOwnerToApplication < ActiveRecord::Migration[5.0] + def change + add_column :oauth_applications, :owner_id, :integer, null: true + add_column :oauth_applications, :owner_type, :string, null: true + add_index :oauth_applications, [:owner_id, :owner_type] + add_foreign_key :oauth_applications, :users, column: :owner_id, on_delete: :cascade + end +end diff --git a/db/schema.rb b/db/schema.rb index 2501e451d0..929a5fd01b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -216,8 +216,11 @@ ActiveRecord::Schema.define(version: 20170720000000) do t.string "scopes", default: "", null: false t.datetime "created_at" t.datetime "updated_at" - t.boolean "superapp", default: false, null: false - t.string "website" + t.boolean "superapp", default: false, null: false + t.string "website" + t.integer "owner_id" + t.string "owner_type" + t.index ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type", using: :btree t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true end diff --git a/spec/controllers/settings/applications_controller_spec.rb b/spec/controllers/settings/applications_controller_spec.rb new file mode 100644 index 0000000000..fa27e6ec6b --- /dev/null +++ b/spec/controllers/settings/applications_controller_spec.rb @@ -0,0 +1,166 @@ +require 'rails_helper' + +describe Settings::ApplicationsController do + render_views + + let!(:user) { Fabricate(:user) } + let!(:app) { Fabricate(:application, owner: user) } + + before do + sign_in user, scope: :user + end + + describe 'GET #index' do + let!(:other_app) { Fabricate(:application) } + + it 'shows apps' do + get :index + expect(response).to have_http_status(:success) + expect(assigns(:applications)).to include(app) + expect(assigns(:applications)).to_not include(other_app) + end + end + + + describe 'GET #show' do + it 'returns http success' do + get :show, params: { id: app.id } + expect(response).to have_http_status(:success) + expect(assigns[:application]).to eql(app) + end + + it 'returns 404 if you dont own app' do + app.update!(owner: nil) + + get :show, params: { id: app.id } + expect(response.status).to eq 404 + end + end + + describe 'GET #new' do + it 'works' do + get :new + expect(response).to have_http_status(:success) + end + end + + describe 'POST #create' do + context 'success' do + def call_create + post :create, params: { + doorkeeper_application: { + name: 'My New App', + redirect_uri: 'urn:ietf:wg:oauth:2.0:oob', + website: 'http://google.com', + scopes: 'read write follow' + } + } + response + end + + it 'creates an entry in the database' do + expect { call_create }.to change(Doorkeeper::Application, :count) + end + + it 'redirects back to applications page' do + expect(call_create).to redirect_to(settings_applications_path) + end + end + + context 'failure' do + before do + post :create, params: { + doorkeeper_application: { + name: '', + redirect_uri: '', + website: '', + scopes: '' + } + } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'renders form again' do + expect(response).to render_template(:new) + end + end + end + + describe 'PATCH #update' do + context 'success' do + let(:opts) { + { + website: 'https://foo.bar/' + } + } + + def call_update + patch :update, params: { + id: app.id, + doorkeeper_application: opts + } + response + end + + it 'updates existing application' do + call_update + expect(app.reload.website).to eql(opts[:website]) + end + + it 'redirects back to applications page' do + expect(call_update).to redirect_to(settings_applications_path) + end + end + + context 'failure' do + before do + patch :update, params: { + id: app.id, + doorkeeper_application: { + name: '', + redirect_uri: '', + website: '', + scopes: '' + } + } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'renders form again' do + expect(response).to render_template(:show) + end + end + end + + describe 'destroy' do + before do + post :destroy, params: { id: app.id } + end + + it 'redirects back to applications page' do + expect(response).to redirect_to(settings_applications_path) + end + + it 'removes the app' do + expect(Doorkeeper::Application.find_by(id: app.id)).to be_nil + end + end + + describe 'regenerate' do + let(:token) { user.token_for_app(app) } + before do + expect(token).to_not be_nil + put :regenerate, params: { application_id: app.id } + end + + it 'should create new token' do + expect(user.token_for_app(app)).to_not eql(token) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ef45818b97..99aeca01b2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -286,4 +286,24 @@ RSpec.describe User, type: :model do Fabricate(:user) end end + + describe 'token_for_app' do + let(:user) { Fabricate(:user) } + let(:app) { Fabricate(:application, owner: user) } + + it 'returns a token' do + expect(user.token_for_app(app)).to be_a(Doorkeeper::AccessToken) + end + + it 'persists a token' do + t = user.token_for_app(app) + expect(user.token_for_app(app)).to eql(t) + end + + it 'is nil if user does not own app' do + app.update!(owner: nil) + + expect(user.token_for_app(app)).to be_nil + end + end end