Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions lib/optimizely/config_manager/http_project_config_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,18 @@ def fetch_datafile_config
end

def request_config
@logger.log(
Logger::DEBUG,
"Fetching datafile from #{@datafile_url}"
)
begin
headers = {}
headers['Content-Type'] = 'application/json'
headers['If-Modified-Since'] = @last_modified if @last_modified
headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil?
@logger.log(Logger::DEBUG, "Fetching datafile from #{@datafile_url}")
headers = {}
headers['Content-Type'] = 'application/json'
headers['If-Modified-Since'] = @last_modified if @last_modified
headers['Authorization'] = "Bearer #{@access_token}" unless @access_token.nil?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, that is exactly what i did. This is why i cleansed the headers before logging and replaced the value of Authorization header to *** right below this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

ha, i saw that. i deleted my comment. but, it would be nice to add a simple logger spy test to see that was outputted :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup.. good idea.. doing that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests


# Cleaning headers before logging to avoid exposing authorization token
cleansed_headers = {}
headers.each { |key, value| cleansed_headers[key] = key == 'Authorization' ? '********' : value }
@logger.log(Logger::DEBUG, "Datafile request headers: #{cleansed_headers}")

begin
response = Helpers::HttpUtils.make_request(
@datafile_url, :get, nil, headers, Helpers::Constants::CONFIG_MANAGER['REQUEST_TIMEOUT']
)
Expand All @@ -169,6 +171,9 @@ def request_config
return nil
end

response_code = response.code.to_i
@logger.log(Logger::DEBUG, "Datafile response status code #{response_code}")

# Leave datafile and config unchanged if it has not been modified.
if response.code == '304'
@logger.log(
Expand All @@ -178,9 +183,14 @@ def request_config
return
end

@last_modified = response[Helpers::Constants::HTTP_HEADERS['LAST_MODIFIED']]

config = DatafileProjectConfig.create(response.body, @logger, @error_handler, @skip_json_validation) if response.body
if response_code >= 200 && response_code < 400
@logger.log(Logger::DEBUG, 'Successfully fetched datafile, generating Project config')
config = DatafileProjectConfig.create(response.body, @logger, @error_handler, @skip_json_validation)
@last_modified = response[Helpers::Constants::HTTP_HEADERS['LAST_MODIFIED']]
Copy link
Contributor

@thomaszurkan-optimizely thomaszurkan-optimizely Jul 6, 2020

Choose a reason for hiding this comment

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

nit: what happens if last modified is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is assigned as nil. I just moved this line of code inside the check. assuming it was working correctly earlier as well without a nil check

@logger.log(Logger::DEBUG, "Saved last modified header value from response: #{@last_modified}.")
else
@logger.log(Logger::DEBUG, "Datafile fetch failed, status: #{response.code}, message: #{response.message}")
end

config
end
Expand Down
26 changes: 24 additions & 2 deletions spec/config_manager/http_project_config_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
'Content-Type' => 'application/json'
}
)
.to_return(status: 403, body: '', headers: {})
.to_return(status: [403, 'Forbidden'], body: '', headers: {})
end

after(:example) do
Expand Down Expand Up @@ -236,7 +236,7 @@
sleep 0.3

expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'Fetching datafile from https://cdn.optimizely.com/datafiles/valid_sdk_key.json').once

expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'Datafile response status code 200').once
expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'Received new datafile and updated config. ' \
'Old revision number: 42. New revision number: 81.').once

Expand Down Expand Up @@ -361,6 +361,17 @@

expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Invalid url_template https://cdn.optimizely.com/datafiles/%d.json provided.')
end

it 'Should log failure message with status code when failed to fetch datafile' do
@http_project_config_manager = Optimizely::HTTPProjectConfigManager.new(
url: 'https://cdn.optimizely.com/datafiles/invalid_sdk_key.json',
sdk_key: 'valid_sdk_key',
datafile_access_token: 'the-token',
logger: spy_logger
)
sleep 0.1
expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'Datafile fetch failed, status: 403, message: Forbidden').once
end
end

describe '.polling_interval' do
Expand Down Expand Up @@ -504,5 +515,16 @@
sleep 0.1
expect(Optimizely::Helpers::HttpUtils).to have_received(:make_request).with('http://awesomeurl', any_args)
end

it 'should hide access token when printing logs' do
allow(Optimizely::Helpers::HttpUtils).to receive(:make_request)
@http_project_config_manager = Optimizely::HTTPProjectConfigManager.new(
sdk_key: 'valid_sdk_key',
datafile_access_token: 'the-token',
logger: spy_logger
)
sleep 0.1
expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'Datafile request headers: {"Content-Type"=>"application/json", "Authorization"=>"********"}').once
end
end
end