Offer "allow_authorized" as an alternative to "whitelist"

MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.

MIT License

3.7k stars 402 forks source link

Offer "allow_authorized" as an alternative to "whitelist" #491

Closed rthbound closed 3 years ago

rthbound commented 3 years ago

Some teams and projects will wish to use rack-mini-profiler without needing to use terminology they feel is exclusive/biased/harmful
This change adds support for 'allow_authorized' as a suitable alternative for replacing 'whitelist'
In truth, the concept of 'explicit authorization' is more true to how this library works: there is no list of allowed endpoints, but rather some endpoints are explicitly allowed and others are not.
No immediate change required for clients -- they can continue to specify whatever authorization mode they used before this change. Using the old authorization mode identifier will emit a deprecation warning and set the authorization mode to ":allow_authorized" instead.
Include tests showing backward compatibility and support for the new authorization mode identifier.
Update integration spec to: utilize :allow_authorized authorization mode; bring ancillary language into alignment

SamSaffron commented 3 years ago

something about the words here is not working for me:

We have #authorization_mode= , explicit and allow_all

explicit does not feel like the correct word.

Maybe instead allow_all and allow_authorized ? Maybe allow_all and deny_unauthorized ?

Also since we have a setter here, we should exception out if people try to set an unexpected authorization mode.

rthbound commented 3 years ago

@SamSaffron I did consider throwing an exception if people set an unexpected mode, however this could be a breaking change:

It's possible users have configured the library with some typo
They may have passed in strings instead of symbols

In either of these cases, MiniProfiler.config.authorization_mode == :whitelist would have returned false, sure, but there would not have been an exception.

What do you think we should do? We could emit a deprecation warning and claim that in the future we will raise if the given argument is not one of :allow_all or :allow_authorized.

My thinking was that this entire setter method would disappear once :whitelist is fully removed.

SamSaffron commented 3 years ago

Yeah let's do a deprecation for the next 6 months and then we can remove the deprecation and just throw an exception for unexpected modes.

SamSaffron commented 3 years ago

A few tiny changes and I think we are good to merge! thanks @rthbound :confetti_ball:

rthbound commented 3 years ago

@SamSaffron thank you for your collaboration on this!

SamSaffron commented 3 years ago

looking great, merging this in, any idea why CI is playing up?

rthbound commented 3 years ago

I haven't seen that before. Looks flaky -- this report of that particular error claims it was happening around ~5% of the time, once upon a time: https://github.com/rubygems/rubygems/issues/4187

SamSaffron commented 3 years ago

No worried Ryan, yeah looks like it is passing once merged.

Thanks again for the contribution. We will move from master -> main as well some time in the next few weeks.