Allow external users to configure CodeHarbor links

MrSerth · MrSerth · commit 6a61cb0037e5 · 2024-11-16T12:36:08.000+01:00
Previously, only a user themselves was able to change the CodeHarbor link. Now, we modify the behavior, so that admins can change the links, too. Furthermore, this commit fixes the incorrect styling in case of errors, the incorrect bootstrap tooltips and locales shown on the page. Closes #1457
diff --git a/app/assets/stylesheets/base.css.scss b/app/assets/stylesheets/base.css.scss
@@ -200,3 +200,9 @@ html[data-bs-theme="light"] {
     }
   }
 }
+
+.input-group {
+  .field_with_errors {
+    flex: 1 1 auto;
+  }
+}
diff --git a/app/controllers/codeharbor_links_controller.rb b/app/controllers/codeharbor_links_controller.rb
@@ -4,11 +4,15 @@ class CodeharborLinksController < ApplicationController
   include CommonBehavior
   before_action :verify_codeharbor_activation
   before_action :set_codeharbor_link, only: %i[edit update destroy]
+  before_action :set_user_and_authorize
 
   def new
     base_url = CodeOcean::Config.new(:code_ocean).read[:codeharbor][:url] || ''
-    @codeharbor_link = CodeharborLink.new(push_url: "#{base_url}/import_task",
-      check_uuid_url: "#{base_url}/import_uuid_check")
+    @codeharbor_link = CodeharborLink.new(
+      push_url: "#{base_url}/import_task",
+      check_uuid_url: "#{base_url}/import_uuid_check",
+      user: @user
+    )
     authorize!
   end
 
@@ -18,24 +22,26 @@ def edit
 
   def create
     @codeharbor_link = CodeharborLink.new(codeharbor_link_params)
-    @codeharbor_link.user = current_user
+    @codeharbor_link.user = @user
     authorize!
-    create_and_respond(object: @codeharbor_link, path: -> { @codeharbor_link.user })
+    create_and_respond(object: @codeharbor_link, path: -> { @user })
   end
 
   def update
     authorize!
-    update_and_respond(object: @codeharbor_link, params: codeharbor_link_params, path: @codeharbor_link.user)
+    update_and_respond(object: @codeharbor_link, params: codeharbor_link_params, path: @user)
   end
 
   def destroy
-    destroy_and_respond(object: @codeharbor_link, path: @codeharbor_link.user)
+    destroy_and_respond(object: @codeharbor_link, path: @user)
   end
 
   private
 
   def authorize!
-    authorize @codeharbor_link
+    raise Pundit::NotAuthorizedError if @codeharbor_link.present? && @user.present? && @codeharbor_link.user != @user
+
+    authorize(@codeharbor_link)
   end
 
   def verify_codeharbor_activation
@@ -47,6 +53,16 @@ def set_codeharbor_link
     authorize!
   end
 
+  def set_user_and_authorize
+    if params[:external_user_id]
+      @user = ExternalUser.find(params[:external_user_id])
+    else
+      @user = InternalUser.find(params[:internal_user_id])
+    end
+    params[:user_id] = @user.id_with_type # for the breadcrumbs
+    authorize(@user, :change_codeharbor_link?)
+  end
+
   def codeharbor_link_params
     params.require(:codeharbor_link).permit(:push_url, :check_uuid_url, :api_key)
   end
diff --git a/app/models/codeharbor_link.rb b/app/models/codeharbor_link.rb
@@ -1,13 +1,17 @@
 # frozen_string_literal: true
 
 class CodeharborLink < ApplicationRecord
+  include Creation
+
   validates :push_url, presence: true
   validates :check_uuid_url, presence: true
   validates :api_key, presence: true
 
-  belongs_to :user, class_name: 'InternalUser'
-
   def to_s
     "#{model_name.human} #{id}"
   end
+
+  def self.parent_resource
+    User
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
@@ -80,6 +80,14 @@ def self.find_by_id_with_type(id_with_type)
     end
   end
 
