Skip to content

Commit

Permalink
Fix auto-discovery of OIDC details to be too eager
Browse files Browse the repository at this point in the history
For custom providers it was not possible to overwrite
the discovered default settings, because the discovery would be
re-triggered every time that any update was performed,
thereby overwriting the custom settings just entered.

The only way out so far was to completely remove the metadata_url
from the provider.

For custom providers the discovery now only takes place on the
form where the metadata_url is being entered, not on other forms.
For Google and Microsoft discovery remains active on all forms.
Due to further implementation details this means that on creation
they will first fetch their metadata when saving the client details
form, while during edit they will also refetch on the name form
(where Entra allows changing the tenant, which influences the URLs).
  • Loading branch information
NobodysNightmare committed Dec 9, 2024
1 parent 4d7bff2 commit 526ff9f
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

module OpenIDConnect::Providers::Sections
class FormComponent < ::Saml::Providers::Sections::SectionComponent
attr_reader :edit_state, :next_edit_state, :edit_mode
attr_reader :edit_state, :next_edit_state, :edit_mode, :fetch_metadata

def initialize(provider,
edit_state:,
Expand All @@ -39,7 +39,8 @@ def initialize(provider,
banner: nil,
banner_scheme: :default,
next_edit_state: nil,
edit_mode: nil)
edit_mode: nil,
fetch_metadata: false)
super(provider)

@edit_state = edit_state
Expand All @@ -49,6 +50,7 @@ def initialize(provider,
@heading = heading
@banner = banner
@banner_scheme = banner_scheme
@fetch_metadata = fetch_metadata
end

def url
Expand All @@ -61,9 +63,9 @@ def url

def form_url_params
if edit_mode
{ edit_state:, edit_mode:, next_edit_state: }
{ edit_state:, fetch_metadata:, edit_mode:, next_edit_state: }
else
{ edit_state: }
{ edit_state:, fetch_metadata: }
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
edit_state:,
next_edit_state: :client_details,
edit_mode:,
heading: nil
heading: nil,
fetch_metadata: true
)
else
OpenIDConnect::Providers::Sections::ShowComponent.new(
Expand All @@ -44,6 +45,7 @@
edit_state:,
edit_mode:,
heading: nil,
fetch_metadata: true
))
else
render(OpenIDConnect::Providers::Sections::ShowComponent.new(
Expand All @@ -66,7 +68,8 @@
edit_state:,
next_edit_state: :client_details,
edit_mode:,
heading: nil
heading: nil,
fetch_metadata: true
)
else
OpenIDConnect::Providers::Sections::ShowComponent.new(
Expand All @@ -87,7 +90,8 @@
form_class: OpenIDConnect::Providers::ClientDetailsForm,
edit_state:,
edit_mode:,
heading: nil
heading: nil,
fetch_metadata: true
))
else
render(OpenIDConnect::Providers::Sections::ShowComponent.new(
Expand Down Expand Up @@ -137,7 +141,8 @@
heading: nil,
edit_state:,
edit_mode:,
next_edit_state: :metadata_details
next_edit_state: :metadata_details,
fetch_metadata: true
))
else
render(OpenIDConnect::Providers::Sections::ShowComponent.new(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def update
.permit(:display_name, :oidc_provider, :limit_self_registration,
*OpenIDConnect::Provider.stored_attributes[:options])
call = OpenIDConnect::Providers::UpdateService
.new(model: @provider, user: User.current)
.new(model: @provider, user: User.current, fetch_metadata: fetch_metadata?)
.call(update_params)

if call.success?
Expand Down Expand Up @@ -162,5 +162,9 @@ def set_edit_state
@edit_mode = ActiveRecord::Type::Boolean.new.cast(params[:edit_mode])
@next_edit_state = params[:next_edit_state].to_sym if params.key?(:next_edit_state)
end

def fetch_metadata?
params[:fetch_metadata] == "true"
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,16 @@ class AttributesContract < Dry::Validation::Contract
end
end

def initialize(*, fetch_metadata: false, **)
super(*, **)

@fetch_metadata = fetch_metadata
end

def after_validate(_params, call)
model = call.result
metadata_url = get_metadata_url(model)
return call if metadata_url.blank?
return call if metadata_url.blank? || !@fetch_metadata

extract_metadata(call, metadata_url, model)
end
Expand Down

0 comments on commit 526ff9f

Please sign in to comment.