Skip to content

Commit 8558d65

Browse files
davidtaylorhqdavidtaylorhq
committed
DEV: Additional tests, and improved JWT error handling
1 parent ba3685f commit 8558d65

File tree

2 files changed

+119
-40
lines changed

2 files changed

+119
-40
lines changed

‎lib/omniauth_open_id_connect.rb‎

Lines changed: 39 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -78,30 +78,26 @@ def token_params
7878
super.merge(params)
7979
end
8080

81-
uid { id_token_info['sub'] }
81+
def callback_phase
82+
if request.params["error"] && request.params["error_description"] && response = options.error_handler.call(request.params["error"], request.params["error_description"])
83+
return redirect(response)
84+
end
8285

83-
info do
84-
data_source = options.use_userinfo ? userinfo_response : id_token_info
85-
prune!(
86-
name: data_source['name'],
87-
email: data_source['email'],
88-
first_name: data_source['given_name'],
89-
last_name: data_source['family_name'],
90-
nickname: data_source['preferred_username'],
91-
picture: data_source['picture']
92-
)
93-
end
86+
begin
87+
discover! if options[:discovery]
9488

95-
extra do
96-
hash = {}
97-
hash[:raw_info] = options.use_userinfo ? userinfo_response : id_token_info
98-
prune! hash
99-
end
89+
oauth2_callback_phase = super
90+
return oauth2_callback_phase if env['omniauth.error']
10091

101-
def userinfo_response
102-
@raw_info ||= access_token.get(options[:client_options][:userinfo_endpoint]).parsed
103-
return fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) unless @raw_info['sub'] == id_token_info['sub']
104-
@raw_info
92+
if id_token_info["nonce"].nil? || id_token_info["nonce"].empty? || id_token_info["nonce"] != session.delete("omniauth.nonce")
93+
return fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
94+
end
95+
oauth2_callback_phase
96+
rescue ::OmniAuth::OpenIDConnect::DiscoveryError => e
97+
fail!(:openid_connect_discovery_error, e)
98+
rescue JWT::DecodeError => e
99+
fail!(:jwt_decode_failed, e)
100+
end
105101
end
106102

107103
def id_token_info
@@ -122,25 +118,30 @@ def id_token_info
122118
).first
123119
end
124120

125-
def callback_phase
126-
if request.params["error"] && request.params["error_description"] && response = options.error_handler.call(request.params["error"], request.params["error_description"])
127-
return redirect(response)
128-
end
129-
130-
begin
131-
discover! if options[:discovery]
132-
rescue ::OmniAuth::OpenIDConnect::DiscoveryError => e
133-
fail!(:openid_connect_discovery_error, e)
134-
end
121+
def userinfo_response
122+
@raw_info ||= access_token.get(options[:client_options][:userinfo_endpoint]).parsed
123+
return fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) unless @raw_info['sub'] == id_token_info['sub']
124+
@raw_info
125+
end
135126

136-
oauth2_callback_phase = super
127+
uid { id_token_info['sub'] }
137128

138-
return oauth2_callback_phase if env['omniauth.error']
129+
info do
130+
data_source = options.use_userinfo ? userinfo_response : id_token_info
131+
prune!(
132+
name: data_source['name'],
133+
email: data_source['email'],
134+
first_name: data_source['given_name'],
135+
last_name: data_source['family_name'],
136+
nickname: data_source['preferred_username'],
137+
picture: data_source['picture']
138+
)
139+
end
139140

140-
if id_token_info["nonce"].empty? || id_token_info["nonce"] != session.delete("omniauth.nonce")
141-
return fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
142-
end
143-
oauth2_callback_phase
141+
extra do
142+
hash = {}
143+
hash[:raw_info] = options.use_userinfo ? userinfo_response : id_token_info
144+
prune! hash
144145
end
145146

146147
private
@@ -169,7 +170,7 @@ def prune!(hash)
169170