+  def self.find_by(*args, id: nil)
+    # Allow `User.find_by(id: 'e123')` and `User.find_by(id: 'i123')`
+    # Inherited classes keep their original behavior.
+    return super unless self == User
+
+    find_by_id_with_type(id)
+  end
+
   def self.ransackable_attributes(auth_object)
     if auth_object.present? && auth_object.admin?
       %w[name email external_id consumer_id platform_admin id]
diff --git a/app/policies/codeharbor_link_policy.rb b/app/policies/codeharbor_link_policy.rb
@@ -3,32 +3,16 @@
 class CodeharborLinkPolicy < ApplicationPolicy
   CODEHARBOR_CONFIG = CodeOcean::Config.new(:code_ocean).read[:codeharbor]
 
-  def index?
-    no_one
+  %i[index? show?].each do |action|
+    define_method(action) { no_one }
   end
 
-  def show?
-    no_one
+  %i[new? create?].each do |action|
+    define_method(action) { enabled? && (admin? || teacher?) && (@user.admin? || @user.teacher?) }
   end
 
-  def new?
-    enabled? && (teacher? || admin?)
-  end
-
-  def create?
-    enabled? && (teacher? || admin?)
-  end
-
-  def edit?
-    enabled? && owner?
-  end
-
-  def update?
-    enabled? && owner?
-  end
-
-  def destroy?
-    enabled? && owner?
+  %i[destroy? update? edit?].each do |action|
+    define_method(action) { enabled? && (admin? || owner?) }
   end
 
   def enabled?
@@ -38,6 +22,6 @@ def enabled?
   private
 
   def owner?
-    @record.reload.user == @user
+    @record.user == @user
   end
 end
diff --git a/app/policies/external_user_policy.rb b/app/policies/external_user_policy.rb
@@ -17,6 +17,10 @@ def tag_statistics?
     admin?
   end
 
+  def change_codeharbor_link?
+    admin? || @record == @user
+  end
+
   class Scope < Scope
     def resolve
       if @user.admin?
diff --git a/app/policies/internal_user_policy.rb b/app/policies/internal_user_policy.rb
@@ -13,6 +13,10 @@ def show?
     admin? || @record == @user || teacher_in_study_group?
   end
 
+  def change_codeharbor_link?
+    admin? || @record == @user
+  end
+
   class Scope < Scope
     def resolve
       if @user.admin?
diff --git a/app/views/codeharbor_links/_form.html.slim b/app/views/codeharbor_links/_form.html.slim
@@ -1,18 +1,18 @@
-= form_for(@codeharbor_link) do |f|
+= form_for([@user, @codeharbor_link]) do |f|
   = render('shared/form_errors', object: @codeharbor_link)
   .mb-3
     = f.label(:push_url, class: 'form-label')
-    = f.text_field :push_url, data: {toggle: 'tooltip', placement: 'bottom'}, title: t('codeharbor_link.info.push_url'), class: 'form-control'
+    = f.text_field :push_url, data: {'bs-toggle': 'tooltip', 'bs-placement': 'bottom'}, title: t('codeharbor_link.info.push_url'), class: 'form-control'
   .mb-3
     = f.label(:check_uuid_url, class: 'form-label')
-    = f.text_field :check_uuid_url, data: {toggle: 'tooltip', placement: 'bottom'}, title: t('codeharbor_link.info.check_uuid_url'), class: 'form-control'
+    = f.text_field :check_uuid_url, data: {'bs-toggle': 'tooltip', 'bs-placement': 'bottom'}, title: t('codeharbor_link.info.check_uuid_url'), class: 'form-control'
   .mb-3
     = f.label(:api_key, class: 'form-label')
     .input-group
-      = f.text_field(:api_key, data: {toggle: 'tooltip', placement: 'bottom'}, title: t('codeharbor_link.info.api_key'), class: 'form-control api_key')
+      = f.text_field(:api_key, data: {'bs-toggle': 'tooltip', 'bs-placement': 'bottom'}, title: t('codeharbor_link.info.api_key'), class: 'form-control api_key')
       .input-group-btn
         = button_tag t('codeharbor_link.generate'), type: 'button', class: 'generate-api_key btn btn-default'
   .actions
     = render('shared/submit_button', f:, object: @codeharbor_link)
     - if @codeharbor_link.persisted?
