search

RedHatInsights / insights-api-common-rails

Header, Encryption, RBAC, Serialization, Pagination and other common behavior for Insights microservices built with Rails
Apache License 2.0
3 stars 25 forks source link

Use custom error_message if a policy exposes it #214

Closed eclarizio closed 3 years ago

eclarizio commented 3 years ago

When we check requirements for actions, we generally check against the parent Portfolio if the user has the permission to update the Portfolio. This happens for example when creating portfolio items or when editing surveys, resetting service plans, and various other actions for portfolio items and service plans.

However, because we're checking against a Portfolio, the way Pundit handles it if the check fails is that the record stored in the exception is now Portfolio, which ends up generating bad messages because the user will see "You are not authorized to action for this Portfolio" when they were trying to create/edit either a portfolio item or a service plan. There's not a really great way that I could find to directly override the exception.record without monkeypatching Pundit itself, and that felt pretty gross.

This change is very minimal to the common repo in favor of pushing the responsibility of setting a correct error message on the policy side. If the policy wishes to have a custom message, they simply set the instance variable in the method where they would like the custom message, and this should handle it. If not, we will default to the default message, although I did change the wording a little bit to better reflect the changes I made to the policy error messages.

For more info, Please see the catalog-api PR here: https://github.com/RedHatInsights/catalog-api/pull/883 And the JIRA story here: https://issues.redhat.com/browse/SSP-1977

@lindgrenj6 Please review, or tag someone who you should think review. Thanks!

eclarizio commented 3 years ago

@brahmanim Tagging you here as well Benny because once this goes through the error message language is slightly changed, so want to make sure you are aware.

dippy-bot commented 3 years ago

Checked commit https://github.com/eclarizio/manageiq-api-common/commit/7b8bdf3430c4b4af92a2fdf58cf9e16dea2eb913 with ruby 2.5.7, rubocop 0.82.0, haml-lint 0.35.0, and yamllint 4 files checked, 0 offenses detected Everything looks fine. :cookie: