Prevent alerts to be sent to users who don't have access to the related service

[thalesmiguel]

What this PR does / why we need it:

This PR updates how we grant the permission to :show AlertRelatedEvents. Right now, any provider on an account has this permission, even though they might not have permissions for a service the alert refers to. With this PR, we're adding a check on service level to prevent the issue reported on the linked ticket.

Which issue(s) this PR fixes

fixes THREESCALE-6318

Verification steps

Verification steps copied from the linked ticket:

1. As Admin user, create a Product i.e. Test API, an application and application plan. Set a limit of 2 requests per minute.
2. Create a member i.e “Tom” user with the following rights:

    Accounts – Applications
    Analytics
    Integration & Application Plans
    Disable “All current and future APIs” and check 1 APi, i.e. Test API. 

3. Sing in at the admin portal as Tom user and enable the following:
   • Go to the Gear icon - Settings > Notification Preferences > Usage Alerts. Check both “Alert usage warning” and “Alert usage violation”. Then Click Update Notificaton Preferences. See screenshot Notification_preferences.png.
   • Go to Product Cars > Applications > Settings - Usage Rules > Alerts, check both “Show Web Alerts to Admins of this Account” and “Send Email Alerts to Admins of this Account”, from 50% to 100%, see screenshot Alerts.png. Both alerts are necessary due to this issue THREESCALE-1870.
4. Repeat the same steps 2 & 3 for another user, i.e. Marta, however don't allow access to Test API but to another API. See screenshot Marta.png.
5. Make a request to Test API until hit the limit. Check the emails and you’ll see that Marta has received an alert email regarding Test Api which hasn’t access.

Special notes for your reviewer:

To test this, I ended-up having to run Events.async_fetch_backend_events! on a Rails console to force System to fetch the alert event from Apisonator. This might be happening due to a misconfiguration on my local environment but, just for the sake of sharing, I wanted to add this note to the PR.

[mayorova]

Looks good and works as expected, but I am wondering - would it make sense to move the check to provider_member.rb instead, as provider admin (not member) by definition has access to all services.

[thalesmiguel]

would it make sense to move the check to provider_member.rb instead, as provider admin (not member) by definition has access to all services.

Good question, @mayorova.
This is the only place where we set permissions for events, so it's not clear to me if we could move all of them to be set on provider_member.rb.
By doing so, we'd also need to set those permissions on provider_admin.rb, but without checking for services, so the difference between those roles is clear.
Looks like things work correctly now because has_access_to_service? is able to determinate correctly which services a member or admin has access to 🤔
From what I can gather looking at the current code, I think it makes sense to try and set this permission for any provider, regardless of the role but I might be missing something...
All that to say that I'm not 100% sure one option makes more sense over the other. As a guideline, I try to have permissions set in a less places as possible but, of course, sometimes it makes sense to have it spread.

[mayorova]

Yep, I am not sure whether there would be any benefit in splitting it between the two files (_member and _admin). So let's keep it where it is.

[akostadinov]

Looks nice!

I have one concern though. Do our users have the use case of a monitoring user that is not allowed to use the APIs? Because such use cases would be broken.

I'm not saying it makes sense. Just asking.

[thalesmiguel]

I have one concern though. Do our users have the use case of a monitoring user that is not allowed to use the APIs? Because such use cases would be broken.

This is interesting @akostadinov and MAYBE 3Scale supports that already.
I think this could be accomplished by saying a member has Access & query analytics of All current and future existing API products (or a set of products) 🤔

If it's something used at all, I guess Product would be able to answer us.

[mayorova]

LGTM 👍

[josemigallas]

I only request the commits are rebased properly as discussed

[thalesmiguel]

I only request the commits are rebased properly as discussed

@josemigallas My plan was actually to Squash and merge this PR, as I've been doing. Is it really a blocker for this PR not to have the 2 commits as suggested?

[thalesmiguel]

Force pushed in order to have 2 commits: One for the fix and another one for the Rubocop warnings, as per #3134 (comment)