-      = link_to(t('codeharbor_link.delete'), codeharbor_link_path(@codeharbor_link), data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'btn btn-danger float-end')
+      = link_to(t('codeharbor_link.delete'), polymorphic_path([@codeharbor_link.user, @codeharbor_link]), data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'btn btn-danger float-end')
diff --git a/app/views/external_users/show.html.slim b/app/views/external_users/show.html.slim
@@ -31,6 +31,9 @@ h1 = @user.displayname
   - else
     = t('users.show.no_groups')
 
+- if @user == current_user || current_user.admin?
+  = row(label: 'codeharbor_link.profile_label', value: @user.codeharbor_link.nil? ? link_to(t('codeharbor_link.new'), polymorphic_path([@user, CodeharborLink], action: :new), class: 'btn btn-secondary') : link_to(t('codeharbor_link.edit'), polymorphic_path([@user, @user.codeharbor_link], action: :edit), class: 'btn btn-secondary'))
+
 h4.mt-4 = link_to(t('.exercise_statistics'), statistics_external_user_path(@user)) if policy(@user).statistics?
 
 - if current_user.admin?
diff --git a/app/views/internal_users/show.html.slim b/app/views/internal_users/show.html.slim
@@ -28,5 +28,5 @@ h1
   - else
     = t('users.show.no_groups')
 
-- if @user == current_user
-  = row(label: 'codeharbor_link.profile_label', value: @user.codeharbor_link.nil? ? link_to(t('codeharbor_link.new'), new_codeharbor_link_path, class: 'btn btn-secondary') : link_to(t('codeharbor_link.edit'), edit_codeharbor_link_path(@user.codeharbor_link), class: 'btn btn-secondary'))
+- if @user == current_user || current_user.admin?
+  = row(label: 'codeharbor_link.profile_label', value: @user.codeharbor_link.nil? ? link_to(t('codeharbor_link.new'), polymorphic_path([@user, CodeharborLink], action: :new), class: 'btn btn-secondary') : link_to(t('codeharbor_link.edit'), polymorphic_path([@user, @user.codeharbor_link], action: :edit), class: 'btn btn-secondary'))
diff --git a/config/locales/de/codeharbor_link.yml b/config/locales/de/codeharbor_link.yml
@@ -3,7 +3,9 @@ de:
   activerecord:
     attributes:
       codeharbor_link:
-        oauth2token: OAuth2 Token
+        api_key: API-Schlüssel
+        check_uuid_url: Check-UUID-URL
+        push_url: Push-URL
     models:
       codeharbor_link:
         one: CodeHarbor-Link
diff --git a/config/locales/en/codeharbor_link.yml b/config/locales/en/codeharbor_link.yml
@@ -3,7 +3,9 @@ en:
   activerecord:
     attributes:
       codeharbor_link:
-        oauth2token: OAuth2 Token
+        api_key: API Key
+        check_uuid_url: Check UUID URL
+        push_url: Push URL
     models:
       codeharbor_link:
         one: CodeHarbor Link
diff --git a/config/routes.rb b/config/routes.rb
@@ -16,7 +16,6 @@
       get 'by_file_type/:file_type_id', as: :by_file_type, action: :by_file_type
     end
   end
-  resources :codeharbor_links, only: %i[new create edit update destroy]
   resources :request_for_comments, except: %i[edit destroy] do
     member do
       get :mark_as_solved, defaults: {format: :json}
@@ -124,6 +123,7 @@
     member do
       get :tag_statistics
     end
+    resources :codeharbor_links, only: %i[new create edit update destroy]
   end
 
   namespace :code_ocean do
@@ -139,6 +139,7 @@
       match 'activate', to: 'internal_users#activate', via: %i[get patch put]
       match 'reset_password', to: 'internal_users#reset_password', via: %i[get patch put]
     end
+    resources :codeharbor_links, only: %i[new create edit update destroy]
   end
 
   match '/forgot_password', as: 'forgot_password', to: 'internal_users#forgot_password', via: %i[get post]
