From 661412dedd3430686eac336ce2df8b2993cfaadd Mon Sep 17 00:00:00 2001 From: Andrew Hodgkinson Date: Wed, 15 Nov 2023 15:25:43 +1300 Subject: [PATCH 1/3] Ensure Content-Type is always set in responses and use more common WWW-Authenticate header name, rather than the previous Rack-like name --- .../scimitar/application_controller.rb | 9 +- .../scimitar/application_controller_spec.rb | 4 +- ...record_backed_resources_controller_spec.rb | 144 +++++++++++++----- 3 files changed, 119 insertions(+), 38 deletions(-) diff --git a/app/controllers/scimitar/application_controller.rb b/app/controllers/scimitar/application_controller.rb index 4e29857..01ed3af 100644 --- a/app/controllers/scimitar/application_controller.rb +++ b/app/controllers/scimitar/application_controller.rb @@ -124,8 +124,13 @@ def add_mandatory_response_headers # # https://stackoverflow.com/questions/10239970/what-is-the-delimiter-for-www-authenticate-for-multiple-schemes # - response.set_header('WWW_AUTHENTICATE', 'Basic' ) if Scimitar.engine_configuration.basic_authenticator.present? - response.set_header('WWW_AUTHENTICATE', 'Bearer') if Scimitar.engine_configuration.token_authenticator.present? + response.set_header('WWW-Authenticate', 'Basic' ) if Scimitar.engine_configuration.basic_authenticator.present? + response.set_header('WWW-Authenticate', 'Bearer') if Scimitar.engine_configuration.token_authenticator.present? + + # No matter what a caller might request via headers, the only content + # type we can ever respond with is JSON-for-SCIM. + # + response.set_header('Content-Type', "#{Mime::Type.lookup_by_extension(:scim)}; charset=utf-8") end def authenticate diff --git a/spec/controllers/scimitar/application_controller_spec.rb b/spec/controllers/scimitar/application_controller_spec.rb index b284030..4622882 100644 --- a/spec/controllers/scimitar/application_controller_spec.rb +++ b/spec/controllers/scimitar/application_controller_spec.rb @@ -24,7 +24,7 @@ def index get :index, params: { format: :scim } expect(response).to be_ok expect(JSON.parse(response.body)).to eql({ 'message' => 'cool, cool!' }) - expect(response.headers['WWW_AUTHENTICATE']).to eql('Basic') + expect(response.headers['WWW-Authenticate']).to eql('Basic') end it 'renders failure with bad password' do @@ -84,7 +84,7 @@ def index get :index, params: { format: :scim } expect(response).to be_ok expect(JSON.parse(response.body)).to eql({ 'message' => 'cool, cool!' }) - expect(response.headers['WWW_AUTHENTICATE']).to eql('Bearer') + expect(response.headers['WWW-Authenticate']).to eql('Bearer') end it 'renders failure with bad token' do diff --git a/spec/requests/active_record_backed_resources_controller_spec.rb b/spec/requests/active_record_backed_resources_controller_spec.rb index 9090f7f..fd519f6 100644 --- a/spec/requests/active_record_backed_resources_controller_spec.rb +++ b/spec/requests/active_record_backed_resources_controller_spec.rb @@ -26,13 +26,17 @@ context '#index' do context 'with no items' do - it 'returns empty list' do + before :each do MockUser.delete_all + end + it 'returns empty list' do expect_any_instance_of(MockUsersController).to receive(:index).once.and_call_original get '/Users', params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(0) @@ -46,7 +50,9 @@ it 'returns all items' do get '/Users', params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(3) @@ -64,7 +70,9 @@ it 'returns all items' do get '/Groups', params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(3) @@ -84,7 +92,9 @@ filter: 'name.givenName eq "FOO" and name.familyName pr and emails ne "home_1@test.com"' } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(1) @@ -103,7 +113,9 @@ filter: 'name.GIVENNAME eq "Foo" and name.Familyname pr and emails ne "home_1@test.com"' } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(1) @@ -126,7 +138,9 @@ filter: "id eq \"#{@u3.primary_key}\"" } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(1) @@ -145,7 +159,9 @@ filter: "externalID eq \"#{@u2.scim_uid}\"" } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(1) @@ -164,7 +180,9 @@ filter: "Meta.LastModified eq \"#{@u3.updated_at}\"" } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(1) @@ -184,7 +202,9 @@ count: 2 } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(3) @@ -203,7 +223,9 @@ startIndex: 2 } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['totalResults']).to eql(3) @@ -224,8 +246,11 @@ filter: 'name.givenName' } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['scimType']).to eql('invalidFilter') end end # "context 'with bad calls' do" @@ -239,7 +264,9 @@ expect_any_instance_of(MockUsersController).to receive(:show).once.and_call_original get "/Users/#{@u2.primary_key}", params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -254,7 +281,9 @@ expect_any_instance_of(MockGroupsController).to receive(:show).once.and_call_original get "/Groups/#{@g2.id}", params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@g2.id.to_s) # Note - ID was converted String; not Integer @@ -266,8 +295,11 @@ it 'renders 404' do get '/Users/xyz', params: { format: :scim } - expect(response.status).to eql(404) + expect(response.status ).to eql(404) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['status']).to eql('404') end end # "context '#show' do" @@ -291,7 +323,9 @@ mock_after = MockUser.all.to_a new_mock = (mock_after - mock_before).first - expect(response.status).to eql(201) + expect(response.status ).to eql(201) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(new_mock.primary_key.to_s) @@ -332,7 +366,9 @@ mock_after = MockUser.all.to_a new_mock = (mock_after - mock_before).first - expect(response.status).to eql(201) + expect(response.status ).to eql(201) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(new_mock.id.to_s) @@ -363,8 +399,11 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(409) + expect(response.status ).to eql(409) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['scimType']).to eql('uniqueness') expect(result['detail']).to include('already been taken') end @@ -377,8 +416,11 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['scimType']).to eql('invalidValue') expect(result['detail']).to include('is required') end @@ -391,7 +433,9 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['scimType']).to eql('invalidValue') @@ -410,7 +454,9 @@ mock_after = MockUser.all.to_a new_mock = (mock_after - mock_before).first - expect(response.status).to eql(201) + expect(response.status ).to eql(201) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + expect(new_mock.username).to eql(CustomSaveMockUsersController::CUSTOM_SAVE_BLOCK_USERNAME_INDICATOR) end end # "context '#create' do" @@ -428,7 +474,9 @@ put "/Users/#{@u2.primary_key}", params: attributes.merge(format: :scim) }.to_not change { MockUser.count } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -459,8 +507,11 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['scimType']).to eql('invalidValue') expect(result['detail']).to include('is required') @@ -480,7 +531,9 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['scimType']).to eql('invalidValue') @@ -502,8 +555,11 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(404) + expect(response.status ).to eql(404) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['status']).to eql('404') end end # "context '#replace' do" @@ -535,7 +591,9 @@ patch "/Users/#{@u2.primary_key}", params: payload.merge(format: :scim) }.to_not change { MockUser.count } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -572,7 +630,9 @@ patch "/Users/#{@u2.primary_key}", params: payload.merge(format: :scim) }.to_not change { MockUser.count } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -604,7 +664,9 @@ patch "/Users/#{@u2.primary_key}", params: payload.merge(format: :scim) }.to_not change { MockUser.count } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -636,7 +698,9 @@ patch "/Users/#{@u2.primary_key}", params: payload.merge(format: :scim) }.to_not change { MockUser.count } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['id']).to eql(@u2.primary_key.to_s) @@ -675,7 +739,9 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(400) + expect(response.status ).to eql(400) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['scimType']).to eql('invalidValue') @@ -703,8 +769,11 @@ } }.to_not change { MockUser.count } - expect(response.status).to eql(404) + expect(response.status ).to eql(404) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['status']).to eql('404') end @@ -741,7 +810,9 @@ get "/Groups/#{@g1.id}", params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['members']).to be_empty @@ -768,7 +839,9 @@ get "/Groups/#{@g1.id}", params: { format: :scim } - expect(response.status).to eql(200) + expect(response.status ).to eql(200) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) expect(result['members'].map { |m| m['value'] }.sort()).to eql(expected_remaining_user_ids) @@ -879,8 +952,11 @@ delete '/Users/xyz', params: { format: :scim } }.to_not change { MockUser.count } - expect(response.status).to eql(404) + expect(response.status ).to eql(404) + expect(response.headers['Content-Type']).to eql('application/scim+json; charset=utf-8') + result = JSON.parse(response.body) + expect(result['status']).to eql('404') end end # "context '#destroy' do" From ae0bd5415ca625888bcdda005ee55a0679098a80 Mon Sep 17 00:00:00 2001 From: Andrew Hodgkinson Date: Wed, 15 Nov 2023 15:28:23 +1300 Subject: [PATCH 2/3] Updates for v2.6.1 --- CHANGELOG.md | 5 +++++ Gemfile.lock | 2 +- lib/scimitar/version.rb | 4 ++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61a131e..7fa0349 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +# 2.6.1 (2023-11-15) + +* Always returns `Content-Type` header of `application/scim+json; charset=utf-8` for any response, since that's the only format the gem can ever answer with. Fixes [#59](https://github.com/RIPAGlobal/scimitar/issues/59). +* Uses the more common header name form of `WWW-Authenticate` rather than the Rack-like `WWW_AUTHENTICATE` in responses. + # 2.6.0 (2023-11-14) Features: diff --git a/Gemfile.lock b/Gemfile.lock index 67f374f..00eea3e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: . specs: - scimitar (2.6.0) + scimitar (2.6.1) rails (~> 7.0) GEM diff --git a/lib/scimitar/version.rb b/lib/scimitar/version.rb index bef9423..ebcd4ee 100644 --- a/lib/scimitar/version.rb +++ b/lib/scimitar/version.rb @@ -3,11 +3,11 @@ module Scimitar # Gem version. If this changes, be sure to re-run "bundle install" or # "bundle update". # - VERSION = '2.6.0' + VERSION = '2.6.1' # Date for VERSION. If this changes, be sure to re-run "bundle install" # or "bundle update". # - DATE = '2023-11-14' + DATE = '2023-11-15' end From 2f983bf7f8aeadd8e8f25996f1a2d6e87499e682 Mon Sep 17 00:00:00 2001 From: Andrew Hodgkinson Date: Wed, 15 Nov 2023 15:32:38 +1300 Subject: [PATCH 3/3] Better CHANGELOG.md wording --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fa0349..6347cb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # 2.6.1 (2023-11-15) -* Always returns `Content-Type` header of `application/scim+json; charset=utf-8` for any response, since that's the only format the gem can ever answer with. Fixes [#59](https://github.com/RIPAGlobal/scimitar/issues/59). +* Always returns a `Content-Type` header with value `application/scim+json; charset=utf-8` in any response, since that's the only format the gem can write. Fixes [#59](https://github.com/RIPAGlobal/scimitar/issues/59). * Uses the more common header name form of `WWW-Authenticate` rather than the Rack-like `WWW_AUTHENTICATE` in responses. # 2.6.0 (2023-11-14)