Skip to content

Commit

Permalink
Remove non-functional redirect options from HTTPS requirements settings.
Browse files Browse the repository at this point in the history
The redirect options were never actually implemented when we
transitioned to the Lua-based internal in v0.9.0. However, they were
left in the interface, but were non-functional.

Since we had been discussing removing the redirect options since as soon
as we added them
(18F/api.data.gov#34 (comment)),
this goes ahead and removes these broken options, since they haven't
been functional since v0.9.0.
  • Loading branch information
GUI committed Feb 20, 2017
1 parent c493546 commit 8d98616
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 20 deletions.
2 changes: 1 addition & 1 deletion docs/admin/api-swagger.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,7 @@ definitions:
description: HTTP basic authentication
require_https:
type: string
enum: ["required_return_error", "required_return_redirect", "transition_return_error", "transition_return_redirect optional"]
enum: ["required_return_error", "transition_return_error", "optional"]
description: HTTPS requirements
require_https_transition_start_at:
type: string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ export default Ember.Component.extend({
requireHttpsOptions: [
{ id: null, name: I18n.t('admin.api.settings.require_https_options.inherit') },
{ id: 'required_return_error', name: I18n.t('admin.api.settings.require_https_options.required_return_error') },
{ id: 'required_return_redirect', name: I18n.t('admin.api.settings.require_https_options.required_return_redirect') },
{ id: 'transition_return_error', name: I18n.t('admin.api.settings.require_https_options.transition_return_error') },
{ id: 'transition_return_redirect', name: I18n.t('admin.api.settings.require_https_options.transition_return_redirect') },
{ id: 'optional', name: I18n.t('admin.api.settings.require_https_options.optional') },
],

Expand Down
2 changes: 1 addition & 1 deletion src/api-umbrella/web-app/app/models/api/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class Api::Settings

# Validations
validates :require_https,
:inclusion => { :in => %w(required_return_error required_return_redirect transition_return_error transition_return_redirect optional), :allow_blank => true }
:inclusion => { :in => %w(required_return_error transition_return_error optional), :allow_blank => true }
validates :api_key_verification_level,
:inclusion => { :in => %w(none transition_email required_email), :allow_blank => true }
validates :rate_limit_mode,
Expand Down
14 changes: 5 additions & 9 deletions src/api-umbrella/web-app/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,8 @@ en:
require_https_tooltip_markdown: |-
Choose whether HTTPS is required to access this API. HTTPS is encouraged to protect the API keys.
- **Required & return message:** HTTPS is required to access the API. HTTP requests will return an error message instructing the user to use an HTTPS URL instead. This is the recommended and default strategy.
- **Required & return redirect:** HTTPS is required to access the API. HTTP requests will return a redirect to the HTTPS URL.
- **Transitionary & return message:** New API keys that signup after choosing this setting will be forced to use HTTPS. Existing API keys may continue to use either HTTP or HTTPS. New API keys using HTTP will return an error message instructing the user to use an HTTPS URL instead.
- **Transitionary & return redirect:** New API keys that signup after choosing this setting will be forced to use HTTPS. Existing API keys may continue to use either HTTP or HTTPS. New API keys using HTTP will return a redirect to the HTTPS URL.
- **Required:** HTTPS is required to access the API. HTTP requests will return an error message instructing the user to use an HTTPS URL instead. This is the recommended and default strategy.
- **Transitionary:** New API keys that signup after choosing this setting will be forced to use HTTPS. Existing API keys may continue to use either HTTP or HTTPS. New API keys using HTTP will return an error message instructing the user to use an HTTPS URL instead.
- **Optional**: HTTPS is optional and either HTTP or HTTPS may be used.
*Notes on redirects:*
Expand All @@ -126,11 +124,9 @@ en:
- If API clients rely on the redirect for HTTPS access, this strategy does not secure the API keys, since the client may still be making an insecure initial HTTP request with their API key.
- For GET requests a `301 Moved Permanently` redirect will be returned. For all other HTTP methods a `307 Temporary Redirect` redirect will be returned (to instruct the client to retry using the same HTTP method).
require_https_options:
inherit: Inherit (default - required & return message)
required_return_error: Required & return message - HTTP requests will receive a message to use HTTPS
required_return_redirect: Required & return redirects - HTTP requests will be redirected to HTTPS
transition_return_error: Transitionary & return message - Optional for existing API keys, required for new API keys
transition_return_redirect: Transitionary & return redirects - Optional for existing API keys, required for new API keys
inherit: Inherit (default - required)
required_return_error: Required - HTTP requests will receive a message to use HTTPS
transition_return_error: Transitionary - Optional for existing API keys, required for new API keys
optional: Optional - HTTPS is optional
disable_api_key: API Key Checks
disable_api_key_options:
Expand Down
4 changes: 2 additions & 2 deletions test/admin_ui/test_apis.rb
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def test_form
within(".modal") do
select "OPTIONS", :from => "HTTP Method"
fill_in "Regex", :with => "^/foo.*"
select "Required & return message - HTTP requests will receive a message to use HTTPS", :from => "HTTPS Requirements"
select "Required - HTTP requests will receive a message to use HTTPS", :from => "HTTPS Requirements"
select "Disabled - API keys are optional", :from => "API Key Checks"
select "E-mail verification required - Existing API keys will break, only new API keys will work if verified", :from => "API Key Verification Requirements"
# FIXME: Without this sleep, then the selectize test below will randomly
Expand Down Expand Up @@ -290,7 +290,7 @@ def test_form
within(".modal") do
assert_select("HTTP Method", :selected => "OPTIONS")
assert_field("Regex", :with => "^/foo.*")
assert_select("HTTPS Requirements", :selected => "Required & return message - HTTP requests will receive a message to use HTTPS")
assert_select("HTTPS Requirements", :selected => "Required - HTTP requests will receive a message to use HTTPS")
assert_select("API Key Checks", :selected => "Disabled - API keys are optional")
assert_select("API Key Verification Requirements", :selected => "E-mail verification required - Existing API keys will break, only new API keys will work if verified")
field = find_field("Required Roles")
Expand Down
7 changes: 2 additions & 5 deletions test/apis/v1/config/test_publish_transitionary_https.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def test_transition_return_error_set_timestamp
assert_set_timestamp(:transition_return_error)
end

["transition_return_error", "transition_return_redirect"].each do |mode|
["transition_return_error"].each do |mode|
define_method("test_#{mode}_set_timestamp") do
api = FactoryGirl.create(:api, {
:settings => {
Expand Down Expand Up @@ -125,9 +125,6 @@ def test_transition_return_error_set_timestamp
api.settings.require_https = "required_return_error"
api.save!

api.settings.require_https = "required_return_redirect"
api.save!

api.settings.require_https = "optional"
api.save!

Expand Down Expand Up @@ -160,7 +157,7 @@ def test_transition_return_error_set_timestamp
end
end

["required_return_error", "required_return_redirect", "optional", nil].each do |mode|
["required_return_error", "optional", nil].each do |mode|
mode_method_name = mode || mode.inspect

define_method("test_#{mode_method_name}_unset_timestamp") do
Expand Down

0 comments on commit 8d98616

Please sign in to comment.