Skip to content

Adds HTTP basic authentication#4

Open
cannikin wants to merge 11 commits intonpezza93:mainfrom
cannikin:http-basic-auth
Open

Adds HTTP basic authentication#4
cannikin wants to merge 11 commits intonpezza93:mainfrom
cannikin:http-basic-auth

Conversation

@cannikin
Copy link

@cannikin cannikin commented Dec 12, 2024

I borrowed the implementation from mission_control-jobs since it seemed pretty simple:

  • Adds a concern, includes it in ActiveInsights::ApplicationController
  • Adds config vars to the ActiveInsights module
  • Tests
  • Updates README to include config examples

There's one Rubocop warning because I'm subclassing ActionController::Base in the test instead of ApplicationController. 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!

@jondavidchristopher
Copy link
Contributor

BAHAHA, its all your fault @cannikin

Copy link
Owner

@npezza93 npezza93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

def authenticate_by_http_basic
return unless http_basic_authentication_enabled?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this extra check? If the creds are set i think we are safe to assume auth is enabled.

Copy link
Author

@cannikin cannikin Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@npezza93
Copy link
Owner

npezza93 commented Dec 13, 2024

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!

Hahaha small world. All the duct tape is still holding up!

@cannikin
Copy link
Author

Is it possible that we could piggy back off of mission controls configuration and default to using it?

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 after_initialize to override the auto ones we set, which is a little counterintuitive. (I'm actually doing after_initialize for MissionControl itself because I'm not using Rails credentials, and the automagic setting happens after application.rb config is run and after config/initializers are invoked.)

@cannikin
Copy link
Author

@npezza93 any feedback on my comments above?

@npezza93
Copy link
Owner

Hey @cannikin, ill review this weekend and get back to you


module ActiveInsights
VERSION = "1.3.1"
VERSION = "1.4.0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be reverted

module ActiveInsights
mattr_accessor :connects_to, :ignored_endpoints, :enabled
mattr_accessor :connects_to, :ignored_endpoints, :enabled,
:http_basic_auth_enabled, :http_basic_auth_user,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about moving this into a new object? For example:

config.active_insights.http_basic_auth.enabled = true

Then we can remove a bunch of the controller stuff and move it into its own class:

class ActiveInsights::HttpBasicAuth
  attr_accessor :enabled, :user, :password 

  def enabled?
    enabled
  end

  ..
end

module ActiveInsights
  def self.http_basic_auth
    @http_basic_auth ||= HttpBasicAuth.new
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simple auth support (HTTP basic)?

3 participants

Comments