From 8d6b27ebe6327bf0fd9587ad56393466dffe8507 Mon Sep 17 00:00:00 2001 From: Glenn Roberts Date: Fri, 16 Jun 2017 18:23:44 +1000 Subject: [PATCH 1/3] feat: improve response error codes --- lib/webpush/errors.rb | 16 +++++++++++++++- lib/webpush/request.rb | 12 +++++++++--- spec/webpush_spec.rb | 29 +++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/lib/webpush/errors.rb b/lib/webpush/errors.rb index 0f7060b..d65e4a3 100644 --- a/lib/webpush/errors.rb +++ b/lib/webpush/errors.rb @@ -3,7 +3,21 @@ class Error < RuntimeError; end class ConfigurationError < Error; end - class ResponseError < Error; end + class ResponseError < Error; + attr_reader :response, :host + + def initialize(response, host) + @response = response + @host = host + super "host: #{host}, #{@response.inspect}\nbody:\n#{@response.body}" + end + end class InvalidSubscription < ResponseError; end + + class ExpiredSubscription < ResponseError; end + + class PayloadTooLarge < ResponseError; end + + class TooManyRequests < ResponseError; end end diff --git a/lib/webpush/request.rb b/lib/webpush/request.rb index 8647966..33ce1a7 100644 --- a/lib/webpush/request.rb +++ b/lib/webpush/request.rb @@ -24,9 +24,15 @@ def perform if resp.is_a?(Net::HTTPGone) || #Firefox unsubscribed response (resp.is_a?(Net::HTTPBadRequest) && resp.message == "UnauthorizedRegistration") #Chrome unsubscribed response - raise InvalidSubscription.new(resp.inspect) - elsif !resp.is_a?(Net::HTTPSuccess) #unknown/unhandled response error - raise ResponseError.new "host: #{uri.host}, #{resp.inspect}\nbody:\n#{resp.body}" + raise InvalidSubscription.new(resp, uri.host) + elsif resp.is_a?(Net::HTTPNotFound) # 404 + raise ExpiredSubscription.new(resp, uri.host) + elsif resp.is_a?(Net::HTTPRequestEntityTooLarge) # 413 + raise PayloadTooLarge.new(resp, uri.host) + elsif resp.is_a?(Net::HTTPTooManyRequests) # 429, try again later! + raise TooManyRequests.new(resp, uri.host) + elsif !resp.is_a?(Net::HTTPSuccess) # unknown/unhandled response error + raise ResponseError.new(resp, uri.host) end resp diff --git a/spec/webpush_spec.rb b/spec/webpush_spec.rb index 8c39f7a..ed9c33c 100644 --- a/spec/webpush_spec.rb +++ b/spec/webpush_spec.rb @@ -201,6 +201,24 @@ expect { subject }.not_to raise_error(Webpush::InvalidSubscription) end + it 'raises ExpiredSubscription if the API returns a 404 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 404, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::ExpiredSubscription) + end + + it 'raises PayloadTooLarge if the API returns a 413 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 413, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::PayloadTooLarge) + end + + it 'raises TooManyRequests if the API returns a 429 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 429, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::TooManyRequests) + end + it 'raises ResponseError for unsuccessful status code by default' do stub_request(:post, expected_endpoint). to_return(status: 401, body: "", headers: {}) @@ -208,6 +226,17 @@ expect { subject }.to raise_error(Webpush::ResponseError) end + it 'supplies the original status code on the ResponseError' do + stub_request(:post, expected_endpoint). + to_return(status: 401, body: "Oh snap", headers: {}) + + expect { subject }.to raise_error { |error| + expect(error).to be_a(Webpush::ResponseError) + expect(error.response.code).to eq '401' + expect(error.response.body).to eq 'Oh snap' + } + end + it 'raises exception on error by default' do stub_request(:post, expected_endpoint).to_raise(StandardError) From 43121b938606fa63495348c7f420294aa99566d1 Mon Sep 17 00:00:00 2001 From: Glenn Roberts Date: Fri, 16 Jun 2017 18:42:22 +1000 Subject: [PATCH 2/3] assume GCM and VAPID protocol errors can be treated the same --- spec/webpush_spec.rb | 141 ++++++++++++++++++------------------------- 1 file changed, 60 insertions(+), 81 deletions(-) diff --git a/spec/webpush_spec.rb b/spec/webpush_spec.rb index ed9c33c..d468744 100644 --- a/spec/webpush_spec.rb +++ b/spec/webpush_spec.rb @@ -5,6 +5,64 @@ expect(Webpush::VERSION).not_to be nil end + shared_examples 'web push protocol standard error handling' do + it 'raises InvalidSubscription if and only if the combination of status code and message indicate an invalid subscription' do + stub_request(:post, expected_endpoint). + to_return(status: 410, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::InvalidSubscription) + + stub_request(:post, expected_endpoint). + to_return(status: [400, "UnauthorizedRegistration"], body: "", headers: {}) + expect { subject }.to raise_error(Webpush::InvalidSubscription) + + stub_request(:post, expected_endpoint). + to_return(status: 400, body: "", headers: {}) + expect { subject }.not_to raise_error(Webpush::InvalidSubscription) + end + + it 'raises ExpiredSubscription if the API returns a 404 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 404, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::ExpiredSubscription) + end + + it 'raises PayloadTooLarge if the API returns a 413 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 413, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::PayloadTooLarge) + end + + it 'raises TooManyRequests if the API returns a 429 Error' do + stub_request(:post, expected_endpoint). + to_return(status: 429, body: "", headers: {}) + expect { subject }.to raise_error(Webpush::TooManyRequests) + end + + it 'raises ResponseError for unsuccessful status code by default' do + stub_request(:post, expected_endpoint). + to_return(status: 401, body: "", headers: {}) + + expect { subject }.to raise_error(Webpush::ResponseError) + end + + it 'supplies the original status code on the ResponseError' do + stub_request(:post, expected_endpoint). + to_return(status: 401, body: "Oh snap", headers: {}) + + expect { subject }.to raise_error { |error| + expect(error).to be_a(Webpush::ResponseError) + expect(error.response.code).to eq '401' + expect(error.response.body).to eq 'Oh snap' + } + end + + it 'raises exception on error by default' do + stub_request(:post, expected_endpoint).to_raise(StandardError) + + expect { subject }.to raise_error + end + end + shared_examples 'request headers with VAPID' do let(:message) { JSON.generate({ body: 'body' }) } let(:p256dh) { 'BN4GvZtEZiZuqFxSKVZfSfluwKBD7UxHNBmWkfiZfCtgDE8Bwh-_MtLXbBxTBAWH9r7IPKL0lhdcaqtL1dfxU5E=' } @@ -64,32 +122,7 @@ expect(result.code).to eql('201') end - it 'raises InvalidSubscription if and only if the combination of status code and message indicate an invalid subscription' do - stub_request(:post, expected_endpoint). - to_return(status: 410, body: "", headers: {}) - expect { subject }.to raise_error(Webpush::InvalidSubscription) - - stub_request(:post, expected_endpoint). - to_return(status: [400, "UnauthorizedRegistration"], body: "", headers: {}) - expect { subject }.to raise_error(Webpush::InvalidSubscription) - - stub_request(:post, expected_endpoint). - to_return(status: 400, body: "", headers: {}) - expect { subject }.not_to raise_error(Webpush::InvalidSubscription) - end - - it 'raises ResponseError for unsuccessful status code by default' do - stub_request(:post, expected_endpoint). - to_return(status: 401, body: "", headers: {}) - - expect { subject }.to raise_error(Webpush::ResponseError) - end - - it 'raises exception on error by default' do - stub_request(:post, expected_endpoint).to_raise(StandardError) - - expect { subject }.to raise_error - end + include_examples 'web push protocol standard error handling' it 'message is optional' do expect(Webpush::Encryption).to_not receive(:encrypt) @@ -187,61 +220,7 @@ expect(result.code).to eql('201') end - it 'raises InvalidSubscription if and only if the combination of status code and message indicate an invalid subscription' do - stub_request(:post, expected_endpoint). - to_return(status: 410, body: "", headers: {}) - expect { subject }.to raise_error(Webpush::InvalidSubscription) - - stub_request(:post, expected_endpoint). - to_return(status: [400, "UnauthorizedRegistration"], body: "", headers: {}) - expect { subject }.to raise_error(Webpush::InvalidSubscription) - - stub_request(:post, expected_endpoint). - to_return(status: 400, body: "", headers: {}) - expect { subject }.not_to raise_error(Webpush::InvalidSubscription) - end - - it 'raises ExpiredSubscription if the API returns a 404 Error' do - stub_request(:post, expected_endpoint). - to_return(status: 404, body: "", headers: {}) - expect { subject }.to raise_error(Webpush::ExpiredSubscription) - end - - it 'raises PayloadTooLarge if the API returns a 413 Error' do - stub_request(:post, expected_endpoint). - to_return(status: 413, body: "", headers: {}) - expect { subject }.to raise_error(Webpush::PayloadTooLarge) - end - - it 'raises TooManyRequests if the API returns a 429 Error' do - stub_request(:post, expected_endpoint). - to_return(status: 429, body: "", headers: {}) - expect { subject }.to raise_error(Webpush::TooManyRequests) - end - - it 'raises ResponseError for unsuccessful status code by default' do - stub_request(:post, expected_endpoint). - to_return(status: 401, body: "", headers: {}) - - expect { subject }.to raise_error(Webpush::ResponseError) - end - - it 'supplies the original status code on the ResponseError' do - stub_request(:post, expected_endpoint). - to_return(status: 401, body: "Oh snap", headers: {}) - - expect { subject }.to raise_error { |error| - expect(error).to be_a(Webpush::ResponseError) - expect(error.response.code).to eq '401' - expect(error.response.body).to eq 'Oh snap' - } - end - - it 'raises exception on error by default' do - stub_request(:post, expected_endpoint).to_raise(StandardError) - - expect { subject }.to raise_error - end + include_examples 'web push protocol standard error handling' it 'message and encryption keys are optional' do expect(Webpush::Encryption).to_not receive(:encrypt) From ec2fad42ebf8ab8861a0dc17ba07a0b6e146804d Mon Sep 17 00:00:00 2001 From: Glenn Roberts Date: Fri, 16 Jun 2017 18:51:26 +1000 Subject: [PATCH 3/3] add spec coverage for the ResponseError message --- spec/webpush_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/webpush_spec.rb b/spec/webpush_spec.rb index d468744..50dc503 100644 --- a/spec/webpush_spec.rb +++ b/spec/webpush_spec.rb @@ -56,6 +56,19 @@ } end + it 'sets the error message to be the host + stringified response' do + stub_request(:post, expected_endpoint). + to_return(status: 401, body: "Oh snap", headers: {}) + + host = URI.parse(expected_endpoint).host + + expect { subject }.to raise_error { |error| + expect(error.message).to eq( + "host: #{host}, #\nbody:\nOh snap" + ) + } + end + it 'raises exception on error by default' do stub_request(:post, expected_endpoint).to_raise(StandardError)