From 84bd3a69d73ab873cc5e372412a51521f3ec83f1 Mon Sep 17 00:00:00 2001 From: Timothy Ng <5664347+timorthi@users.noreply.github.com> Date: Tue, 8 Jan 2019 09:30:54 -0800 Subject: [PATCH] ApiSecret model for Developer API (#1369) * Add ApiSecret model scaffold * Add relationships between ApiSecret, User, Organization * Add placeholder template to account tab * Add description column to ApiSecrets * Add very basic access token generation * Add basic access token deletion * Show access token on success flash * Add placeholder message when user has no secrets * Use cta style on token generation button * Add presence validation for ApiSecret description * Add ApiSecrets factory * Add pundit policy for ApiSecret * Nest form field within api_secret hash to allow rails strong params * Use pundit to authorize ApiSecretController actions * Add error message flash for ApiSecretsController actions * Add specs for ApiSecretsController * Add length validation to api secret description * Flash model error instead of generic message on save failure * Truncate ApiSecret factory objects descriptions to prevent validation error * Remove length restriction on ApiSecret.description * Use darker font color for token creation date * Consolidate ApiSecret migrations --- app/assets/stylesheets/settings.scss | 35 ++++++++++++++- app/controllers/api_secrets_controller.rb | 32 ++++++++++++++ app/models/api_secret.rb | 5 +++ app/models/organization.rb | 1 + app/models/user.rb | 1 + app/policies/api_secret_policy.rb | 19 ++++++++ app/views/users/_account.html.erb | 36 ++++++++++++++- config/routes.rb | 2 + .../20181219215401_create_api_secrets.rb | 13 ++++++ db/schema.rb | 12 ++++- spec/factories/api_secrets.rb | 7 +++ spec/models/api_secret_spec.rb | 8 ++++ spec/models/user_spec.rb | 1 + spec/policies/api_secret_policy_spec.rb | 29 ++++++++++++ spec/requests/api_secrets_create_spec.rb | 44 +++++++++++++++++++ spec/requests/api_secrets_destroy_spec.rb | 41 +++++++++++++++++ 16 files changed, 282 insertions(+), 4 deletions(-) create mode 100644 app/controllers/api_secrets_controller.rb create mode 100644 app/models/api_secret.rb create mode 100644 app/policies/api_secret_policy.rb create mode 100644 db/migrate/20181219215401_create_api_secrets.rb create mode 100644 spec/factories/api_secrets.rb create mode 100644 spec/models/api_secret_spec.rb create mode 100644 spec/policies/api_secret_policy_spec.rb create mode 100644 spec/requests/api_secrets_create_spec.rb create mode 100644 spec/requests/api_secrets_destroy_spec.rb diff --git a/app/assets/stylesheets/settings.scss b/app/assets/stylesheets/settings.scss index 40164e65a..f1573aaf9 100755 --- a/app/assets/stylesheets/settings.scss +++ b/app/assets/stylesheets/settings.scss @@ -85,7 +85,7 @@ max-width:700px; float: left; h2{ - font-size: 2.5em; + font-size: 2.5em; } @media screen and ( min-width: 835px ) { width: 66%; @@ -516,4 +516,35 @@ ul.delete__account{ li{ padding-bottom: 5px; } -} \ No newline at end of file +} + +.api__secret { + &__container { + background-color: $off-white; + height: 3rem; + margin: 1rem 0; + padding: 0.5rem; + } + + &__desc { + width: 70%; + float: left; + + .title { + font-weight: bold; + margin: 0; + padding-bottom: 0.25rem; + } + .subtitle { + margin: 0; + color: $dark-medium-gray; + } + } + + &__revoke { + float: left; + width: 30%; + text-align: right; + margin: 0 auto; + } +} diff --git a/app/controllers/api_secrets_controller.rb b/app/controllers/api_secrets_controller.rb new file mode 100644 index 000000000..21df36e5b --- /dev/null +++ b/app/controllers/api_secrets_controller.rb @@ -0,0 +1,32 @@ +class ApiSecretsController < ApplicationController + before_action :set_api_secret, only: :destroy + after_action :verify_authorized + + def create + authorize ApiSecret + @secret = ApiSecret.new(permitted_attributes(ApiSecret)) + @secret.user_id = current_user.id + if @secret.save + flash[:notice] = "Your access token has been generated: #{@secret.secret}. Be sure to copy it to somewhere safe now. You won’t be able to see it again!" + else + flash[:error] = @secret.errors.full_messages.to_sentence + end + redirect_back(fallback_location: root_path) + end + + def destroy + authorize @secret + if @secret.destroy + flash[:notice] = "Your access token has been revoked." + else + flash[:error] = "An error occurred. Please try again or send an email to: yo@dev.to" + end + redirect_back(fallback_location: root_path) + end + + private + + def set_api_secret + @secret = ApiSecret.find_by_id(params[:id]) || not_found + end +end diff --git a/app/models/api_secret.rb b/app/models/api_secret.rb new file mode 100644 index 000000000..3a03dbfa6 --- /dev/null +++ b/app/models/api_secret.rb @@ -0,0 +1,5 @@ +class ApiSecret < ApplicationRecord + has_secure_token :secret + belongs_to :user + validates :description, presence: true +end diff --git a/app/models/organization.rb b/app/models/organization.rb index 771fae51b..a50eb5bbe 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -5,6 +5,7 @@ class Organization < ApplicationRecord has_many :job_listings has_many :users + has_many :api_secrets, through: :users has_many :articles has_many :collections has_many :display_ads diff --git a/app/models/user.rb b/app/models/user.rb index 60450008e..2687a948b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -15,6 +15,7 @@ class User < ApplicationRecord acts_as_follower belongs_to :organization, optional: true + has_many :api_secrets, dependent: :destroy has_many :articles, dependent: :destroy has_many :badge_achievements, dependent: :destroy has_many :badges, through: :badge_achievements diff --git a/app/policies/api_secret_policy.rb b/app/policies/api_secret_policy.rb new file mode 100644 index 000000000..614f6b460 --- /dev/null +++ b/app/policies/api_secret_policy.rb @@ -0,0 +1,19 @@ +class ApiSecretPolicy < ApplicationPolicy + def create? + true + end + + def destroy? + user_is_owner? + end + + def permitted_attributes + %i[description] + end + + private + + def user_is_owner? + user.id == record.user_id + end +end diff --git a/app/views/users/_account.html.erb b/app/views/users/_account.html.erb index 2b0b261c5..a24464f2b 100644 --- a/app/views/users/_account.html.erb +++ b/app/views/users/_account.html.erb @@ -14,6 +14,40 @@ <% end %> +

Access Tokens

+

You can generate personal access tokens to use for authentication with the DEV API.

+

Generate an access token

+

Token description

+<%= form_tag users_api_secrets_path, method: :post do %> + <%= fields_for :api_secret do |api_secret| %> +
+ <%= api_secret.text_field(:description, placeholder: "What's this token for?", required: true) %> +
+ <% end %> + +<% end %> + +

Active access tokens

+<% if @user.api_secrets.empty? %> +

None yet!

+<% end %> +<% @user.api_secrets.order(created_at: :desc).each do |api_secret| %> +
+
+

<%= api_secret.description %> +

Created <%= api_secret.created_at.to_date.to_s %>

+
+
+ <%= form_tag users_api_secrets_path(id: api_secret.id), method: :delete do %> + <%= button_tag('Revoke', class: 'big-action danger-button') %> + <% end %> +
+
+<% end %> + +

Danger Zone

<% if @user.identities.count == 2 %>

Remove OAuth Associations

@@ -28,7 +62,7 @@ Note that this does not revoke our OAuth app access; you will have to do so in your - Twitter profile settings or your + Twitter profile settings or your GitHub profile settings.
diff --git a/config/routes.rb b/config/routes.rb index 6aec1d332..44bc0b218 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -181,6 +181,8 @@ Rails.application.routes.draw do delete "users/remove_association", to: "users#remove_association" delete "users/destroy", to: "users#destroy" post "organizations/generate_new_secret" => "organizations#generate_new_secret" + post "users/api_secrets" => "api_secrets#create" + delete "users/api_secrets" => "api_secrets#destroy" # The priority is based upon order of creation: first created -> highest priority. # See how all your routes lay out with "rake routes". diff --git a/db/migrate/20181219215401_create_api_secrets.rb b/db/migrate/20181219215401_create_api_secrets.rb new file mode 100644 index 000000000..33fe61067 --- /dev/null +++ b/db/migrate/20181219215401_create_api_secrets.rb @@ -0,0 +1,13 @@ +class CreateApiSecrets < ActiveRecord::Migration[5.1] + def change + create_table :api_secrets do |t| + t.string :secret + t.integer :user_id + t.string :description, null: false + + t.timestamps + end + add_index :api_secrets, :secret, unique: true + add_index :api_secrets, :user_id + end +end diff --git a/db/schema.rb b/db/schema.rb index e67ef4a23..cfeccf580 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20181130224531) do +ActiveRecord::Schema.define(version: 20181219215401) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -35,6 +35,16 @@ ActiveRecord::Schema.define(version: 20181130224531) do t.index ["user_id", "user_type"], name: "index_ahoy_messages_on_user_id_and_user_type" end + create_table "api_secrets", force: :cascade do |t| + t.datetime "created_at", null: false + t.string "description", null: false + t.string "secret" + t.datetime "updated_at", null: false + t.integer "user_id" + t.index ["secret"], name: "index_api_secrets_on_secret", unique: true + t.index ["user_id"], name: "index_api_secrets_on_user_id" + end + create_table "articles", id: :serial, force: :cascade do |t| t.string "abuse_removal_reason" t.boolean "allow_big_edits", default: true diff --git a/spec/factories/api_secrets.rb b/spec/factories/api_secrets.rb new file mode 100644 index 000000000..ec026cfb8 --- /dev/null +++ b/spec/factories/api_secrets.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :api_secret do + user + description { Faker::Lorem.sentence } + secret { SecureRandom.base58(24) } + end +end diff --git a/spec/models/api_secret_spec.rb b/spec/models/api_secret_spec.rb new file mode 100644 index 000000000..dd2efda75 --- /dev/null +++ b/spec/models/api_secret_spec.rb @@ -0,0 +1,8 @@ +require "rails_helper" + +RSpec.describe ApiSecret, type: :model do + describe "validations" do + it { is_expected.to belong_to(:user) } + it { is_expected.to validate_presence_of(:description) } + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7cd936573..ba6353621 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -13,6 +13,7 @@ RSpec.describe User, type: :model do before { mock_auth_hash } describe "validations" do + it { is_expected.to have_many(:api_secrets) } it { is_expected.to have_many(:articles) } it { is_expected.to have_many(:badge_achievements).dependent(:destroy) } it { is_expected.to have_many(:badges).through(:badge_achievements) } diff --git a/spec/policies/api_secret_policy_spec.rb b/spec/policies/api_secret_policy_spec.rb new file mode 100644 index 000000000..e6f1638a9 --- /dev/null +++ b/spec/policies/api_secret_policy_spec.rb @@ -0,0 +1,29 @@ +require "rails_helper" + +RSpec.describe ApiSecretPolicy do + subject { described_class.new(user, api_secret) } + + let(:api_secret) { build(:api_secret) } + let(:valid_attributes) { %i[description] } + + context "when user is not signed in" do + let(:user) { nil } + + it { within_block_is_expected.to raise_error(Pundit::NotAuthorizedError) } + end + + context "when user owns the secret" do + let(:user) { api_secret.user } + + it { is_expected.to permit_actions %i[create destroy] } + it { is_expected.to permit_mass_assignment_of(valid_attributes) } + end + + context "when user does not own the secret" do + let(:user) { build(:user) } + + it { is_expected.to permit_actions %i[create] } + it { is_expected.to forbid_actions %i[destroy] } + it { is_expected.to permit_mass_assignment_of(valid_attributes) } + end +end diff --git a/spec/requests/api_secrets_create_spec.rb b/spec/requests/api_secrets_create_spec.rb new file mode 100644 index 000000000..cd0446006 --- /dev/null +++ b/spec/requests/api_secrets_create_spec.rb @@ -0,0 +1,44 @@ +require "rails_helper" + +RSpec.describe "ApiSecretsCreate", type: :request do + let(:user) { build(:user) } + + before { sign_in user } + + describe "POST /users/api_secrets" do + context "when create succeeds" do + let(:valid_params) { { description: "My Test 3rd Party App" } } + + it "creates an ApiSecret for the user" do + expect { post "/users/api_secrets", params: { api_secret: valid_params } }. + to change { user.api_secrets.count }.by 1 + end + + it "sets the description" do + post "/users/api_secrets", params: { api_secret: valid_params } + expect(user.api_secrets.last.description).to eq valid_params[:description] + end + + it "flashes a message containing the token" do + post "/users/api_secrets", params: { api_secret: valid_params } + expect(flash[:notice]).to include(ApiSecret.last.secret) + expect(flash[:error]).to be_nil + end + end + + context "when create fails" do + let(:invalid_params) { { description: nil } } # Force model validation error + + it "does not create the ApiSecret" do + expect { post "/users/api_secrets", params: { api_secret: invalid_params } }. + not_to (change { user.api_secrets.count }) + end + + it "flashes an error message" do + post "/users/api_secrets", params: { api_secret: invalid_params } + expect(flash[:error]).to be_truthy + expect(flash[:notice]).to be_nil + end + end + end +end diff --git a/spec/requests/api_secrets_destroy_spec.rb b/spec/requests/api_secrets_destroy_spec.rb new file mode 100644 index 000000000..3c4203898 --- /dev/null +++ b/spec/requests/api_secrets_destroy_spec.rb @@ -0,0 +1,41 @@ +require "rails_helper" + +RSpec.describe "ApiSecretsDestroy", type: :request do + let(:api_secret) { create(:api_secret) } + let(:user) { api_secret.user } + + before { sign_in user } + + describe "DELETE /users/api_secrets" do + context "when delete succeeds" do + it "deletes the ApiSecret for the user" do + expect { delete "/users/api_secrets", params: { id: api_secret.id } }. + to change { user.api_secrets.count }.by -1 + end + + it "flashes a notice" do + delete "/users/api_secrets", params: { id: api_secret.id } + expect(flash[:notice]).to be_truthy + expect(flash[:error]).to be_nil + end + end + + context "when delete fails" do + before do + allow(ApiSecret).to receive(:find_by_id).and_return api_secret + allow(api_secret).to receive(:destroy).and_return false + end + + it "does not delete the ApiSecret" do + expect { delete "/users/api_secrets", params: { id: api_secret.id } }. + not_to (change { user.api_secrets.count }) + end + + it "flashes an error message" do + delete "/users/api_secrets", params: { id: api_secret.id } + expect(flash[:error]).to be_truthy + expect(flash[:notice]).to be_nil + end + end + end +end