diff --git a/db/migrate/20241116105338_backfill_user_type_to_codeharbor_links.rb b/db/migrate/20241116105338_backfill_user_type_to_codeharbor_links.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class BackfillUserTypeToCodeharborLinks < ActiveRecord::Migration[7.2]
+  class CodeharborLink < ApplicationRecord
+  end
+
+  class InternalUser < ApplicationRecord
+  end
+
+  disable_ddl_transaction!
+
+  def change
+    CodeharborLink.where(user_type: nil).update_all(user_type: InternalUser.name.demodulize) # rubocop:disable Rails/SkipsModelValidations
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/spec/controllers/codeharbor_links_controller_spec.rb b/spec/controllers/codeharbor_links_controller_spec.rb
@@ -15,7 +15,7 @@
 
   describe 'GET #new' do
     before do
-      get :new
+      get :new, params: {internal_user_id: user.id}
     end
 
     expect_http_status(:ok)
@@ -24,13 +24,13 @@
   describe 'GET #edit' do
     let(:codeharbor_link) { create(:codeharbor_link, user:) }
 
-    before { get :edit, params: {id: codeharbor_link.id} }
+    before { get :edit, params: {id: codeharbor_link.id, internal_user_id: user.id} }
 
     expect_http_status(:ok)
   end
 
   describe 'POST #create' do
-    let(:post_request) { post :create, params: {codeharbor_link: params} }
+    let(:post_request) { post :create, params: {codeharbor_link: params, internal_user_id: user.id} }
     let(:params) { {push_url: 'https://foo.bar/push', check_uuid_url: 'https://foo.bar/check', api_key: 'api_key'} }
 
     it 'creates a codeharbor_link' do
@@ -56,7 +56,7 @@
 
   describe 'PUT #update' do
     let(:codeharbor_link) { create(:codeharbor_link, user:) }
-    let(:put_request) { patch :update, params: {id: codeharbor_link.id, codeharbor_link: params} }
+    let(:put_request) { patch :update, params: {id: codeharbor_link.id, codeharbor_link: params, internal_user_id: user.id} }
     let(:params) { {push_url: 'https://foo.bar/push', check_uuid_url: 'https://foo.bar/check', api_key: 'api_key'} }
 
     it 'updates push_url' do
@@ -90,7 +90,7 @@
 
   describe 'DELETE #destroy' do
     let!(:codeharbor_link) { create(:codeharbor_link, user:) }
-    let(:destroy_request) { delete :destroy, params: {id: codeharbor_link.id} }
+    let(:destroy_request) { delete :destroy, params: {id: codeharbor_link.id, internal_user_id: user.id} }
 
     it 'deletes codeharbor_link' do
       expect { destroy_request }.to change(CodeharborLink, :count).by(-1)
diff --git a/spec/models/codeharbor_link_spec.rb b/spec/models/codeharbor_link_spec.rb
@@ -13,6 +13,6 @@
 
     let(:codeharbor_link) { create(:codeharbor_link) }
 
-    it { is_expected.to eql codeharbor_link.id.to_s }
+    it { is_expected.to eql "#{described_class.model_name.human} #{codeharbor_link.id}" }
   end
 end
diff --git a/spec/policies/codeharbor_link_policy_spec.rb b/spec/policies/codeharbor_link_policy_spec.rb
@@ -42,8 +42,12 @@
           expect(policy).to permit(codeharbor_link.user, codeharbor_link)
         end
 
+        it 'grants access to admins' do
+          expect(policy).to permit(create(:admin), codeharbor_link)
+        end
+
         it 'does not grant access to arbitrary users' do
-          %i[external_user admin teacher].each do |factory_name|
+          %i[external_user teacher].each do |factory_name|
             expect(policy).not_to permit(create(factory_name), codeharbor_link)
           end
         end

Original file line number	Diff line number	Diff line change
`@@ -200,3 +200,9 @@ html[data-bs-theme="light"] {`
`200`	`200`	`}`
`201`	`201`	`}`
`202`	`202`	`}`
	`203`	`+`
	`204`	`+.input-group {`
	`205`	`+ .field_with_errors {`
	`206`	`+ flex: 1 1 auto;`
	`207`	`+ }`
	`208`	`+}`