From 94cb4cd86817f6c51fd6b81614815d0112241a68 Mon Sep 17 00:00:00 2001 From: Lea Ann Bradford Date: Thu, 10 Aug 2023 15:03:26 -0700 Subject: [PATCH] delete auth provider code, put identity provider behind feature flipper (#667) --- app/controllers/auth_providers_controller.rb | 98 ------------------- app/models/account.rb | 1 - app/models/auth_provider.rb | 11 --- app/views/auth_providers/_form.html.erb | 41 -------- app/views/auth_providers/_oidc.html.erb | 5 - app/views/auth_providers/_saml.html.erb | 5 - app/views/auth_providers/edit.html.erb | 6 -- app/views/auth_providers/new.html.erb | 7 -- .../dashboard/sidebar/_configuration.html.erb | 16 +-- config/features.rb | 4 +- .../20230810193550_drop_auth_providers.rb | 16 +++ db/schema.rb | 15 +-- spec/factories/auth_providers.rb | 9 -- spec/models/auth_provider_spec.rb | 56 ----------- 14 files changed, 22 insertions(+), 268 deletions(-) delete mode 100644 app/controllers/auth_providers_controller.rb delete mode 100644 app/models/auth_provider.rb delete mode 100644 app/views/auth_providers/_form.html.erb delete mode 100644 app/views/auth_providers/_oidc.html.erb delete mode 100644 app/views/auth_providers/_saml.html.erb delete mode 100644 app/views/auth_providers/edit.html.erb delete mode 100644 app/views/auth_providers/new.html.erb create mode 100644 db/migrate/20230810193550_drop_auth_providers.rb delete mode 100644 spec/factories/auth_providers.rb delete mode 100644 spec/models/auth_provider_spec.rb diff --git a/app/controllers/auth_providers_controller.rb b/app/controllers/auth_providers_controller.rb deleted file mode 100644 index 28bc0a949..000000000 --- a/app/controllers/auth_providers_controller.rb +++ /dev/null @@ -1,98 +0,0 @@ -class AuthProvidersController < ApplicationController - layout 'hyrax/dashboard' - - before_action :ensure_admin! - before_action :set_auth_provider, only: %i[edit update destroy] - - # GET /auth_providers/new - def new - add_breadcrumbs - existing_auth_provider = AuthProvider.find(current_account.settings['auth_provider']) if current_account.settings['auth_provider'] - - # users should not be able to reach the new auth provider page unless it is the first time they are setting up an auth provider. - if existing_auth_provider - redirect_to edit_auth_provider_url(existing_auth_provider) - else - @auth_provider = AuthProvider.new - end - end - - # GET /auth_providers/1/edit - def edit - add_breadcrumbs - end - - # POST /auth_providers or /auth_providers.json - def create - @auth_provider = AuthProvider.new(auth_provider_params) - respond_to do |format| - if @auth_provider.save - current_account.settings['auth_provider'] = @auth_provider.id - current_account.save - format.html { redirect_to edit_auth_provider_url(@auth_provider), notice: "Auth provider was successfully created." } - format.json { render :show, status: :created, location: @auth_provider } - else - format.html { render :new, status: :unprocessable_entity } - format.json { render json: @auth_provider.errors, status: :unprocessable_entity } - end - end - end - - # PATCH/PUT /auth_providers/1 or /auth_providers/1.json - def update - respond_to do |format| - if @auth_provider.update(auth_provider_params) - current_account.settings['auth_provider'] = @auth_provider.id - current_account.save - format.html { redirect_to edit_auth_provider_url(@auth_provider), notice: "Auth provider was successfully updated." } - format.json { render :show, status: :ok, location: @auth_provider } - else - format.html { render :edit, status: :unprocessable_entity } - format.json { render json: @auth_provider.errors, status: :unprocessable_entity } - end - end - end - - # DELETE /auth_providers/1 or /auth_providers/1.json - def destroy - @auth_provider.destroy - current_account.settings['auth_provider'] = nil - current_account.save - respond_to do |format| - format.html { redirect_to new_auth_provider_url, notice: "Auth provider was successfully destroyed." } - format.json { head :no_content } - end - end - - def add_breadcrumbs - add_breadcrumb t(:'hyrax.controls.home'), root_path - add_breadcrumb t(:'hyrax.dashboard.breadcrumbs.admin'), hyrax.dashboard_path - add_breadcrumb t(:'hyrax.admin.sidebar.configuration'), '#' - add_breadcrumb t(:'hyrax.admin.sidebar.auth_provider'), request.path - end - - private - - # Use callbacks to share common setup or constraints between actions. - def set_auth_provider - @auth_provider = AuthProvider.find(params[:id]) - end - - def ensure_admin! - authorize! :read, :admin_dashboard - end - - # Only allow a list of trusted parameters through. - def auth_provider_params - params.require(:auth_provider).permit( - :provider, - :account_id, - :saml_client_id, - :saml_client_secret, - :saml_idp_sso_service_url, - :oidc_client_id, - :oidc_client_secret, - :oidc_idp_sso_service_url - ) - end -end diff --git a/app/models/account.rb b/app/models/account.rb index e707e1d8e..ba9500bfb 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -7,7 +7,6 @@ class Account < ApplicationRecord include AccountCname attr_readonly :tenant - has_one :auth_provider, dependent: :destroy has_many :sites, dependent: :destroy has_many :domain_names, dependent: :destroy has_many :full_account_cross_searches, diff --git a/app/models/auth_provider.rb b/app/models/auth_provider.rb deleted file mode 100644 index 9aa62a24f..000000000 --- a/app/models/auth_provider.rb +++ /dev/null @@ -1,11 +0,0 @@ -class AuthProvider < ApplicationRecord - belongs_to :account - - validates :provider, presence: true - - validates :oidc_client_id, :oidc_client_secret, :oidc_idp_sso_service_url, - presence: true, if: -> { provider == 'oidc' } - - validates :saml_client_id, :saml_client_secret, :saml_idp_sso_service_url, - presence: true, if: -> { provider == 'saml' } -end diff --git a/app/views/auth_providers/_form.html.erb b/app/views/auth_providers/_form.html.erb deleted file mode 100644 index 8d165c09c..000000000 --- a/app/views/auth_providers/_form.html.erb +++ /dev/null @@ -1,41 +0,0 @@ -
- <%= simple_form_for(@auth_provider) do |f| %> -
- <% if @auth_provider.errors.any? %> -
-

<%= pluralize(@auth_provider.errors.count, "error") %> prohibited this authentication provider from being saved:

-
    - <% @auth_provider.errors.messages.each do |key, messages| %> -
  • <%= key %> "<%= @auth_provider.errors.details[key].first[:value] %>" <%= messages.join(' and ') %>
    • - <% end %> -
-
- <% end %> - <%= f.hidden_field :account_id, value: @current_account.id %> - <%= f.input :provider, - collection: [["saml", "SAML"], ["oidc", "OIDC"]], - label_method: :second, - value_method: :first, - label: t('hyku.auth_provider.label.provider'), - required: true %> - -
-

Specific fields for provider are available only when the provider is selected

-
- <%= render partial: 'saml', locals: {f: f} %> -
-
- <%= render partial: 'oidc', locals: {f: f} %> -
-
-
- -
- <% if AuthProvider.count.nonzero? %> - <%= link_to 'Delete', auth_provider, method: :delete, data: { confirm: 'Are you sure?' }, class: - 'btn btn-danger' %> - <% end %> - <%= f.submit class: 'btn btn-primary action-save' %> -
- <% end %> -
diff --git a/app/views/auth_providers/_oidc.html.erb b/app/views/auth_providers/_oidc.html.erb deleted file mode 100644 index 45c69c3f6..000000000 --- a/app/views/auth_providers/_oidc.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%# TODO: Remove header and add appropriate form fields when we have more info %> -

OIDC FIELDS

-<%= f.input :oidc_client_id, label: t('hyku.auth_provider.label.client_id') %> -<%= f.input :oidc_client_secret, label: t('hyku.auth_provider.label.client_secret') %> -<%= f.input :oidc_idp_sso_service_url, label: t('hyku.auth_provider.label.idp_sso_service_url') %> diff --git a/app/views/auth_providers/_saml.html.erb b/app/views/auth_providers/_saml.html.erb deleted file mode 100644 index 99a348865..000000000 --- a/app/views/auth_providers/_saml.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%# TODO: Remove header and add appropriate form fields when we have more info %> -

SAML FIELDS

-<%= f.input :saml_client_id, label: t('hyku.auth_provider.label.client_id') %> -<%= f.input :saml_client_secret, label: t('hyku.auth_provider.label.client_secret') %> -<%= f.input :saml_idp_sso_service_url, label: t('hyku.auth_provider.label.idp_sso_service_url') %> \ No newline at end of file diff --git a/app/views/auth_providers/edit.html.erb b/app/views/auth_providers/edit.html.erb deleted file mode 100644 index 0739408e2..000000000 --- a/app/views/auth_providers/edit.html.erb +++ /dev/null @@ -1,6 +0,0 @@ -<% provide :page_header do %> -

<%= t('hyku.auth_provider.header') %>

-<% end %> - -<%= render 'form', auth_provider: @auth_provider %> - diff --git a/app/views/auth_providers/new.html.erb b/app/views/auth_providers/new.html.erb deleted file mode 100644 index 3503b06b0..000000000 --- a/app/views/auth_providers/new.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<% provide :page_header do %> -

<%= t('hyku.auth_provider.header') %>

-<% end %> - -<%= render 'form', auth_provider: @auth_provider %> - - diff --git a/app/views/hyrax/dashboard/sidebar/_configuration.html.erb b/app/views/hyrax/dashboard/sidebar/_configuration.html.erb index 71a5ed756..d4859b344 100644 --- a/app/views/hyrax/dashboard/sidebar/_configuration.html.erb +++ b/app/views/hyrax/dashboard/sidebar/_configuration.html.erb @@ -11,22 +11,12 @@ <%= t('hyrax.admin.sidebar.account') %> <% end %> - <% if Flipflop.show_auth_provider_in_admin_dashboard? %> - <% if AuthProvider.count > 0 %> - <%= menu.nav_link(main_app.edit_auth_provider_path(AuthProvider.where(account_id: @current_account.id).first)) do %> - <%= t('hyrax.admin.sidebar.auth_provider') %> - <% end %> - <% else %> - <%= menu.nav_link(main_app.new_auth_provider_path) do %> - <%= t('hyrax.admin.sidebar.auth_provider') %> - <% end %> + <% if Flipflop.show_identity_provider_in_admin_dashboard? %> + <%= menu.nav_link(main_app.identity_providers_path) do %> + <%= t('hyrax.admin.sidebar.identity_providers') %> <% end %> <% end %> - <%= menu.nav_link(main_app.identity_providers_path) do %> - <%= t('hyrax.admin.sidebar.identity_providers') %> - <% end %> - <%= menu.nav_link(main_app.edit_site_labels_path) do %> <%= t('hyrax.admin.sidebar.labels') %> <% end %> diff --git a/config/features.rb b/config/features.rb index 64f1fb0f6..122797297 100644 --- a/config/features.rb +++ b/config/features.rb @@ -21,7 +21,7 @@ default: true, description: "Shows the Recently Uploaded tab on the homepage." - feature :show_auth_provider_in_admin_dashboard, + feature :show_identity_provider_in_admin_dashboard, default: false, - description: "Shows the Auth Provider tab on the admin dashboard." + description: "Shows the Identity Provider tab on the admin dashboard." end diff --git a/db/migrate/20230810193550_drop_auth_providers.rb b/db/migrate/20230810193550_drop_auth_providers.rb new file mode 100644 index 000000000..784a74ecd --- /dev/null +++ b/db/migrate/20230810193550_drop_auth_providers.rb @@ -0,0 +1,16 @@ +class DropAuthProviders < ActiveRecord::Migration[5.2] + def change + drop_table :auth_providers do |t| + t.string "provider" + t.integer "account_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.string "oidc_client_id" + t.string "saml_client_id" + t.string "oidc_client_secret" + t.string "saml_client_secret" + t.string "oidc_idp_sso_service_url" + t.string "saml_idp_sso_service_url" + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 309efbf41..e111b4d6a 100755 --- 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: 2023_08_04_202804) do +ActiveRecord::Schema.define(version: 2023_08_10_193550) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -46,19 +46,6 @@ t.index ["solr_endpoint_id"], name: "index_accounts_on_solr_endpoint_id", unique: true end - create_table "auth_providers", force: :cascade do |t| - t.string "provider" - t.integer "account_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.string "oidc_client_id" - t.string "saml_client_id" - t.string "oidc_client_secret" - t.string "saml_client_secret" - t.string "oidc_idp_sso_service_url" - t.string "saml_idp_sso_service_url" - end - create_table "bookmarks", id: :serial, force: :cascade do |t| t.integer "user_id", null: false t.string "user_type" diff --git a/spec/factories/auth_providers.rb b/spec/factories/auth_providers.rb deleted file mode 100644 index 097061005..000000000 --- a/spec/factories/auth_providers.rb +++ /dev/null @@ -1,9 +0,0 @@ -FactoryBot.define do - factory :auth_provider, class: 'AuthProvider' do - provider { 'saml' } - client_id { 'client_id' } - client_secret { 'client_secret' } - idp_sso_service_url { 'idp_sso_service_url' } - account_id { 1 } - end -end diff --git a/spec/models/auth_provider_spec.rb b/spec/models/auth_provider_spec.rb deleted file mode 100644 index 430de4ae1..000000000 --- a/spec/models/auth_provider_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -require 'rails_helper' - -RSpec.describe AuthProvider, type: :model do - subject do - described_class.new( - provider: 'saml', - oidc_client_id: 'client_id', - saml_client_id: 'client_id', - oidc_client_secret: 'client_secret', - saml_client_secret: 'client_secret', - oidc_idp_sso_service_url: 'oidc_idp_sso_service_url', - saml_idp_sso_service_url: 'saml_idp_sso_service_url', - account_id: 1 - ) - end - - context 'attributes and validations' do - it 'is valid with valid attributes' do - expect(subject).to be_valid - end - - it 'is not valid without a provider' do - subject.provider = nil - expect(subject).not_to be_valid - end - - it 'is not valid without a client_id' do - subject.oidc_client_id = nil - subject.saml_client_id = nil - expect(subject).not_to be_valid - end - - it 'is not valid without a client_secret' do - subject.oidc_client_secret = nil - subject.saml_client_secret = nil - expect(subject).not_to be_valid - end - end - - context 'transactions' do - it 'has none to begin with' do - expect(described_class.count).to eq 0 - end - - it 'has one after adding one' do - AuthProvider.create( - provider: 'oidc', - oidc_client_id: 'new oidc_client_id', - oidc_client_secret: 'new oidc_client_secret', - oidc_idp_sso_service_url: 'new oidc_idp_sso_service_url', - account_id: 1 - ) - expect(described_class.count).to eq 1 - end - end -end