Skip to content

Commit 94e5952

Browse files
committed
Enforce admin export-all server-side and de-flake projects spec
- Only honor export_all when current_user is an admin, so the param can't be forged by a non-admin to export non-approved stories (the view-only guard was not enough). Add a non-admin guard spec and sign in as an admin in the admin spec, which previously never actually authenticated as one. - Simplify export: drop the 4-way branch and the unused export_all argument, and move generate_csv under private. - Fix a flaky madmin projects spec by comparing against the HTML-escaped title (Faker names with apostrophes were escaped in the page and failed the match).
1 parent 41b05e4 commit 94e5952

2 files changed

Lines changed: 49 additions & 31 deletions

File tree

app/controllers/stories_controller.rb

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -87,38 +87,19 @@ def import
8787
end
8888

8989
def export
90-
csv = if params[:export_with_comments] == "1" && params[:export_all] == "1"
91-
generate_csv(@project.stories.includes(:comments), with_comments: true, export_all: true)
92-
elsif params[:export_with_comments] == "1"
93-
generate_csv(@project.stories.includes(:comments).approved, with_comments: true, export_all: false)
94-
elsif params[:export_all] == "1"
95-
generate_csv(@project.stories, with_comments: false, export_all: true)
96-
else
97-
generate_csv(@project.stories.approved, with_comments: false, export_all: false)
98-
end
90+
with_comments = params[:export_with_comments] == "1"
91+
# Only admins may export non-approved stories. Enforce it here rather than
92+
# relying on the checkbox being hidden in the view, so the param can't be
93+
# forged by a non-admin.
94+
export_all = params[:export_all] == "1" && current_user.admin?
9995

100-
filename = "#{@project.title.gsub(/[^\w]/, "_")}-#{Time.now.to_formatted_s(:short).tr(" ", "_")}.csv"
101-
send_data csv, filename: filename
102-
end
96+
stories = export_all ? @project.stories : @project.stories.approved
97+
stories = stories.includes(:comments) if with_comments
10398

104-
def generate_csv(stories, with_comments: false, export_all: false)
105-
CSV.generate(headers: true) do |csv|
106-
headers = CSV_HEADERS.dup
107-
headers << "comment" if with_comments
108-
csv << headers
99+
csv = generate_csv(stories, with_comments: with_comments)
109100

110-
stories.by_position.each do |story|
111-
comments = []
112-
113-
if with_comments
114-
comments = story.comments.map do |comment|
115-
"#{display_name(comment.user)}: #{comment.body}"
116-
end
117-
end
118-
119-
csv << [story.id, story.title, story.description, story.position] + comments
120-
end
121-
end
101+
filename = "#{@project.title.gsub(/[^\w]/, "_")}-#{Time.now.to_formatted_s(:short).tr(" ", "_")}.csv"
102+
send_data csv, filename: filename
122103
end
123104

124105
def render_markdown
@@ -161,6 +142,26 @@ def pending
161142

162143
private
163144

145+
def generate_csv(stories, with_comments: false)
146+
CSV.generate(headers: true) do |csv|
147+
headers = CSV_HEADERS.dup
148+
headers << "comment" if with_comments
149+
csv << headers
150+
151+
stories.by_position.each do |story|
152+
comments = []
153+
154+
if with_comments
155+
comments = story.comments.map do |comment|
156+
"#{display_name(comment.user)}: #{comment.body}"
157+
end
158+
end
159+
160+
csv << [story.id, story.title, story.description, story.position] + comments
161+
end
162+
end
163+
end
164+
164165
def find_project
165166
@project = Project.find(params[:project_id])
166167
end

spec/controllers/stories_controller_spec.rb

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,7 @@
189189

190190
context "when an admin" do
191191
it "exports a CSV file with all stories" do
192-
user = FactoryBot.create(:user)
193-
user.admin = true
192+
sign_in FactoryBot.create(:user, :admin)
194193

195194
story2 = FactoryBot.create(:story, project: project, status: :rejected)
196195
story3 = FactoryBot.create(:story, project: project, status: :pending)
@@ -208,6 +207,24 @@
208207
end
209208
end
210209

210+
context "when not an admin" do
211+
it "ignores export_all and exports only approved stories" do
212+
# The signed-in user from the before block is not an admin, so a
213+
# forged export_all param must not leak non-approved stories.
214+
FactoryBot.create(:story, project: project, status: :rejected)
215+
FactoryBot.create(:story, project: project, status: :pending)
216+
get :export, params: {project_id: project.id, export_all: "1"}
217+
expect(response).to have_http_status(:ok)
218+
219+
csv_data = CSV.parse(response.body)
220+
expected_csv_content = [
221+
["id", "title", "description", "position"],
222+
[story.id.to_s, story.title, story.description, story.position.to_s]
223+
]
224+
expect(csv_data).to eq(expected_csv_content)
225+
end
226+
end
227+
211228
context "with comments" do
212229
it "exports a CSV file with only approved stories" do
213230
user = FactoryBot.create(:user)

0 commit comments

Comments
 (0)