Track why authorization was skipped, for later use in verify_authorized

[madleech]

For our use case, it would be helpful to track why authorization was skipped, so that we can propagate that reason through to e.g. logs or custom metrics. This PR extends skip_authorization to allow specifying a reason, e.g. skip_authorization :api_key, which we can then handle in a custom verify_authorized method.

Hopefully this strikes a balance between adding a useful capability, versus adding a bunch of extra variables and methods that no one will use.

e.g.

class TestController < ActionController::Base
  def index
    skip_authorization :api_key
    ...
  end

  after_action def verify_authorized
   logger.info("Authorization skipped, reason = #{@_pundit_policy_authorized}") if @_pundit_policy_authorized.is_a?(Symbol)
  end
end

[ioquatix]

@Burgestrand @dgmstuart do you mind reviewing this and if it's acceptable merge it? Thanks!

[dgmstuart]

@madleech I'm afraid I don't understand why this would be useful. Can you expand on your use case? Is skip_authorization called conditionally, or every time? If it's called every time, then the fact that authorization is skipped is a hard-coded property of that action, right? Every time that action is called then authorization is skipped, so why is it useful to log that?

In any case it seems like you could implement this in your application by defining a helper function something like this?

# app/controllers/application_controller.rb

def skip_authorization_with_reason(reason)
  skip_authorization
  logger.info("Authorization skipped, reason = #{reason}")
end

I also noticed that in your example code you're overriding the verify_authorized method? That would make the feature which checks whether authorize was called not work, right? What's going on there?

[ioquatix]

This is mostly an ergonomic change, not a semantic change.

The use case is explicitly and declaratively capturing why authorisation was skipped, so that we can systematically capture it as a metric, across a system which has potentially 100s of such calls, rather than having to provide a wrapper like you suggested which requires us to do a bespoke implementation in every system which we want to support like this. i.e. it feels like the right place for this to be is here in this gem, rather than a custom wrapper in the app code (which is the monkey patch we are currently using). It's a minor change but allows us to capture the reason WHY it was appropriate to skip authorization.

why is it useful to log that?

We want to ensure our code bases follows secure conventions, and as such we need to do both static and run-time analysis of whether code is doing the right thing. Capturing metrics is part of this, and the verify_authorized hook is another mechanism by which we capture data about the running system. We can track over time what kind of authorisations are being performed and skipped, and grouping authorisations that are being explicitly skipped by the functional area gives us better insight into the system as a whole and to what extent we are skipping auth for various reasons. A more specific example would be the api_key example, which means auth was validated by an external api key. We actually want to eventually remove this approach, so using a metric for this allows us to track progress.

Ultimately, we can stick with the monkey patch, but this seems like a good, compatible extension to the existing library, and it's probably better here than a monkey patch.

[dgmstuart]

@ioquatix Thanks for the detailed response.

The thing I'm still not understanding is that, based on what you're describing, this is a property that you could understand purely with static code analysis: eg. by counting the number of skip_authorization calls in the code?

Skipping authorisation is something that is done when code is written, not when it's executed at runtime. It's something which is declared by a developer, not something which eg. a user decides to do.

When authorization fails, that's an event, but authorization being skipped is not an event that happens: it's a property of the code.

Either way it feels like an extremely niche use case, in which case it's not something we'd be keen to maintain. Our feeling is that Pundit already has a few too many features, and we're extremely cautious about adding new ones.

Regarding the monkey patch, I would recommend that you use a helper function as I described above - there shouldn't be any need to make a monkey patch based on internal implementation details of Pundit (which could potentially change in future).

[dgmstuart]

rather than having to provide a wrapper like you suggested which requires us to do a bespoke implementation in every system which we want to support like this

@ioquatix If you have a lot of projects that you want to share this behaviour between, one approach would be to create your own gem which depends on pundit and adds a skip_authorization_with_reason function. That way you get your extra functionality, but you (the people who understand the use case best) would be responsible for maintaining it rather than us.

[ioquatix]

@dgmstuart what do you see as the maintenance cost here?

Yes, we are happy to fork pundit for internal use but we'd prefer to contribute to the open source gem so everyone benefits. We have other things we'd like to contribute too :)

[dgmstuart]

Yes, we are happy to fork pundit for internal use.

@ioquatix that was not my suggestion: I'm suggesting that you create a new gem which depends on pundit and includes this feature.

[Burgestrand]

Hi again! :D

I remember https://github.com/varvet/pundit/pull/679 and this PR really feels like it's in the same spirit (same with #726), so I believe I understand where it's coming from.

I don't really know how to ask my next question without it coming off as negative, so I hope the following is not read as such! I probably have a bias towards merging PRs rather than not, so even though I'm probably in the camp of "it's not that bad" I feel like I shouldn't be; and so I also would really like to know more.

Could you help me understand why this change is better in Pundit rather than any of the following examples?

Number 1, logging right away?

class ApplicationController < ActionController::Base
  def skip_authorization(reason = "N/A")
    super()
    logger.info("Authorization skipped, reason = #{reason}")
  end
end

class AdminController < ApplicationController
  def index
    skip_authorization :api_key
  end
end

… or if you really want to log after the action:

class ApplicationController < ActionController::Base
  def skip_authorization(reason = "N/A")
    super()
    @authorization_skipped_reason = reason
  end

  after_action def log_authorization_skip
    reason = @authorization_skipped_reason || "no reason given"
    logger.info("Authorization skipped, reason = #{reason}")
  end
end

[ioquatix]

Thanks for your continued support and discussion.

Yesterday we worked on a PR to introduce:

1. The monkey patch / method in ApplicationController like what you suggested.
2. Changes across the code base which utilise this feature.

We use code owners and as you can imagine, this PR required about 10 reviews from different teams because it touched a lot of different methods. I finally got one person who urgently contacted me as one of his team approved the change but it was actually incompatible with what they were doing. They are extracting some part of our application into a separate service and have an automated code sync copying controller code including the skip_authorization :reason. Because we had to do this using a monkey patch, obviously that wouldn't work in the other app if it didn't also have the monkey patch. So the immediate solution was just to revert those particular changes.

So, from my point of view, this concern of mine was no longer a hypothetical. Someone in another project is using pundit and the interface is a monkey patch. Whatever way you cut it, either we monkey patch everywhere, introduce a gem everywhere, or something else. I honestly just feel that this is simple enough and useful enough to be directly in pundit.

We could defer logging, and we could implement it different ways, but all of them basically just make things more complex for consumers of the code. We actually already have a gem for some facets of this integration, and we could extend it further, but I feel this is sufficiently generic that other people might benefit from it, which is why I asked @madleech to make a PR. Of course, all the options you suggested are valid options, but they all have different trade offs and in many cases introduce some form of long term technical debt.

Thinking more generally, we'd like to see pundit as an interface for capturing authorization behaviour and this PR fits with my mental model of what this should look like in an application which has to deal with a variety of different situations. Here is a graph which shows the use of the meta-data w.r.t. tracking authorisations which hopefully illustrates in more detail the kind of model I'm aiming for and why this information is helpful:

After we merged this change, we immediately get more detailed feedback regarding how our application is working and how/why it's performing/skipping auth. This is incredibly useful information. The biggest argument I have is that semantically this seems like a very reasonable piece of information to capture when skipping auth as the bar for skipping auth should be set pretty high and this forces people to think about it.

[ioquatix]

By the way, we don't care so much about the sequence of authorisations that was invoked, just the final authorization (or skip) that allowed the action to proceed. We have some cases where it looks like a public action, but can then be authorizaed using some session state if it's present. We don't care about the fact the initial action implementation skipped auth explicitly - but we care about that the session state was present and it was used for allowing the action to continue.

[ioquatix]

@Burgestrand any update?

[madleech]

Closing as we've created our own overlay that extends Pundit with these kinds of attributes (and more).

[ioquatix]

Thanks Michael. It's too bad we couldn't get this merged.

[dgmstuart]

Closing as we've created our own overlay that extends Pundit with these kinds of attributes (and more).

Great - that sounds like the right solution 👍