diff --git a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb index 7c392f5957c..573cddb809f 100644 --- a/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb +++ b/lib_static/open_project/authentication/strategies/warden/jwt_oidc.rb @@ -20,21 +20,14 @@ module OpenProject end def authenticate! - verified_payload, provider = ::OpenIDConnect::JwtParser.new(required_claims: ["sub"]).parse(@access_token) - - user = User.find_by(identity_url: "#{provider.slug}:#{verified_payload['sub']}") - success!(user) if user - rescue JWT::ExpiredSignature - fail_with_header!(error: "invalid_token", error_description: "The access token expired") - rescue JWT::ImmatureSignature - # happens when nbf time is less than current - fail_with_header!(error: "invalid_token", error_description: "The access token is used too early") - rescue JWT::InvalidAudError - fail_with_header!(error: "invalid_token", error_description: "The access token audience claim is wrong") - rescue JSON::JWK::Set::KidNotFound - fail_with_header!(error: "invalid_token", error_description: "The access token signature kid is unknown") - rescue ::OpenIDConnect::JwtParser::Error => e - fail_with_header!(error: "invalid_token", error_description: e.message) + ::OpenIDConnect::JwtParser.new(required_claims: ["sub"]).parse(@access_token).either( + ->(payload_and_provider) do + payload, provider = payload_and_provider + user = User.find_by(identity_url: "#{provider.slug}:#{payload['sub']}") + success!(user) if user + end, + ->(error) { fail_with_header!(error: "invalid_token", error_description: error) } + ) end end end diff --git a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb index 3c5f7da91e6..e1ece957b58 100644 --- a/modules/openid_connect/app/services/openid_connect/associate_user_token.rb +++ b/modules/openid_connect/app/services/openid_connect/associate_user_token.rb @@ -48,18 +48,20 @@ module OpenIDConnect @user.oidc_user_tokens.destroy_all if clear_previous - token = @user.oidc_user_tokens.build(access_token:, refresh_token:) - token.audiences = merge_audiences(known_audiences, discover_audiences(access_token)) + token = prepare_token(access_token:, refresh_token:, known_audiences:) token.save! if token.audiences.any? end private + def prepare_token(access_token:, refresh_token:, known_audiences:) + @user.oidc_user_tokens.build(access_token:, refresh_token:).tap do |token| + token.audiences = merge_audiences(known_audiences, discover_audiences(access_token).value_or([])) + end + end + def discover_audiences(access_token) - decoded, = @jwt_parser.parse(access_token) - Array(decoded["aud"]) - rescue StandardError - [] + @jwt_parser.parse(access_token).fmap { |decoded, _| Array(decoded["aud"]) } end def merge_audiences(*args) diff --git a/modules/openid_connect/app/services/openid_connect/jwt_parser.rb b/modules/openid_connect/app/services/openid_connect/jwt_parser.rb index f371e6e5f7d..ec789fcaa56 100644 --- a/modules/openid_connect/app/services/openid_connect/jwt_parser.rb +++ b/modules/openid_connect/app/services/openid_connect/jwt_parser.rb @@ -30,7 +30,7 @@ module OpenIDConnect class JwtParser - class Error < StandardError; end + include Dry::Monads[:result] SUPPORTED_JWT_ALGORITHMS = %w[ RS256 @@ -44,41 +44,41 @@ module OpenIDConnect end def parse(token) - issuer, alg, kid = parse_unverified_iss_alg_kid(token) - raise Error, "Token signature algorithm #{alg} is not supported" if SUPPORTED_JWT_ALGORITHMS.exclude?(alg) + parse_unverified_iss_alg_kid(token).bind do |issuer, alg, kid| + return Failure("Token signature algorithm #{alg} is not supported") if SUPPORTED_JWT_ALGORITHMS.exclude?(alg) - provider = fetch_provider(issuer) - raise Error, "The access token issuer is unknown" if provider.blank? + provider = fetch_provider(issuer) + return Failure("The access token issuer is unknown") if provider.blank? - jwks_uri = provider.jwks_uri - key = JSON::JWK::Set::Fetcher.fetch(jwks_uri, kid:).to_key + verified_payload, = JWT.decode( + token, + fetch_key(provider:, kid:), + true, + { + algorithm: alg, + verify_aud: @verify_audience, + aud: provider.client_id, + required_claims: all_required_claims + } + ) - verified_payload, = JWT.decode( - token, - key, - true, - { - algorithm: alg, - verify_aud: @verify_audience, - aud: provider.client_id, - required_claims: all_required_claims - } - ) - - [verified_payload, provider] + Success([verified_payload, provider]) + rescue JWT::DecodeError => e + Failure(e.message) + rescue JSON::JWK::Set::KidNotFound + Failure("The signature key ID is unknown") + end end private def parse_unverified_iss_alg_kid(token) unverified_payload, unverified_header = JWT.decode(token, nil, false) - raise Error, "The token's Key Identifier (kid) is missing" unless unverified_header.key?("kid") + return Failure("The token's Key Identifier (kid) is missing") unless unverified_header.key?("kid") - [ - unverified_payload["iss"], - unverified_header.fetch("alg"), - unverified_header.fetch("kid") - ] + Success([unverified_payload["iss"], unverified_header.fetch("alg"), unverified_header.fetch("kid")]) + rescue JWT::DecodeError => e + Failure(e.message) end def fetch_provider(issuer) @@ -87,6 +87,10 @@ module OpenIDConnect OpenIDConnect::Provider.where(available: true).where("options->>'issuer' = ?", issuer).first end + def fetch_key(provider:, kid:) + JSON::JWK::Set::Fetcher.fetch(provider.jwks_uri, kid:).to_key + end + def all_required_claims claims = ["iss"] + @required_claims claims << "aud" if @verify_audience diff --git a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb index aa7db97e443..d0f8fa8694b 100644 --- a/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb +++ b/modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb @@ -39,7 +39,7 @@ RSpec.describe OpenIDConnect::AssociateUserToken do let(:access_token) { "access-token-foo" } let(:refresh_token) { "refresh-token-bar" } - let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: [parsed_jwt, nil]) } + let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: Success([parsed_jwt, nil])) } let(:parsed_jwt) { { "aud" => ["aud1", "aud2"] } } before do @@ -80,9 +80,7 @@ RSpec.describe OpenIDConnect::AssociateUserToken do end context "when the access token is not a valid JWT" do - before do - allow(parser).to receive(:parse).and_raise("Oops, not a JWT!") - end + let(:parser) { instance_double(OpenIDConnect::JwtParser, parse: Failure("Oops, not a JWT!")) } it "creates a correct user token", :aggregate_failures do expect { subject }.to change(OpenIDConnect::UserToken, :count).by(1) diff --git a/modules/openid_connect/spec/services/openid_connect/jwt_parser_spec.rb b/modules/openid_connect/spec/services/openid_connect/jwt_parser_spec.rb index d22156b0e0a..21fcb28a65a 100644 --- a/modules/openid_connect/spec/services/openid_connect/jwt_parser_spec.rb +++ b/modules/openid_connect/spec/services/openid_connect/jwt_parser_spec.rb @@ -51,13 +51,15 @@ RSpec.describe OpenIDConnect::JwtParser do provider.save! end + it { is_expected.to be_success } + it "parses the token" do - parsed, = parse + parsed, = parse.value! expect(parsed).to eq payload end it "returns the provider configuration for the associated provider" do - _, p = parse + _, p = parse.value! expect(p).to eq provider end @@ -70,8 +72,10 @@ RSpec.describe OpenIDConnect::JwtParser do context "when the provider signing the token is not known" do let(:known_issuer) { "Lunar Gateway" } - it "raises an error" do - expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /issuer is unknown/) + it { is_expected.to be_failure } + + it "indicates the problem" do + expect(parse.failure).to match(/issuer is unknown/) end end @@ -80,24 +84,26 @@ RSpec.describe OpenIDConnect::JwtParser do provider.update!(available: false) end - it "raises an error" do - expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /issuer is unknown/) + it { is_expected.to be_failure } + + it "indicates the problem" do + expect(parse.failure).to match(/issuer is unknown/) end end context "when the token is not a valid JWT" do let(:token) { Base64.encode64("banana").strip } - it "raises an error" do - expect { parse }.to raise_error(JWT::DecodeError) - end + it { is_expected.to be_failure } end context "when the token is signed using an unsupported signature" do let(:token) { JWT.encode(payload, "secret", "HS256", { kid: "key-identifier" }) } - it "raises an error" do - expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /HS256 is not supported/) + it { is_expected.to be_failure } + + it "indicates the problem" do + expect(parse.failure).to match(/HS256 is not supported/) end end @@ -106,44 +112,36 @@ RSpec.describe OpenIDConnect::JwtParser do payload["aud"] = "Alice" end - it "raises an error" do - expect { parse }.to raise_error(JWT::InvalidAudError) - end + it { is_expected.to be_failure } context "and the audience shall not be verified" do subject(:parse) { described_class.new(verify_audience: false).parse(token) } - it "parses the token" do - parsed, = parse - expect(parsed).to eq payload - end + it { is_expected.to be_success } end end context "when the token does not indicate a Key Identifier" do let(:token) { JWT.encode(payload, private_key, "RS256") } - it "raises an error" do - expect { parse }.to raise_error(OpenIDConnect::JwtParser::Error, /Key Identifier .+ is missing/) + it { is_expected.to be_failure } + + it "indicates the problem" do + expect(parse.failure).to match(/Key Identifier .+ is missing/) end end context "when requiring a specific claim" do subject(:parse) { described_class.new(required_claims: ["sub"]).parse(token) } - it "parses the token" do - parsed, = parse - expect(parsed).to eq payload - end + it { is_expected.to be_success } context "and when the required claim is missing" do before do payload.delete("sub") end - it "raises an error" do - expect { parse }.to raise_error(JWT::MissingRequiredClaim) - end + it { is_expected.to be_failure } end end end diff --git a/spec/requests/api/v3/authentication_spec.rb b/spec/requests/api/v3/authentication_spec.rb index 4bc41dadf22..5b2a95a5f80 100644 --- a/spec/requests/api/v3/authentication_spec.rb +++ b/spec/requests/api/v3/authentication_spec.rb @@ -467,13 +467,13 @@ RSpec.describe "API V3 Authentication" do context "when access token has not expired yet" do context "when aud does not contain client_id" do - let(:token_aud) { ["master-realm", "account"] } + let(:token_aud) { ["Lisa", "Bart"] } it do get resource expect(last_response).to have_http_status :unauthorized - error = "The access token audience claim is wrong" + error = 'Invalid audience. Expected https://openproject.local, received ["Lisa", "Bart"]' expect(last_response.header["WWW-Authenticate"]) .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="#{error}"}) expect(JSON.parse(last_response.body)).to eq(error_response_body) @@ -497,7 +497,7 @@ RSpec.describe "API V3 Authentication" do expect(last_response).to have_http_status :unauthorized expect(last_response.header["WWW-Authenticate"]) - .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="The access token expired"}) + .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="Signature has expired"}) expect(JSON.parse(last_response.body)).to eq(error_response_body) end @@ -531,7 +531,7 @@ RSpec.describe "API V3 Authentication" do get resource expect(last_response).to have_http_status :unauthorized expect(JSON.parse(last_response.body)).to eq(error_response_body) - error = "The access token signature kid is unknown" + error = "The signature key ID is unknown" expect(last_response.header["WWW-Authenticate"]) .to eq(%{Bearer realm="OpenProject API", error="invalid_token", error_description="#{error}"}) end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c0fdf04017d..62ae6183075 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -54,4 +54,5 @@ RSpec.configure do |config| # Have the FactoryBot methods like #create and #build_stubbed without # having to call it on FactoryBot. config.include FactoryBot::Syntax::Methods + config.include Dry::Monads[:result] end