More granular User API Key scopes, and plugin support (PR #10836)

This consolidates logic used to match routes in ApiKey, UserApiKey and DefaultCurrentUserProvider. This reduces duplicated logic, and will allow UserApiKeysScope to easily re-use the parameter matching logic from ApiKeyScope

GitHub

The title of this pull request changed from “REFACTOR: Introduce RouteMatcher class” to "More granular User API Key scopes, and plugin support

This is looking good, but it feels like the RouteMatcher is getting constructed at the wrong time. At the moment you’re having to pass in allowed_param_values to the match method, but I think it would be clearer if the RouteMatcher had all of the data necessary to match against an arbitrary request. You could either achieve this by either allowing a RouteMatcher to be refined after construction, i.e. RouteMatcher.new(...).with_allowed_param_values(...) or not putting RouteMatchers into the constant.

it feels like the RouteMatcher is getting constructed at the wrong time. At the moment you’re having to pass in allowed_param_values to the match method

All the parameters which RouteMatcher is initialized are constant. The “allowed param values” vary at runtime based on user-provided configuration. So they do need to be separated somehow, but we can certainly try and improve the ergonomics.

You could achieve this by either allowing a RouteMatcher to be refined after construction, i.e. RouteMatcher.new(…).with_allowed_param_values(…)

I would be a little worried about making this thread-safe. with_allowed_param_values would have to return a fresh RouteMatcher to avoid polluting the unrefined RouteMatcher. Is that what you had in mind?

not putting RouteMatchers into the constant.

Yeah I wasn’t too sure about this… it feels a little weird. But one major advantage is that it means missing/extra initialisation parameters are detected at boot, rather than at runtime.

I would be a little worried about making this thread-safe. with_allowed_param_values would have to return a fresh RouteMatcher to avoid polluting the unrefined RouteMatcher. Is that what you had in mind?

Yes, a fresh RouteMatcher.

I bring this up, because the name RouteMatcher implies to me that it has everything that it needs to match a route.

I think we don’t need to assign the params variable anymore if we pass the env directly.

Do we want to drop the old parameters at some point? If so, maybe we should a deprecation notice?

Thanks @romanrizzi and @danielwaterworth - have made some improvements based on your comments.

Fixed the failing specs, and updated the plugin API to automatically prefix scope names with the plugin name to prevent clashes (thanks for the suggestion @danielwaterworth)