Skip to content

Conversation

@rsmithlal
Copy link
Member

Summary

  • add HostDashboardPolicy enforcing manage_platform permission
  • authorize host dashboard in controller
  • test authorization for policy and controller access

Testing

  • bundle exec rubocop (fails: command not found: rubocop)
  • bundle exec brakeman -q -w2 (fails: command not found: brakeman)
  • bundle exec bundler-audit --update (fails: command not found: bundler-audit)
  • bin/codex_style_guard (fails: command not found: rubocop)
  • bin/ci (fails: command not found: rails)

https://chatgpt.com/codex/tasks/task_e_689a5937d7808321b6844e6be1fd15ae

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds authorization to the host dashboard by implementing a HostDashboardPolicy that enforces the manage_platform permission. The implementation includes the policy class, controller authorization check, and test coverage for both the policy and controller.

Key Changes

  • New HostDashboardPolicy enforcing manage_platform permission requirement
  • Authorization check added to HostDashboardController#index action
  • Test coverage for policy authorization rules and controller access control

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
app/policies/better_together/host_dashboard_policy.rb New policy class enforcing manage_platform permission for dashboard access
app/controllers/better_together/host_dashboard_controller.rb Added authorize :host_dashboard, :index? call to secure the index action
spec/policies/better_together/host_dashboard_policy_spec.rb Policy tests covering authorized/unauthorized access scenarios
spec/controllers/better_together/host_dashboard_controller_spec.rb Controller tests verifying authorization enforcement

Comment on lines +1 to +40
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe BetterTogether::HostDashboardController, type: :controller do
include Devise::Test::ControllerHelpers
include BetterTogether::DeviseSessionHelpers

routes { BetterTogether::Engine.routes }

before do
configure_host_platform
@request.env['devise.mapping'] = Devise.mappings[:user]
end

describe 'GET #index' do
context 'when user can manage platform' do
let(:user) { BetterTogether::User.find_by(email: '[email protected]') }

before { sign_in user }

it 'returns http success' do
get :index, params: { locale: I18n.default_locale }
expect(response).to be_successful
end
end

context 'when user cannot manage platform' do
let(:user) { create(:user, :confirmed) }

before { sign_in user }

it 'raises Pundit::NotAuthorizedError' do
expect do
get :index, params: { locale: I18n.default_locale }
end.to raise_error(Pundit::NotAuthorizedError)
end
end
end
end
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the project's testing architecture standards (guideline 1000000), this should be a request spec (type: :request) instead of a controller spec. The project follows a consistent pattern of using request specs for all controller testing to maintain architectural consistency and handle Rails engine routing automatically.

Please convert this to a request spec similar to spec/requests/better_together/communities_controller_spec.rb:

# frozen_string_literal: true

require 'rails_helper'

RSpec.describe 'BetterTogether::HostDashboard', :as_platform_manager do
  let(:locale) { I18n.default_locale }

  describe 'GET /:locale/host' do
    it 'renders index for platform manager' do
      get better_together.host_dashboard_path(locale:)
      expect(response).to have_http_status(:ok)
    end
  end

  describe 'authorization', :skip_host_setup do
    before { configure_host_platform }

    context 'when user cannot manage platform', :as_user do
      it 'returns forbidden' do
        get better_together.host_dashboard_path(locale:)
        expect(response).to have_http_status(:forbidden)
      end
    end
  end
end

Copilot generated this review using guidance from repository custom instructions.
module BetterTogether
class HostDashboardPolicy < ApplicationPolicy # rubocop:todo Style/Documentation
def index?
user.present? && user.permitted_to?('manage_platform')
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The policy implementation is checking user.permitted_to? directly on the user object, but this is inconsistent with the codebase pattern. According to ApplicationPolicy (lines 96-98), policies should use the protected permitted_to? method which delegates to agent (the user's person).

The current implementation will fail if user is nil, even though the policy already has a nil check. The pattern used in HubPolicy (line 6) shows the correct approach:

def index?
  permitted_to?('manage_platform')
end

This change aligns with the authorization architecture where permissions are associated with Person records, not User records directly.

Suggested change
user.present? && user.permitted_to?('manage_platform')
permitted_to?('manage_platform')

Copilot uses AI. Check for mistakes.
Comment on lines +3 to +4
# app/policies/better_together/host_dashboard_policy.rb

Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file path comment on line 3 is redundant and violates the DRY principle. The file path is already evident from the actual file location, and this type of comment is not used consistently across the codebase. It should be removed to maintain consistency with other policy files like HubPolicy, GuestAccessPolicy, etc.

Suggested change
# app/policies/better_together/host_dashboard_policy.rb

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants