Perf: Refactor to check user role in memory not db

[coalest]

Doesn't resolve any issue

Description

Since we have multiple current_user.has_role? calls in many controllers/views, let's use Rolify's has_cached_role? instead as this performs the authorization check with user's roles in memory rather than going to the database every single time.

The difference is that has_role? performs a query everytime, while has_cached_role? loads the roles on the user object and performs the check without any queries.

The documentation for this method has a warning:

This method should be used with caution. If you don't preload the roles, the has_cached_role? might return false. In the above example, it would return false for @user.has_cached_role?(:member, Forum), because User.with_role(:admin, Forum) will load only the :admin roles.

I was not able to replicate that behavior personally (like mentioned in this issue). Also with this change we are fetching all roles right after the user is set regardless.

I know that "caching" authorization checks sounds like a bad idea. But it is only "cached" for the length of the request, so I think it's fine. And it would help with performance across the app.

An example is the distributions index page, which went from 6 database queries regarding roles down to 2 (one loading all the roles and one when current_role is called to fetch the first role).

If this approach is rejected, I would propose we set up our own way of caching/storing these rolify checks in views. Because some pages trigger a lot of these queries.

Type of change

• Refactor (performance)

How Has This Been Tested?

Locally and by running the test suite.

[coalest]

I can hide this change behind a feature flag if we want to extra careful.

[cielf]

Re: feature flag. We mostly use feature flags for multi-stage changes (like the current 'packs' milestone, or things we would initially release to limited users for beta.

I feel that this is a change that either we are very confident in or we don't put forward to production at all.

It would provide rollback-ability, of course.

[dorner]

I'm OK with this change. All tests are passing and from what I can read, there don't seem to be any reproductions of the original claim of flakiness (which was added when the feature first was, almost 10 years ago.

I'm good with merging and we can see if anyone complains of issues. @cielf thoughts?

[cielf]

A question: If this were to go awry, do we know what the expression would be? Banks don't always complain.

and

My knee-jerk response is that I'm a bit iffy about putting it in so close to high new-banks-who-are-using-the-system-for-real season, just because of the higher support load.

But/and my knee-jerk response has always been a bit over-cautious.

[coalest]

Hard to say how it would fail, because it doesn't look like it should.

But if it did fail, it would be bad because it touches both authorization and authentication. So potentially users getting signed out, or not being able to view pages they should have access to. I imagine it would be noticeable in logs though. I would expect a higher number of 302 responses (redirecting users to different pages if they don't have access or redirecting to the sign in page if they don't pass authentication).

So I understand the cautiousness. We can either wait till later or put it behind a feature flag so that we could quickly turn it off.

[cielf]

Hmm... so this might interact with #4511.

[cielf]

What's with the gazillion commits on this one too?