Fix 500 error when browsing the roadmap page for a group the user is not authorized to view
What does this MR do?
Fixes #199473 (closed)
Sentry: https://sentry.gitlab.net/gitlab/gitlabcom/issues/1199982/
Reason:
def group
@group ||= find_routable!(Group, params[:group_id] || params[:id])
end
behaves differently when called as before_action :group
and just as group
. In fact, this method has been designed to act like a before filter that issues 404
if the group is not found, or if the user does not have access to the group.
See find_routable!
However, in our current scenario, before_action :group
is called only after before_action :check_epics_available!
and then
check_epics_available!
calls group.feature_available?(:epics)
.
During the execution of group.feature_available?(:epics)
, group
calls find_routable!
and it is not able to find a group and it tries to issue a 404. But since this call is not performed from a before_action
, render
-ing of any sort will not be done, find_routable!
simply returns nil
and nil.feature_available?(:epics)
causes the 500 error that we are currently seeing.
Fix:
group
should be called from a before_action
only (the very first time, at least) and we need this to be called as the first thing in the execution chain of before_filter
s.
Groups::ApplicationController
already does this for us and hence this same line of code can be removed from our controller.
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team