Skip to content

Fix 500 error when browsing the roadmap page for a group the user is not authorized to view

Manoj M J requested to merge 199473-nomethoderror-undefined-method-feature_available-for-nil-nilclass into master

What does this MR do?

Fixes #199473 (closed)

Sentry: https://sentry.gitlab.net/gitlab/gitlabcom/issues/1199982/

Reason:

This code

  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_filters.

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

Availability and Testing

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
Edited by Manoj M J

Merge request reports