170171
def build_access_token
171172
return super if options.use_userinfo
172-
response = client.request(:get, options[:client_options][:token_url], params: get_token_options)
173+
response = client.request(:post, options[:client_options][:token_url], body: get_token_options)
173174
::OAuth2::AccessToken.from_hash(client, response.parsed)
174175
end
175176

‎spec/lib/omniauth_open_id_connect_spec.rb‎

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
describe OmniAuth::Strategies::OpenIDConnect do
99
# let(:request) { double('Request', params: {}, cookies: {}, env: {}) }
1010
let(:app) do
11-
lambda do
11+
lambda do |*args|
1212
[200, {}, ['Hello.']]
1313
end
1414
end
@@ -107,9 +107,87 @@
107107
allow(subject).to receive(:request) { double("Request", params: {}) }
108108
expect(subject.token_params[:p]).to eq("someallowedvalue")
109109
end
110-
111110
end
112111

112+
describe "callback_phase" do
113+
before do
114+
auth_params = subject.authorize_params
115+
116+
allow(subject).to receive(:full_host).and_return("https://example.com")
117+
118+
allow(subject).to receive(:request) do
119+
double("Request", params: { "state" => auth_params[:state], "code" => "supersecretcode" })
120+
end
121+
122+
payload = {
123+
iss: "https://id.example.com/",
124+
sub: "someuserid",
125+
aud: "appid",
126+
iat: Time.now.to_i - 30,
127+
exp: Time.now.to_i + 120,
128+
nonce: auth_params[:nonce],
129+
name: "My Auth Token Name",
130+
email: "tokenemail@example.com"
131+
}
132+
@token = JWT.encode payload, nil, 'none'
133+
end
134+
135+
context "with userinfo disabled" do
136+
before do
137+
stub_request(:post, "https://id.example.com/token").
138+
with(body: hash_including("code" => "supersecretcode", "p" => "someallowedvalue")).
139+
to_return(status: 200, body: {
140+
"id_token": @token,
141+
}.to_json, headers: { "Content-Type" => "application/json" })
142+
143+
subject.options.use_userinfo = false
144+
end
145+
146+
it "fetches auth token correctly, and uses it for user info" do
147+
expect(subject.callback_phase[0]).to eq(200)
148+
expect(subject.uid).to eq("someuserid")
149+
expect(subject.info[:name]).to eq("My Auth Token Name")
150+
expect(subject.info[:email]).to eq("tokenemail@example.com")
151+
end
152+
153+
it "checks the nonce" do
154+
subject.session["omniauth.nonce"] = "overriddenNonce"
155+
expect(subject.callback_phase[0]).to eq(302)
156+
end
157+
158+
it "checks the issuer" do
159+
subject.options.client_id = "overriddenclientid"
160+
expect(subject.callback_phase[0]).to eq(302)
161+
end
162+
end
163+
164+
context "with userinfo enabled" do
165+
before do
166+
stub_request(:post, "https://id.example.com/token").
167+
with(body: hash_including("code" => "supersecretcode", "p" => "someallowedvalue")).
168+
to_return(status: 200, body: {
169+
"access_token": "AnAccessToken",
170+
"expires_in": 3600,
171+
"id_token": @token,
172+
}.to_json, headers: { "Content-Type" => "application/json" })
173+
174+
stub_request(:get, "https://id.example.com/userinfo").
175+
with(headers: { 'Authorization' => 'Bearer AnAccessToken' }).
176+
to_return(status: 200, body: {
177+
sub: "someuserid",
178+
name: "My Userinfo Name",
179+
email: "userinfoemail@example.com",
180+
}.to_json, headers: { "Content-Type" => "application/json" })
181+
end
182+
183+
it "fetches credentials and auth token correctly" do
184+
expect(subject.callback_phase[0]).to eq(200)
185+
expect(subject.uid).to eq("someuserid")
186+
expect(subject.info[:name]).to eq("My Userinfo Name")
187+
expect(subject.info[:email]).to eq("userinfoemail@example.com")
188+
end
189+
end
190+
end
113191
end
114192

115193
end

0 commit comments

Comments
 (0)