Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting abilities based on user.current_team introduces bugs for multiple browser windows opened on different teams #916

Open
pascallaliberte opened this issue Sep 21, 2024 · 2 comments

Comments

@pascallaliberte
Copy link
Member

Scenario to reproduce

  • Setup a super_admin? predicate method on Team, checking an ENV SUPER_ADMIN_TEAM_ID, e.g. team id 1
  • In ability.rb, add a block if user.current_team&.super_admin? with a special can :manage, ExampleResource definition just for super_admin teams
  • Create non-super_admin team for the user
  • Create a non-super_admin-only resource
  • Make sure both super_admin-only resource and the other resource are on teams/show, with the super_admin-only resource wrapped in an <% if can? :read, ExampleResource.new(team: @team) %>
  • Open two browser windows (could be different browsers, it doesn't check for the user's session for current_team), and in each browsers, visit a different team dashboard/show page.

Expected result

Reloading each page should show only the resources boxes allowed

Actual result

Reloading each page shows the wrong resource boxes, as if the <% if can? :read... %> blocks don't work properly

Possible source of the bug

I think the source of the bug is that on each request, Ability.new(user) is called before current_team is set to the team of the page's context. The can? helper will make its determination based on the current_team stored in the latest page request, and not the current page request.

def user=(user)
super
if user
Time.zone = user.time_zone
self.ability = Ability.new(user)
else
Time.zone = nil
self.ability = nil
end
update_membership
end

@pascallaliberte
Copy link
Member Author

There's probably a way to not rely on current_team in ability.rb at all that I should know about, or a way that relies on current_team evaluation at run-time, like smarter role definitions or something.

@jagthedrummer
Copy link
Contributor

I think we ultimately need to move away from setting/capturing current_team in any way. Instead we'll need to include a teams/:id segment in any routes that need a team but for which we can't determine one based on the object ownership graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants