diff --git a/lib/omniauth/strategies/codebar.rb b/lib/omniauth/strategies/codebar.rb index 6a344fa5e..4a52bd314 100644 --- a/lib/omniauth/strategies/codebar.rb +++ b/lib/omniauth/strategies/codebar.rb @@ -83,12 +83,19 @@ def callback_phase return fail!(:invalid_jwt, StandardError.new('JWT verification failed')) end - # The planner resolves members by email; a token without an email claim - # must not fall back to `sub` (the better-auth user id) — keying a member - # on it creates an account with no subscriptions or roles. - email = payload['email'] + # The planner resolves members by email, and since better-auth 1.7 only + # the UserInfo response carries it. A failed request fails the callback + # as :userinfo_failed (provider trouble) and a blank email as + # :missing_email: keying a member on `sub` (the better-auth user id) + # creates an account with no subscriptions or roles. + userinfo = fetch_userinfo(tokens['access_token']) + if userinfo.nil? + return fail!(:userinfo_failed, StandardError.new('UserInfo request failed')) + end + + email = userinfo['email'] if email.blank? - return fail!(:missing_email, StandardError.new('id_token has no email claim')) + return fail!(:missing_email, StandardError.new('UserInfo response has no email')) end # Build omniauth.auth hash @@ -97,7 +104,7 @@ def callback_phase uid: email, info: { email:, - name: payload['name'] || email + name: userinfo['name'].presence || payload['name'].presence || email }, credentials: { token: tokens['access_token'], @@ -171,6 +178,33 @@ def exchange_code(code, code_verifier) nil end + # Fetch the OIDC UserInfo response for an access token. The scope-gated + # claims there (`email`, `name`) are the planner's identity source because + # the id_token no longer carries them. + def fetch_userinfo(access_token) + uri = URI("#{options.auth_url}/api/auth/oauth2/userinfo") + request = Net::HTTP::Get.new(uri.path) + request['Authorization'] = "Bearer #{access_token}" + request['User-Agent'] = 'Codebar Planner/1.0' + + response = http_for(uri).request(request) + + if response.code.to_i == 200 + parsed = JSON.parse(response.body) + return parsed if parsed.is_a?(Hash) + + Rails.logger.warn 'Codebar auth: userinfo response is not a JSON object' + else + Rails.logger.warn "Codebar auth: userinfo fetch returned HTTP #{response.code}" + end + nil + rescue Net::OpenTimeout, Net::ReadTimeout, SocketError, Errno::ECONNREFUSED, + Errno::ECONNRESET, Errno::EHOSTUNREACH, Errno::ENETUNREACH, Errno::ETIMEDOUT, + Errno::EPIPE, EOFError, OpenSSL::SSL::SSLError, Net::HTTPBadResponse, JSON::ParserError => e + Rails.logger.warn "Codebar auth: userinfo fetch failed: #{e.class}: #{e.message}" + nil + end + # Verify JWT signature using auth app's JWKS. def verify_jwt(token) jwks = fetch_jwks diff --git a/spec/lib/omniauth/strategies/codebar_spec.rb b/spec/lib/omniauth/strategies/codebar_spec.rb index a15ab3e68..4ab271192 100644 --- a/spec/lib/omniauth/strategies/codebar_spec.rb +++ b/spec/lib/omniauth/strategies/codebar_spec.rb @@ -190,17 +190,27 @@ def build_env(path, query: '', session: {}) describe 'successful callback' do let(:rsa_key) { OpenSSL::PKey::RSA.generate(2048) } let(:jwk) { JWT::JWK.new(rsa_key, { kid: 'test-key-1' }) } + let(:userinfo_url) { "#{auth_url}/api/auth/oauth2/userinfo" } + # Sparse id_token: since better-auth 1.7 the email and name live in the + # UserInfo response, not in the token. let(:token_payload) do { 'sub' => 'better-auth-user-id', - 'email' => email, - 'name' => name, + 'github_id' => '4242', 'iss' => auth_url, 'aud' => 'planner', 'iat' => Time.now.to_i, 'exp' => Time.now.to_i + 3600 } end + let(:userinfo_body) do + { + 'sub' => 'better-auth-user-id', + 'email' => email, + 'email_verified' => true, + 'name' => name + } + end let(:id_token) do JWT.encode(token_payload, rsa_key, 'RS256', { kid: 'test-key-1' }) end @@ -224,9 +234,13 @@ def build_env(path, query: '', session: {}) stub_request(:get, jwks_url) .with(headers: { 'User-Agent' => 'Codebar Planner/1.0' }) .to_return(status: 200, body: { keys: [jwk.export] }.to_json, headers: { 'Content-Type' => 'application/json' }) + + stub_request(:get, userinfo_url) + .with(headers: { 'Authorization' => 'Bearer test-access-token', 'User-Agent' => 'Codebar Planner/1.0' }) + .to_return(status: 200, body: userinfo_body.to_json, headers: { 'Content-Type' => 'application/json' }) end - it 'builds the auth hash with correct data' do + it 'builds the auth hash from the userinfo response' do strategy.call!(callback_env) auth_hash = callback_env['omniauth.auth'] @@ -236,70 +250,169 @@ def build_env(path, query: '', session: {}) expect(auth_hash[:info][:email]).to eq(email) expect(auth_hash[:info][:name]).to eq(name) expect(auth_hash[:credentials][:token]).to eq('test-access-token') - expect(auth_hash[:extra][:raw_info]).to include('sub' => 'better-auth-user-id', 'email' => email, 'name' => name) + expect(auth_hash[:extra][:raw_info]).to include('sub' => 'better-auth-user-id', 'github_id' => '4242') end - describe 'with no name claim' do - let(:token_payload) do - { - 'sub' => 'better-auth-user-id', - 'email' => email, - 'iss' => auth_url, - 'aud' => 'planner', - 'iat' => Time.now.to_i, - 'exp' => Time.now.to_i + 3600 - } - end + describe 'email source' do + it 'takes the email from userinfo even when the id_token carries a different one' do + stub_request(:post, token_url) + .to_return(status: 200, body: { + access_token: 'test-access-token', + id_token: JWT.encode(token_payload.merge('email' => 'token-level@example.com'), rsa_key, 'RS256', { kid: 'test-key-1' }), + token_type: 'Bearer', + expires_in: 900 + }.to_json, headers: { 'Content-Type' => 'application/json' }) - it 'falls back to the email for name' do strategy.call!(callback_env) - expect(callback_env['omniauth.error.type']).to be_nil - expect(callback_env['omniauth.auth'][:info][:name]).to eq(email) + expect(callback_env['omniauth.auth'][:uid]).to eq(email) + expect(callback_env['omniauth.auth'][:info][:email]).to eq(email) end - end - describe 'with no email claim' do - let(:token_payload) do - { - 'sub' => 'better-auth-user-id', - 'name' => name, - 'iss' => auth_url, - 'aud' => 'planner', - 'iat' => Time.now.to_i, - 'exp' => Time.now.to_i + 3600 - } + it 'fails with missing_email when userinfo has no email, ignoring the id_token claim' do + stub_request(:post, token_url) + .to_return(status: 200, body: { + access_token: 'test-access-token', + id_token: JWT.encode(token_payload.merge('email' => 'token-level@example.com'), rsa_key, 'RS256', { kid: 'test-key-1' }), + token_type: 'Bearer', + expires_in: 900 + }.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, userinfo_url) + .to_return(status: 200, body: { 'sub' => 'better-auth-user-id' }.to_json, headers: { 'Content-Type' => 'application/json' }) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:missing_email) + expect(callback_env['omniauth.auth']).to be_nil end - it 'fails with missing_email and builds no auth hash' do + it 'fails with missing_email when the userinfo email is blank' do + stub_request(:get, userinfo_url) + .to_return(status: 200, body: userinfo_body.merge('email' => '').to_json, headers: { 'Content-Type' => 'application/json' }) + strategy.call!(callback_env) expect(callback_env['omniauth.error.type']).to eq(:missing_email) expect(callback_env['omniauth.auth']).to be_nil end + end - it 'returns the failure response from the middleware instead of a nil Rack response' do - response = strategy.call(callback_env) + describe 'name source' do + it 'falls back to the id_token name when userinfo has none' do + stub_request(:post, token_url) + .to_return(status: 200, body: { + access_token: 'test-access-token', + id_token: JWT.encode(token_payload.merge('name' => name), rsa_key, 'RS256', { kid: 'test-key-1' }), + token_type: 'Bearer', + expires_in: 900 + }.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, userinfo_url) + .to_return(status: 200, body: userinfo_body.except('name').to_json, headers: { 'Content-Type' => 'application/json' }) - expect(response).to be_a(Array) - expect(response[0]).to eq(302) - expect(response[1]['Location']).to start_with('/auth/failure?') + strategy.call!(callback_env) + + expect(callback_env['omniauth.auth'][:info][:name]).to eq(name) + end + + it 'falls back to the email when neither carries a name' do + stub_request(:get, userinfo_url) + .to_return(status: 200, body: userinfo_body.except('name').to_json, headers: { 'Content-Type' => 'application/json' }) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.auth'][:info][:name]).to eq(email) end - it 'also fails when the email claim is present but blank' do + it 'treats a blank userinfo name as missing' do stub_request(:post, token_url) .to_return(status: 200, body: { access_token: 'test-access-token', - id_token: JWT.encode(token_payload.merge('email' => ''), rsa_key, 'RS256', { kid: 'test-key-1' }), + id_token: JWT.encode(token_payload.merge('name' => name), rsa_key, 'RS256', { kid: 'test-key-1' }), token_type: 'Bearer', expires_in: 900 }.to_json, headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, userinfo_url) + .to_return(status: 200, body: userinfo_body.merge('name' => '').to_json, headers: { 'Content-Type' => 'application/json' }) strategy.call!(callback_env) - expect(callback_env['omniauth.error.type']).to eq(:missing_email) + expect(callback_env['omniauth.auth'][:info][:name]).to eq(name) + end + end + + describe 'when the userinfo request fails' do + it 'fails with userinfo_failed when userinfo returns 500' do + stub_request(:get, userinfo_url).to_return(status: 500) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'fails with userinfo_failed when userinfo rejects the access token' do + stub_request(:get, userinfo_url).to_return(status: 401) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'fails with userinfo_failed when userinfo times out' do + stub_request(:get, userinfo_url).to_timeout + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'fails with userinfo_failed when the connection resets' do + stub_request(:get, userinfo_url).to_raise(Errno::ECONNRESET) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'fails with userinfo_failed when userinfo returns invalid JSON' do + stub_request(:get, userinfo_url).to_return(status: 200, body: 'not json') + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) expect(callback_env['omniauth.auth']).to be_nil end + + it 'fails with userinfo_failed when the userinfo body is an array instead of an object' do + stub_request(:get, userinfo_url).to_return(status: 200, body: '[]', headers: { 'Content-Type' => 'application/json' }) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'fails with userinfo_failed when the userinfo body is null instead of an object' do + stub_request(:get, userinfo_url).to_return(status: 200, body: 'null', headers: { 'Content-Type' => 'application/json' }) + + strategy.call!(callback_env) + + expect(callback_env['omniauth.error.type']).to eq(:userinfo_failed) + expect(callback_env['omniauth.auth']).to be_nil + end + + it 'returns the failure response from the middleware instead of a nil Rack response' do + stub_request(:get, userinfo_url).to_return(status: 500) + + response = strategy.call(callback_env) + + expect(response).to be_a(Array) + expect(response[0]).to eq(302) + expect(response[1]['Location']).to start_with('/auth/failure?') + end end end