Improve team admin permissions

We missed a few permission checks, like anybody was able to see the team
list, and any team member was able to add new team members instead of
just admins.

The commit also adds basic rspecs to make sure the feature work.

Author: dutow

app/controllers/team_members_controller.rb

@@ -6,6 +6,7 @@ class TeamMembersController < ApplicationController
 
   def create
     authorize_invite!
+    return if performed?
     username = params[:username].to_s.strip
     user = User.find_by(username: username)
     return redirect_to @team, alert: "User not found" unless user
@@ -24,6 +25,7 @@ def destroy
       redirect_to teams_path, notice: "You left the team"
     else
       authorize_admin!
+      return if performed?
       if @team.last_admin?(membership)
         redirect_to @team, alert: "Cannot remove the last admin"
       else
@@ -40,12 +42,14 @@ def set_team
   end
 
   def authorize_invite!
-    return if @team.admin?(current_user) || @team.member?(current_user)
-    redirect_to @team, alert: "Only team members can add users"
+    return if @team.admin?(current_user)
+    redirect_to @team, alert: "Admins only"
+    return
   end
 
   def authorize_admin!
     return if @team.admin?(current_user)
     redirect_to @team, alert: "Admins only"
+    return
   end
 end

app/controllers/teams_controller.rb

@@ -3,6 +3,7 @@
 class TeamsController < ApplicationController
   before_action :require_authentication, except: [:show, :index]
   before_action :set_team, only: [:show, :destroy]
+  before_action :require_team_member!, only: [:show]
   before_action :require_team_admin!, only: [:destroy]
 
   def index
@@ -47,4 +48,13 @@ def require_team_admin!
       redirect_to @team, alert: "Admins only" and return
     end
   end
+
+  def require_team_member!
+    unless user_signed_in?
+      redirect_to new_session_path, alert: "Please sign in"
+      return
+    end
+
+    render_404 unless @team.member?(current_user)
+  end
 end

spec/requests/team_members_spec.rb

@@ -0,0 +1,49 @@
+require "rails_helper"
+
+RSpec.describe "TeamMembers", type: :request do
+  def sign_in(email:, password: "secret")
+    post session_path, params: { email: email, password: password }
+    expect(response).to redirect_to(root_path)
+  end
+
+  def attach_verified_alias(user, email:, primary: true)
+    al = create(:alias, user: user, email: email)
+    if primary && user.person&.default_alias_id.nil?
+      user.person.update!(default_alias_id: al.id)
+    end
+    Alias.by_email(email).update_all(verified_at: Time.current)
+    al
+  end
+
+  let!(:team) { create(:team) }
+  let!(:admin) { create(:user, password: "secret", password_confirmation: "secret") }
+  let!(:member) { create(:user, password: "secret", password_confirmation: "secret") }
+  let!(:invitee) { create(:user, username: "invitee") }
+
+  before do
+    create(:team_member, team: team, user: admin, role: "admin")
+    create(:team_member, team: team, user: member, role: "member")
+  end
+
+  describe "POST /teams/:team_id/team_members" do
+    it "blocks non-admins from adding members" do
+      attach_verified_alias(member, email: "member@example.com")
+      sign_in(email: "member@example.com")
+
+      expect {
+        post team_team_members_path(team), params: { username: "invitee" }
+      }.not_to change(TeamMember, :count)
+      expect(response).to redirect_to(team_path(team))
+    end
+
+    it "allows admins to add members" do
+      attach_verified_alias(admin, email: "admin@example.com")
+      sign_in(email: "admin@example.com")
+
+      expect {
+        post team_team_members_path(team), params: { username: "invitee" }
+      }.to change(TeamMember, :count).by(1)
+      expect(response).to redirect_to(team_path(team))
+    end
+  end
+end

spec/requests/teams_spec.rb

@@ -0,0 +1,77 @@
+require "rails_helper"
+
+RSpec.describe "Teams", type: :request do
+  def sign_in(email:, password: "secret")
+    post session_path, params: { email: email, password: password }
+    expect(response).to redirect_to(root_path)
+  end
+
+  def attach_verified_alias(user, email:, primary: true)
+    al = create(:alias, user: user, email: email)
+    if primary && user.person&.default_alias_id.nil?
+      user.person.update!(default_alias_id: al.id)
+    end
+    Alias.by_email(email).update_all(verified_at: Time.current)
+    al
+  end
+
+  describe "GET /teams/:id" do
+    let!(:team) { create(:team) }
+    let!(:member) { create(:user, password: "secret", password_confirmation: "secret") }
+    let!(:non_member) { create(:user, password: "secret", password_confirmation: "secret") }
+
+    before do
+      create(:team_member, team: team, user: member)
+    end
+
+    it "redirects guests to sign in" do
+      get team_path(team)
+      expect(response).to redirect_to(new_session_path)
+    end
+
+    it "returns 404 for signed-in non-members" do
+      attach_verified_alias(non_member, email: "non-member@example.com")
+      sign_in(email: "non-member@example.com")
+
+      get team_path(team)
+      expect(response).to have_http_status(:not_found)
+    end
+
+    it "allows signed-in team members" do
+      attach_verified_alias(member, email: "member@example.com")
+      sign_in(email: "member@example.com")
+
+      get team_path(team)
+      expect(response).to have_http_status(:success)
+    end
+  end
+
+  describe "DELETE /teams/:id" do
+    let!(:team) { create(:team) }
+    let!(:admin) { create(:user, password: "secret", password_confirmation: "secret") }
+    let!(:member) { create(:user, password: "secret", password_confirmation: "secret") }
+
+    before do
+      create(:team_member, team: team, user: admin, role: "admin")
+      create(:team_member, team: team, user: member, role: "member")
+    end
+
+    it "blocks non-admins from deleting teams" do
+      attach_verified_alias(member, email: "member@example.com")
+      sign_in(email: "member@example.com")
+
+      delete team_path(team)
+      expect(response).to redirect_to(team_path(team))
+      expect(Team.exists?(team.id)).to be(true)
+    end
+
+    it "allows admins to delete teams" do
+      attach_verified_alias(admin, email: "admin@example.com")
+      sign_in(email: "admin@example.com")
+
+      expect {
+        delete team_path(team)
+      }.to change(Team, :count).by(-1)
+    end
+  end
+end