Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add token exchange capability to FetchService #17678

Open
wants to merge 3 commits into
base: token-fetch-service
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) 2012-2024 the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module OpenIDConnect
module UserTokens
class ExchangeService
include Dry::Monads[:result]
include Dry::Monads::Do.for(:call)

class Disabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 This also hints into the direction of dependency injection.

So, depending on the condition, either the ExchangeService or the DisabledExchangeService is injected.

class << self
include Dry::Monads[:result]

def call(_) = Failure("Token exchange disabled")

def supported? = false
end
end

def initialize(user:)
@user = user
end

def call(audience)
return Failure("Provider does not support token exchange") unless supported?

idp_token = yield FetchService.new(user: @user, token_exchange: Disabled)
.access_token_for(audience: UserToken::IDP_AUDIENCE)

json = yield exchange_token_request(idp_token, audience)

access_token = json["access_token"]
refresh_token = json["refresh_token"]
return Failure("Token exchange response invalid") if access_token.blank?

token = store_exchanged_token(audience:, access_token:, refresh_token:)
Success(token)
end

def supported?
provider&.token_exchange_capable?
end

private

def exchange_token_request(access_token, audience)
response = OpenProject.httpx
.basic_auth(provider.client_id, provider.client_secret)
.post(provider.token_endpoint, form: {
grant_type: "urn:ietf:params:oauth:grant-type:token-exchange",
subject_token: access_token,
audience:
})
response.raise_for_status

Success(response.json)
rescue HTTPX::Error => e
Failure(e)
end

def store_exchanged_token(audience:, access_token:, refresh_token:)
token = @user.oidc_user_tokens.where("audiences ? :audience", audience:).first
if token
if token.audiences.size > 1
raise "Did not expect to update token with multiple audiences (#{token.audiences}) in-place."
end

token.update!(access_token:, refresh_token:)
else
token = @user.oidc_user_tokens.create!(access_token:, refresh_token:, audiences: [audience])
end

token
end

def provider
@user.authentication_provider
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,15 @@ module UserTokens
# client_id at an identity provider that OpenProject and the application have in common.
class FetchService
include Dry::Monads[:result]
include Dry::Monads::Do.for(:access_token_for, :refreshed_access_token_for)

def initialize(user:, jwt_parser: JwtParser.new(verify_audience: false, verify_expiration: false))
def initialize(user:,
jwt_parser: JwtParser.new(verify_audience: false, verify_expiration: false),
token_exchange: ExchangeService.new(user:),
token_refresh: RefreshService.new(user:, token_exchange:))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Seeing constructors like this makes me feel itchy. Should we consider any kind of IoC here? like Dry::AutoInject?

Also peeking into the direction of @mereghost

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair: I would have probably not added token_exchange and token_refresh as parameters at all, but implemented them as private methods and then in specs I'd have mocked the new method of them, but @mereghost told me having a dependency injecting constructor would be preferrable to that.

In this specific case, I was at least able to use the DI for production code as well, by implementing ExchangeService::Disabled, which is a way to avoid passing an additional enable_token_exchange parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked the code example of dry-autoinject. I understand that we use it in certain places (e.g. our storage queries), but to me it feels like a too big gun to use here ("Kanonen auf Spatzen").

If the problem we wanted to solve was, that it's somehow bad practice to mock internal implementation details of a class (e.g. knowing that it would internally call OtherService.new), then I can understand that making the injection of said class a part of the class's public interface (e.g. by passing OtherService.new as an argument).

However, I start to become skeptical, when the solution to now having to pass a class in via an argument, is to use a library that makes the internally used class a secret again (it would be "hidden" in a private method again), but allows to change it from another place (e.g. by changing the outside "container").

That at least does not contribute to clarity IMO. So the question is, which problem we are solving by doing that.

@user = user
@provider = user.authentication_provider
@token_exchange = token_exchange
@token_refresh = token_refresh
@jwt_parser = jwt_parser
end

