kevinsivic/dawarich
1
0
Fork 0
mirror of https://github.com/Freika/dawarich.git synced 2026-01-10 01:01:39 -05:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor family membership and invitation policies for clarity and security

Browse source
This commit is contained in:
Eugene Burmakin 2025-10-13 12:23:01 +02:00
parent 923ea113c8
commit 29ae5c04f1
9 changed files with 98 additions and 266 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
app/controllers/family/memberships_controller.rb
View file

@ -8,17 +8,7 @@ class Family::MembershipsController < ApplicationController
before_action :set_invitation, only: %i[create]
def create
unless @invitation.pending?
redirect_to root_path, alert: 'This invitation has already been processed' and return
end
if @invitation.expired?
redirect_to root_path, alert: 'This invitation is no longer valid or has expired' and return
end
if @invitation.email != current_user.email
redirect_to root_path, alert: 'This invitation is not for your email address' and return
end
authorize @invitation, policy_class: Family::MembershipPolicy
service = Families::AcceptInvitation.new(
invitation: @invitation,
@ -30,6 +20,16 @@ class Family::MembershipsController < ApplicationController
else
redirect_to root_path, alert: service.error_message || 'Unable to accept invitation'
end
rescue Pundit::NotAuthorizedError
if @invitation.expired?
redirect_to root_path, alert: 'This invitation is no longer valid or has expired'
elsif !@invitation.pending?
redirect_to root_path, alert: 'This invitation has already been processed'
elsif @invitation.email != current_user.email
redirect_to root_path, alert: 'This invitation is not for your email address'
else
redirect_to root_path, alert: 'You are not authorized to accept this invitation'
end
rescue StandardError => e
Rails.logger.error "Error accepting family invitation: #{e.message}"
redirect_to root_path, alert: 'An unexpected error occurred. Please try again later'

2
app/jobs/family/invitations/cleanup_job.rb
View file

@ -1,7 +1,7 @@
# frozen_string_literal: true
class Family::Invitations::CleanupJob < ApplicationJob
queue_as :family
queue_as :families
def perform
Rails.logger.info 'Starting family invitations cleanup'

5
app/policies/family/invitation_policy.rb
View file

@ -1,11 +1,6 @@
# frozen_string_literal: true
class Family::InvitationPolicy < ApplicationPolicy
def show?
# Public endpoint for invitation acceptance - no authentication required
true
end
def create?
return false unless user

23
app/policies/family/membership_policy.rb
View file

@ -1,29 +1,22 @@
# frozen_string_literal: true
class Family::MembershipPolicy < ApplicationPolicy
def show?
def create?
return false unless user
return false unless record.is_a?(Family::Invitation)
user.family == record.family
end
def update?
return false unless user
# Users can update their own settings
return true if user == record.user
# Family owners can update any member's settings
show? && user.family_owner?
# User can only accept invitations that:
# 1. Are for their email address
# 2. Are still pending
# 3. Haven't expired
record.email == user.email && record.pending? && !record.expired?
end
def destroy?
return false unless user
# Users can remove themselves (handled by family leave logic)
return true if user == record.user
# Family owners can remove other members
update?
user.family == record.family && user.family_owner?
end
end

8
app/services/families/locations.rb
View file

@ -34,17 +34,15 @@ class Families::Locations
sharing_members.map { _1.points.last }.compact
latest_points.map do |point|
next unless point
{
user_id: point.user_id,
email: point.user.email,
email_initial: point.user.email.first.upcase,
latitude: point.lat.to_f,
longitude: point.lon.to_f,
latitude: point.lat,
longitude: point.lon,
timestamp: point.timestamp.to_i,
updated_at: Time.zone.at(point.timestamp.to_i)
}
end.compact
end
end
end

25
app/services/families/memberships/destroy.rb
View file

@ -6,20 +6,18 @@ module Families
attr_reader :user, :member_to_remove, :error_message
def initialize(user:, member_to_remove: nil)
@user = user # The user performing the action (current_user)
@member_to_remove = member_to_remove || user # The user being removed (defaults to self)
@user = user
@member_to_remove = member_to_remove || user
@error_message = nil
end
def call
return false unless validate_can_leave
# Store family info before removing membership
@family_name = member_to_remove.family.name
@family_owner = member_to_remove.family.owner
ActiveRecord::Base.transaction do
handle_ownership_transfer if member_to_remove.family_owner?
remove_membership
send_notifications
end
@ -52,10 +50,8 @@ module Families
end
def validate_removal_allowed
# If removing self (user == member_to_remove)
return validate_owner_can_leave if removing_self?
# If removing another member, user must be owner and member must be in same family
return false unless validate_remover_is_owner
return false unless validate_same_family
return false unless validate_not_removing_owner
@ -95,19 +91,6 @@ module Families
false
end
def family_has_other_members?
member_to_remove.family.members.count > 1
end
def handle_ownership_transfer
# If this is the last member (owner), delete the family
return unless member_to_remove.family.members.count == 1
member_to_remove.family.destroy!
# If owner tries to leave with other members, it should be prevented in validation
end
def remove_membership
member_to_remove.family_membership.destroy!
end
@ -123,7 +106,6 @@ module Families
end
def send_self_removal_notifications
# Notify the user who left
Notification.create!(
user: member_to_remove,
kind: :info,
@ -131,7 +113,6 @@ module Families
content: "You've left the family \"#{@family_name}\""
)
# Notify the family owner
return unless @family_owner&.persisted?
Notification.create!(
@ -143,7 +124,6 @@ module Families
end
def send_member_removed_notifications
# Notify the member who was removed
Notification.create!(
user: member_to_remove,
kind: :info,
@ -151,7 +131,6 @@ module Families
content: "You have been removed from the family \"#{@family_name}\" by #{user.email}"
)
# Notify the owner who removed the member (if different from the member)
return unless user != member_to_remove
Notification.create!(

2
config/sidekiq.yml
View file

@ -5,7 +5,7 @@
- points
- default
- mailers
- family
- families
- imports
- exports
- stats

68
spec/policies/family/invitation_policy_spec.rb
View file

@ -10,36 +10,10 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
let(:invitation) { create(:family_invitation, family: family, invited_by: owner) }
before do
# Set up family membership for owner
create(:family_membership, family: family, user: owner, role: :owner)
# Set up family membership for regular member
create(:family_membership, family: family, user: member, role: :member)
end
describe '#show?' do
context 'with authenticated user' do
it 'allows any authenticated user to view invitation' do
policy = Family::InvitationPolicy.new(owner, invitation)
expect(policy).to permit(:show)
end
it 'allows other users to view invitation' do
policy = Family::InvitationPolicy.new(other_user, invitation)
expect(policy).to permit(:show)
end
end
context 'with unauthenticated user' do
it 'allows unauthenticated access (public endpoint)' do
policy = Family::InvitationPolicy.new(nil, invitation)
expect(policy).to permit(:show)
end
end
end
describe '#create?' do
context 'when user is family owner' do
before do
@ -48,7 +22,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
end
it 'allows family owner to create invitations' do
policy = Family::InvitationPolicy.new(owner, invitation)
policy = described_class.new(owner, invitation)
expect(policy).to permit(:create)
end
@ -61,7 +35,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
end
it 'denies regular family member from creating invitations' do
policy = Family::InvitationPolicy.new(member, invitation)
policy = described_class.new(member, invitation)
expect(policy).not_to permit(:create)
end
@ -69,7 +43,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'when user is not in the family' do
it 'denies user not in the family from creating invitations' do
policy = Family::InvitationPolicy.new(other_user, invitation)
policy = described_class.new(other_user, invitation)
expect(policy).not_to permit(:create)
end
@ -77,7 +51,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'with unauthenticated user' do
it 'denies unauthenticated user from creating invitations' do
policy = Family::InvitationPolicy.new(nil, invitation)
policy = described_class.new(nil, invitation)
expect(policy).not_to permit(:create)
end
@ -89,7 +63,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
let(:invited_user) { create(:user, email: invitation.email) }
it 'allows user to accept invitation sent to their email' do
policy = Family::InvitationPolicy.new(invited_user, invitation)
policy = described_class.new(invited_user, invitation)
expect(policy).to permit(:accept)
end
@ -97,7 +71,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'when user email does not match invitation email' do
it 'denies user with different email from accepting invitation' do
policy = Family::InvitationPolicy.new(other_user, invitation)
policy = described_class.new(other_user, invitation)
expect(policy).not_to permit(:accept)
end
@ -105,7 +79,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'when family owner tries to accept invitation' do
it 'denies family owner from accepting invitation sent to different email' do
policy = Family::InvitationPolicy.new(owner, invitation)
policy = described_class.new(owner, invitation)
expect(policy).not_to permit(:accept)
end
@ -113,7 +87,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'with unauthenticated user' do
it 'denies unauthenticated user from accepting invitation' do
policy = Family::InvitationPolicy.new(nil, invitation)
policy = described_class.new(nil, invitation)
expect(policy).not_to permit(:accept)
end
@ -128,7 +102,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
end
it 'allows family owner to cancel invitations' do
policy = Family::InvitationPolicy.new(owner, invitation)
policy = described_class.new(owner, invitation)
expect(policy).to permit(:destroy)
end
@ -141,7 +115,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
end
it 'denies regular family member from cancelling invitations' do
policy = Family::InvitationPolicy.new(member, invitation)
policy = described_class.new(member, invitation)
expect(policy).not_to permit(:destroy)
end
@ -149,7 +123,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'when user is not in the family' do
it 'denies user not in the family from cancelling invitations' do
policy = Family::InvitationPolicy.new(other_user, invitation)
policy = described_class.new(other_user, invitation)
expect(policy).not_to permit(:destroy)
end
@ -157,7 +131,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
context 'with unauthenticated user' do
it 'denies unauthenticated user from cancelling invitations' do
policy = Family::InvitationPolicy.new(nil, invitation)
policy = described_class.new(nil, invitation)
expect(policy).not_to permit(:destroy)
end
@ -177,13 +151,13 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
end
it 'denies owner from creating invitations for different family' do
policy = Family::InvitationPolicy.new(owner, other_invitation)
policy = described_class.new(owner, other_invitation)
expect(policy).not_to permit(:create)
end
it 'denies owner from destroying invitations for different family' do
policy = Family::InvitationPolicy.new(owner, other_invitation)
policy = described_class.new(owner, other_invitation)
expect(policy).not_to permit(:destroy)
end
@ -194,7 +168,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
let(:invited_user) { create(:user, email: expired_invitation.email) }
it 'still allows user to attempt to accept expired invitation (business logic handles expiry)' do
policy = Family::InvitationPolicy.new(invited_user, expired_invitation)
policy = described_class.new(invited_user, expired_invitation)
expect(policy).to permit(:accept)
end
@ -202,7 +176,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'allows owner to destroy expired invitation' do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
policy = Family::InvitationPolicy.new(owner, expired_invitation)
policy = described_class.new(owner, expired_invitation)
expect(policy).to permit(:destroy)
end
@ -214,7 +188,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'allows owner to destroy accepted invitation' do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
policy = Family::InvitationPolicy.new(owner, accepted_invitation)
policy = described_class.new(owner, accepted_invitation)
expect(policy).to permit(:destroy)
end
@ -226,7 +200,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'allows owner to destroy cancelled invitation' do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
policy = Family::InvitationPolicy.new(owner, cancelled_invitation)
policy = described_class.new(owner, cancelled_invitation)
expect(policy).to permit(:destroy)
end
@ -237,7 +211,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'ensures owner can both create and destroy invitations' do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
policy = Family::InvitationPolicy.new(owner, invitation)
policy = described_class.new(owner, invitation)
expect(policy).to permit(:create)
expect(policy).to permit(:destroy)
@ -246,7 +220,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'ensures regular members cannot create or destroy invitations' do
allow(member).to receive(:family).and_return(family)
allow(member).to receive(:family_owner?).and_return(false)
policy = Family::InvitationPolicy.new(member, invitation)
policy = described_class.new(member, invitation)
expect(policy).not_to permit(:create)
expect(policy).not_to permit(:destroy)
@ -254,7 +228,7 @@ RSpec.describe Family::InvitationPolicy, type: :policy do
it 'ensures invited users can only accept their own invitations' do
invited_user = create(:user, email: invitation.email)
policy = Family::InvitationPolicy.new(invited_user, invitation)
policy = described_class.new(invited_user, invitation)
expect(policy).to permit(:accept)
expect(policy).not_to permit(:create)

209
spec/policies/family/membership_policy_spec.rb
View file

@ -13,126 +13,49 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
let(:member_membership) { create(:family_membership, family: family, user: member) }
let(:another_member_membership) { create(:family_membership, family: family, user: another_member) }
describe '#show?' do
context 'when user is in the same family' do
before do
allow(owner).to receive(:family).and_return(family)
end
describe '#create?' do
let(:valid_invitation) { create(:family_invitation, family: family, email: member.email) }
let(:expired_invitation) { create(:family_invitation, family: family, email: member.email, expires_at: 1.day.ago) }
let(:accepted_invitation) { create(:family_invitation, :accepted, family: family, email: member.email) }
let(:wrong_email_invitation) { create(:family_invitation, family: family, email: 'wrong@example.com') }
it 'allows family owner to view member details' do
policy = Family::MembershipPolicy.new(owner, member_membership)
context 'when user has valid invitation' do
it 'allows user to create membership with valid pending invitation for their email' do
policy = described_class.new(member, valid_invitation)
expect(policy).to permit(:show)
end
it 'allows family owner to view their own membership' do
policy = Family::MembershipPolicy.new(owner, owner_membership)
expect(policy).to permit(:show)
end
it 'allows regular member to view other members' do
allow(member).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(member, another_member_membership)
expect(policy).to permit(:show)
end
it 'allows member to view their own membership' do
allow(member).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(member, member_membership)
expect(policy).to permit(:show)
expect(policy).to permit(:create)
end
end
context 'when user is not in the same family' do
it 'denies user from different family from viewing membership' do
policy = Family::MembershipPolicy.new(other_user, member_membership)
context 'when invitation is expired' do
it 'denies user from creating membership with expired invitation' do
policy = described_class.new(member, expired_invitation)
expect(policy).not_to permit(:show)
expect(policy).not_to permit(:create)
end
end
context 'when invitation is already accepted' do
it 'denies user from creating membership with already accepted invitation' do
policy = described_class.new(member, accepted_invitation)
expect(policy).not_to permit(:create)
end
end
context 'when invitation is for different email' do
it 'denies user from creating membership with invitation for different email' do
policy = described_class.new(member, wrong_email_invitation)
expect(policy).not_to permit(:create)
end
end
context 'with unauthenticated user' do
it 'denies unauthenticated user from viewing membership' do
policy = Family::MembershipPolicy.new(nil, member_membership)
it 'denies unauthenticated user from creating membership' do
policy = described_class.new(nil, valid_invitation)
expect(policy).not_to permit(:show)
end
end
end
describe '#update?' do
context 'when user is updating their own membership' do
it 'allows user to update their own membership settings' do
allow(member).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(member, member_membership)
expect(policy).to permit(:update)
end
it 'allows owner to update their own membership' do
allow(owner).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(owner, owner_membership)
expect(policy).to permit(:update)
end
end
context 'when user is family owner' do
before do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
end
it 'allows family owner to update other members settings' do
policy = Family::MembershipPolicy.new(owner, member_membership)
expect(policy).to permit(:update)
end
it 'allows family owner to update multiple members' do
policy1 = Family::MembershipPolicy.new(owner, member_membership)
policy2 = Family::MembershipPolicy.new(owner, another_member_membership)
expect(policy1).to permit(:update)
expect(policy2).to permit(:update)
end
end
context 'when user is regular family member' do
before do
allow(member).to receive(:family).and_return(family)
allow(member).to receive(:family_owner?).and_return(false)
end
it 'denies regular member from updating other members settings' do
policy = Family::MembershipPolicy.new(member, another_member_membership)
expect(policy).not_to permit(:update)
end
it 'denies regular member from updating owner settings' do
policy = Family::MembershipPolicy.new(member, owner_membership)
expect(policy).not_to permit(:update)
end
end
context 'when user is not in the family' do
it 'denies user from updating membership of different family' do
policy = Family::MembershipPolicy.new(other_user, member_membership)
expect(policy).not_to permit(:update)
end
end
context 'with unauthenticated user' do
it 'denies unauthenticated user from updating membership' do
policy = Family::MembershipPolicy.new(nil, member_membership)
expect(policy).not_to permit(:update)
expect(policy).not_to permit(:create)
end
end
end
@ -141,14 +64,14 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
context 'when user is removing themselves' do
it 'allows user to remove their own membership (leave family)' do
allow(member).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(member, member_membership)
policy = described_class.new(member, member_membership)
expect(policy).to permit(:destroy)
end
it 'allows owner to remove their own membership' do
allow(owner).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(owner, owner_membership)
policy = described_class.new(owner, owner_membership)
expect(policy).to permit(:destroy)
end
@ -161,14 +84,14 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
end
it 'allows family owner to remove other members' do
policy = Family::MembershipPolicy.new(owner, member_membership)
policy = described_class.new(owner, member_membership)
expect(policy).to permit(:destroy)
end
it 'allows family owner to remove multiple members' do
policy1 = Family::MembershipPolicy.new(owner, member_membership)
policy2 = Family::MembershipPolicy.new(owner, another_member_membership)
policy1 = described_class.new(owner, member_membership)
policy2 = described_class.new(owner, another_member_membership)
expect(policy1).to permit(:destroy)
expect(policy2).to permit(:destroy)
@ -182,13 +105,13 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
end
it 'denies regular member from removing other members' do
policy = Family::MembershipPolicy.new(member, another_member_membership)
policy = described_class.new(member, another_member_membership)
expect(policy).not_to permit(:destroy)
end
it 'denies regular member from removing owner' do
policy = Family::MembershipPolicy.new(member, owner_membership)
policy = described_class.new(member, owner_membership)
expect(policy).not_to permit(:destroy)
end
@ -196,7 +119,7 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
context 'when user is not in the family' do
it 'denies user from removing membership of different family' do
policy = Family::MembershipPolicy.new(other_user, member_membership)
policy = described_class.new(other_user, member_membership)
expect(policy).not_to permit(:destroy)
end
@ -204,7 +127,7 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
context 'with unauthenticated user' do
it 'denies unauthenticated user from removing membership' do
policy = Family::MembershipPolicy.new(nil, member_membership)
policy = described_class.new(nil, member_membership)
expect(policy).not_to permit(:destroy)
end
@ -224,20 +147,8 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
allow(owner).to receive(:family_owner?).and_return(true)
end
it 'denies owner from viewing membership of different family' do
policy = Family::MembershipPolicy.new(owner, other_family_membership)
expect(policy).not_to permit(:show)
end
it 'denies owner from updating membership of different family' do
policy = Family::MembershipPolicy.new(owner, other_family_membership)
expect(policy).not_to permit(:update)
end
it 'denies owner from destroying membership of different family' do
policy = Family::MembershipPolicy.new(owner, other_family_membership)
policy = described_class.new(owner, other_family_membership)
expect(policy).not_to permit(:destroy)
end
@ -252,20 +163,8 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
allow(owner).to receive(:family_owner?).and_return(true)
end
it 'allows owner to view another owner' do
policy = Family::MembershipPolicy.new(owner, co_owner_membership)
expect(policy).to permit(:show)
end
it 'allows owner to update another owner (family owner has full control)' do
policy = Family::MembershipPolicy.new(owner, co_owner_membership)
expect(policy).to permit(:update)
end
it 'allows owner to remove another owner (family owner has full control)' do
policy = Family::MembershipPolicy.new(owner, co_owner_membership)
policy = described_class.new(owner, co_owner_membership)
expect(policy).to permit(:destroy)
end
@ -273,38 +172,32 @@ RSpec.describe Family::MembershipPolicy, type: :policy do
end
describe 'authorization consistency' do
it 'ensures owner can view, update, and destroy all memberships in their family' do
it 'ensures owner can destroy all memberships in their family' do
allow(owner).to receive(:family).and_return(family)
allow(owner).to receive(:family_owner?).and_return(true)
policy = Family::MembershipPolicy.new(owner, member_membership)
policy = described_class.new(owner, member_membership)
expect(policy).to permit(:show)
expect(policy).to permit(:update)
expect(policy).to permit(:destroy)
end
it 'ensures regular members can only manage their own membership' do
it 'ensures regular members can only remove their own membership' do
allow(member).to receive(:family).and_return(family)
allow(member).to receive(:family_owner?).and_return(false)
own_policy = Family::MembershipPolicy.new(member, member_membership)
other_policy = Family::MembershipPolicy.new(member, another_member_membership)
own_policy = described_class.new(member, member_membership)
other_policy = described_class.new(member, another_member_membership)
# Can manage own membership
expect(own_policy).to permit(:show)
expect(own_policy).to permit(:update)
# Can remove own membership
expect(own_policy).to permit(:destroy)
# Can view but not manage others
expect(other_policy).to permit(:show)
expect(other_policy).not_to permit(:update)
# Cannot remove others
expect(other_policy).not_to permit(:destroy)
end
it 'ensures users can always leave the family (remove own membership)' do
allow(member).to receive(:family).and_return(family)
policy = Family::MembershipPolicy.new(member, member_membership)
policy = described_class.new(member, member_membership)
expect(policy).to permit(:destroy)
end