Adds HTTP basic authentication #4
Conversation
|
BAHAHA, its all your fault @cannikin |
There was a problem hiding this comment.
2 things:
Could we add in where we automatically read from the credentials? https://github.com/rails/mission_control-jobs/pull/214/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R61
Is it possible that we could piggy back off of mission controls configuration and default to using it? For example, if i configure mission control, active insights just automagically inherits and uses its settings. If your using both, theres a pretty good chance you just want the same auth for both and saves from having duplicate configs
test/controllers/concerns/active_insights/basic_authentication_test.rb
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| def authenticate_by_http_basic | ||
| return unless http_basic_authentication_enabled? |
There was a problem hiding this comment.
Do we need this extra check? If the creds are set i think we are safe to assume auth is enabled.
There was a problem hiding this comment.
That's what MissionControl is doing, not sure what the benefit is...maybe with ENV vars always being strings you could accidentally set user/pass to an empty string/nil? So having to also set enabled makes it explicit that you really do want to turn it on...
Hahaha small world. All the duct tape is still holding up! |
I thought about this but then realized that if you want to allow someone access to metrics, but not to manage jobs, you're doing to have to jump through a couple of hoops. Whereas right now it's easy to support either config: when setting the user/password for ActiveInsights you either set them to the same values as the MissionControl config, or set them to something different. If we force them to be the same then you have to use an |
|
@npezza93 any feedback on my comments above? |
|
Hey @cannikin, ill review this weekend and get back to you |
I borrowed the implementation from mission_control-jobs since it seemed pretty simple:
ActiveInsights::ApplicationControllerActiveInsightsmoduleThere's one Rubocop warning because I'm subclassing
ActionController::Basein the test instead ofApplicationController. I'll add an ignore comment if you're okay with it!Closes #3
BTW I'm the original dev from Working Not Working! Sorry about the code. 😅 @jondavidchristopher brags about hiring you now that some of your stuff made it into core Rails!