Expand All @@ -51,17 +56,14 @@ def initialize(user:, jwt_parser: JwtParser.new(verify_audience: false, verify_e
# identified as being expired. There is no guarantee that all access tokens will be properly
# recognized as expired, so client's still need to make sure to handle rejected access tokens
# properly. Also see #refreshed_access_token_for.
#
# A token exchange is attempted, if the provider supports OAuth 2.0 Token Exchange and a token
# for the target audience either can't be found or it has expired, but has no available refresh token.
def access_token_for(audience:)
token = token_with_audience(audience)
token = token.bind do |t|
if expired?(t.access_token)
refresh(t)
else
Success(t)
end
end
token = yield token_with_audience(audience)
token = yield @token_refresh.call(token) if expired?(token.access_token)

token.fmap(&:access_token)
Success(token.access_token)
end

##
Expand All @@ -71,45 +73,24 @@ def access_token_for(audience:)
# The access token will always be refreshed before being returned by this method.
# It is advised to use this method, after learning that a remote service rejected
# an access token, because it was expired.
#
# A token exchange is attempted, if the provider supports OAuth 2.0 Token Exchange and a token
# for the target audience either can't be found or it has expired, but has no available refresh token.
def refreshed_access_token_for(audience:)
token_with_audience(audience)
.bind { |t| refresh(t) }
.fmap(&:access_token)
token = yield token_with_audience(audience)
token = yield @token_refresh.call(token)
Success(token.access_token)
end

private

def token_with_audience(aud)
token = @user.oidc_user_tokens.where("audiences ? :aud", aud:).first
token ? Success(token) : Failure("No token for given audience")
end

def refresh(token)
return Failure("Can't refresh the access token") if token.refresh_token.blank?

refresh_token_request(token.refresh_token).bind do |json|
access_token = json["access_token"]
refresh_token = json["refresh_token"]
break Failure("Refresh token response invalid") if access_token.blank?

token.update!(access_token:, refresh_token:)

Success(token)
end
end
return Success(token) if token

def refresh_token_request(refresh_token)
response = OpenProject.httpx
.basic_auth(@provider.client_id, @provider.client_secret)
.post(@provider.token_endpoint, form: {
grant_type: :refresh_token,
refresh_token:
})
response.raise_for_status
return @token_exchange.call(aud) if @token_exchange.supported?

Success(response.json)
rescue HTTPX::Error => e
Failure(e)
Failure("No token for audience '#{aud}'")
end

def expired?(token_string)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) 2012-2024 the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module OpenIDConnect
module UserTokens
class RefreshService
include Dry::Monads[:result]
include Dry::Monads::Do.for(:call)

def initialize(user:, token_exchange:)
@user = user
@token_exchange = token_exchange
end

def call(token)
if token.refresh_token.blank?
return exchange_instead_of_refresh(token)
end

json = yield refresh_token_request(token.refresh_token)

access_token = json["access_token"]
refresh_token = json["refresh_token"]
return Failure("Refresh token response invalid") if access_token.blank?

token.update!(access_token:, refresh_token:)

Success(token)
end

private

def exchange_instead_of_refresh(token)
# We can attempt a token exchange instead of a refresh, if we previously exchanged the token.
# For simplicity we do not consider scenarios where the original token had a wider audience,
# because all tokens obtained through exchange in this service will have exactly one audience.
if @token_exchange.supported? && token.audiences.size == 1
return @token_exchange.call(token.audiences.first)
end

Failure("Can't refresh the access token")
end

def refresh_token_request(refresh_token)
response = OpenProject.httpx
.basic_auth(provider.client_id, provider.client_secret)
.post(provider.token_endpoint, form: {
grant_type: :refresh_token,
refresh_token:
})
response.raise_for_status

Success(response.json)
rescue HTTPX::Error => e
Failure(e)
end

def provider
@user.authentication_provider
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
"authorization_endpoint" => "https://keycloak.local/realms/master/protocol/openid-connect/auth"
}
end

trait :token_exchange_capable do
callback(:after_build) do |provider|
provider.options["grant_types_supported"] ||= []
provider.options["grant_types_supported"] << "urn:ietf:params:oauth:grant-type:token-exchange"
end
end
end

factory :oidc_provider_google, class: "OpenIDConnect::Provider" do
Expand Down
Loading
Loading