FIX: handle nil topic value when removing allowed users

FIX: handle nil topic value when removing allowed users

From 5951e111ad9a4ae5299017cf2442673b0f23642f Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Tue, 20 Nov 2018 22:54:39 +0530
Subject: [PATCH] FIX: handle nil topic value when removing allowed users


diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 002f2aa..6bc4fa4 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -503,7 +503,7 @@ class TopicsController < ApplicationController
     user = User.find_by(username: params[:username])
     guardian.ensure_can_remove_allowed_users!(topic, user)
 
-    if topic.remove_allowed_user(current_user, user)
+    if topic&.remove_allowed_user(current_user, user)
       render json: success_json
     else
       render json: failed_json, status: 422

GitHub

This is not right. If the topic is not found we should return an error with failed json, not eat it.

It isn’t eating the error here :grin: If topic is nil, topic&.remove_allowed_user will evaluate to nil and return a 422. However, we should be checking if the topic is present or not and return raise Discourse::NotFound unless topic like how we do for other controllers.

3 Likes

Agreed. Done in

2 Likes