-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds HTTP basic authentication #4
base: main
Are you sure you want to change the base?
Changes from all commits
9040ba3
b7fee7f
10196ac
8186532
91abb7d
10e797d
79715c2
50c1b6a
7691f6c
9dcfb65
65e0038
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# frozen_string_literal: true | ||
|
||
module ActiveInsights::BasicAuthentication | ||
extend ActiveSupport::Concern | ||
|
||
included do | ||
before_action :authenticate_by_http_basic | ||
end | ||
|
||
def authenticate_by_http_basic | ||
return unless http_basic_authentication_enabled? | ||
|
||
if http_basic_authentication_configured? | ||
http_basic_authenticate_or_request_with(**http_basic_auth_credentials) | ||
else | ||
head :unauthorized | ||
end | ||
end | ||
|
||
def http_basic_authentication_enabled? | ||
ActiveInsights.http_basic_auth_enabled | ||
end | ||
|
||
def http_basic_authentication_configured? | ||
http_basic_auth_credentials.values.all?(&:present?) | ||
end | ||
|
||
def http_basic_auth_credentials | ||
{ | ||
name: ActiveInsights.http_basic_auth_user, | ||
password: ActiveInsights.http_basic_auth_password | ||
}.transform_values(&:presence) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,9 @@ | |
require "active_insights/engine" | ||
|
||
module ActiveInsights | ||
mattr_accessor :connects_to, :ignored_endpoints, :enabled | ||
mattr_accessor :connects_to, :ignored_endpoints, :enabled, | ||
:http_basic_auth_enabled, :http_basic_auth_user, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Then we can remove a bunch of the controller stuff and move it into its own class:
|
||
:http_basic_auth_password | ||
|
||
class << self | ||
def ignored_endpoint?(payload) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
# frozen_string_literal: true | ||
|
||
module ActiveInsights | ||
VERSION = "1.3.1" | ||
VERSION = "1.4.0" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be reverted |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
# frozen_string_literal: true | ||
|
||
require "test_helper" | ||
|
||
class ActiveInsights::BasicAuthenticationTest < ActionDispatch::IntegrationTest | ||
test "it does not require http basic auth by default" do | ||
get active_insights.requests_path | ||
|
||
assert_response :success | ||
end | ||
|
||
test "it requires http basic auth when enabled" do | ||
with_http_basic_auth do | ||
get active_insights.requests_path | ||
|
||
assert_response :unauthorized | ||
end | ||
end | ||
|
||
test "allows access with correct credentials" do | ||
with_http_basic_auth(user: "dev", password: "secret") do | ||
get active_insights.requests_path, headers: auth_headers("dev", "secret") | ||
|
||
assert_response :success | ||
end | ||
end | ||
|
||
test "disallows access with incorrect credentials" do | ||
with_http_basic_auth(user: "dev", password: "secret") do | ||
get active_insights.requests_path, headers: auth_headers("dev", "wrong") | ||
|
||
assert_response :unauthorized | ||
end | ||
end | ||
|
||
private | ||
|
||
# rubocop:disable Metrics/MethodLength | ||
def with_http_basic_auth(enabled: true, user: nil, password: nil) | ||
previous_enabled, ActiveInsights.http_basic_auth_enabled = | ||
ActiveInsights.http_basic_auth_enabled, enabled | ||
previous_user, ActiveInsights.http_basic_auth_user = | ||
ActiveInsights.http_basic_auth_user, user | ||
previous_password, ActiveInsights.http_basic_auth_password = | ||
ActiveInsights.http_basic_auth_password, password | ||
yield | ||
ensure | ||
ActiveInsights.http_basic_auth_enabled = previous_enabled | ||
ActiveInsights.http_basic_auth_user = previous_user | ||
ActiveInsights.http_basic_auth_password = previous_password | ||
end | ||
# rubocop:enable Metrics/MethodLength | ||
|
||
def auth_headers(user, password) | ||
{ | ||
Authorization: | ||
ActionController::HttpAuthentication::Basic.encode_credentials( | ||
user, password | ||
) | ||
} | ||
end | ||
end |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...