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

Update OIDC configuration UI #16935

Merged
merged 41 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
e0c5dd3
Update OIDC configuration UI.
ba1ash Oct 11, 2024
2e22ab2
Remove has_actions
oliverguenther Oct 14, 2024
db82830
Extend form validation
oliverguenther Oct 14, 2024
ef21f6e
Extract metadata_url getter
oliverguenther Oct 14, 2024
e857bcd
Add mapping
oliverguenther Oct 14, 2024
19eab3b
Same button layout as saml
oliverguenther Oct 14, 2024
77b8da4
Don't walk through sections without edit_mode
oliverguenther Oct 14, 2024
a1dd527
Hash builder with mapping
oliverguenther Oct 14, 2024
4e897fb
Allow custom icon
oliverguenther Oct 14, 2024
980dd1c
Move migration to engine
oliverguenther Oct 14, 2024
3b4ccfd
Format migration text
oliverguenther Oct 14, 2024
533312b
Configuration mapper
oliverguenther Oct 14, 2024
30fae2b
Rename seeder
oliverguenther Oct 14, 2024
7247a23
Remove form_post security impact option
oliverguenther Oct 14, 2024
a72c0cb
Allow seeding with path config as shown in docs
oliverguenther Oct 15, 2024
15490f2
Reintroduce sections, add one for mapping
oliverguenther Oct 15, 2024
7ecd5df
Fix inflection
oliverguenther Oct 15, 2024
7c873eb
Extend spec to use absolute URL
oliverguenther Oct 15, 2024
6fe5456
Feature spec
oliverguenther Oct 15, 2024
7b8bbee
Contract specs
oliverguenther Oct 15, 2024
fc4908c
Config mapper spec
oliverguenther Oct 15, 2024
2f3e7da
Service specs
oliverguenther Oct 15, 2024
524a459
Add claims
oliverguenther Oct 16, 2024
dcc293c
Remove metadata_url from discoverable attribute check
oliverguenther Oct 16, 2024
3312cc9
Allow path based attributes after all
oliverguenther Oct 16, 2024
718d934
Fix generation of provider classes from new config
oliverguenther Oct 16, 2024
d0390a2
Skip metadata check for built-in
oliverguenther Oct 16, 2024
46eb11a
Remove unused providers helper
oliverguenther Oct 16, 2024
d5987fa
Remove state lambda
oliverguenther Oct 16, 2024
0959dca
Set default issuer
oliverguenther Oct 16, 2024
15dcc76
Allow setting omniauth direct login provider to the new auth providers
oliverguenther Oct 16, 2024
7989cdb
Show delete warning for deleting SSO providers (#16981)
oliverguenther Oct 17, 2024
5bb581a
Parse tenant from previous config
oliverguenther Oct 17, 2024
343063a
Parse limit_self_registration
oliverguenther Oct 17, 2024
e9ef120
Also map host
oliverguenther Oct 17, 2024
32d4da1
Fix and add test for self-registration
oliverguenther Oct 18, 2024
65fb080
Re-enable custom attribute mapping spec
oliverguenther Oct 18, 2024
0d15a67
Add post_logout_redirect_uri
oliverguenther Oct 21, 2024
32afae1
Allow admin mapping
oliverguenther Oct 21, 2024
98e9d57
Add migration spec
oliverguenther Oct 21, 2024
6ac6af4
Better option mapping
oliverguenther Oct 21, 2024
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
1 change: 1 addition & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ RSpec/DescribeMethod:
# to match the exact file name
RSpec/SpecFilePathFormat:
CustomTransform:
OpenIDConnect: openid_connect
OAuthClients: oauth_clients
IgnoreMethods: true

Expand Down
10 changes: 5 additions & 5 deletions app/components/op_primer/border_box_table_component.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ See COPYRIGHT and LICENSE files for more details.

if rows.empty?
component.with_row(scheme: :default) { render_blank_slate }
end

rows.each do |row|
component.with_row(scheme: :default) do
render(row_class.new(row:, table: self))
else
rows.each do |row|
component.with_row(scheme: :default) do
render(row_class.new(row:, table: self))
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def password_change_not_possible(user)
if user.ldap_auth_source
user.ldap_auth_source.name
else
user.authentication_provider
user.human_authentication_provider
end
open_project_headers "Type" => "Account"

Expand Down
18 changes: 18 additions & 0 deletions app/models/auth_provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,20 @@ class AuthProvider < ApplicationRecord
validates :display_name, presence: true
validates :display_name, uniqueness: true

after_destroy :unset_direct_provider

def self.slug_fragment
raise NotImplementedError
end

def user_count
@user_count ||= User.where("identity_url LIKE ?", "#{slug}%").count
end

def human_type
raise NotImplementedError
end

def auth_url
root_url = OpenProject::StaticRouting::StaticUrlHelpers.new.root_url
URI.join(root_url, "auth/#{slug}/").to_s
Expand All @@ -44,4 +54,12 @@ def auth_url
def callback_url
URI.join(auth_url, "callback").to_s
end

protected

def unset_direct_provider
if Setting.omniauth_direct_login_provider == slug
Setting.omniauth_direct_login_provider = ""
end
end
end
7 changes: 6 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,12 @@ def name(formatter = nil)
def authentication_provider
return if identity_url.blank?

identity_url.split(":", 2).first.titleize
identity_url.split(":", 2).first
end

# Return user's authentication provider for display
def human_authentication_provider
authentication_provider&.titleize
end

##
Expand Down
21 changes: 21 additions & 0 deletions app/views/admin/settings/authentication_settings/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,27 @@ See COPYRIGHT and LICENSE files for more details.
<%= render Settings::NumericSettingComponent.new("invitation_expiration_days", unit: "days") %>
</fieldset>

<fieldset class="form--fieldset">
<legend class="form--fieldset-legend"><%= I18n.t(:'settings.authentication.single_sign_on') %></legend>
<div class="form--field">
<% providers = AuthProvider
Copy link
Member

Choose a reason for hiding this comment

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

These 6 lines should really go into a helper and not live in a template.

.where(available: true)
.order("lower(display_name) ASC")
.select(:type, :display_name, :slug)
.to_a
.map { |p| ["#{p.display_name} (#{p.human_type})", p.slug] }
%>
<%= setting_select :omniauth_direct_login_provider,
[[t(:label_disabled), ""]] + providers,
container_class: '-middle' %>
<span class="form--field-instructions">
<%= t("settings.authentication.omniauth_direct_login_hint_html",
internal_path: internal_signin_url) %>
</span>
</div>
</fieldset>


<fieldset class="form--fieldset">
<fieldset id="registration_footer" class="form--fieldset">
<legend class="form--fieldset-legend"><%= I18n.t(:setting_registration_footer) %></legend>
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/form/authentication/_external.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<div class="form--field -reduced-margin">
<%= styled_label_tag nil, I18n.t('user.authentication_provider') %>
<div class="form--field-container">
<%= @user.authentication_provider %>
<%= @user.human_authentication_provider %>
</div>
</div>
<div class="form--field-instructions">
Expand Down
2 changes: 2 additions & 0 deletions config/initializers/zeitwerk.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
"OAuth#{default_inflect($1, abspath)}"
when /\A(.*)_oauth\z/
"#{default_inflect($1, abspath)}OAuth"
when "openid_connect"
"OpenIDConnect"
when "oauth"
"OAuth"
when /\Aclamav_(.*)\z/
Expand Down
13 changes: 13 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,7 @@ en:
not_a_datetime: "is not a valid date time."
not_a_number: "is not a number."
not_allowed: "is invalid because of missing permissions."
not_json: "is not a valid JSON object."
not_an_integer: "is not an integer."
not_an_iso_date: "is not a valid date. Required format: YYYY-MM-DD."
not_same_project: "doesn't belong to the same project."
Expand Down Expand Up @@ -2141,6 +2142,7 @@ en:
label_api_doc: "API documentation"
label_backup: "Backup"
label_backup_code: "Backup code"
label_basic_details: "Basic details"
label_between: "between"
label_blocked_by: "blocked by"
label_blocks: "blocks"
Expand Down Expand Up @@ -2374,6 +2376,7 @@ en:
label_custom_favicon: "Custom favicon"
label_custom_touch_icon: "Custom touch icon"
label_logout: "Sign out"
label_mapping_for: "Mapping for: %{attribute}"
label_main_menu: "Side Menu"
label_manage: "Manage"
label_manage_groups: "Manage groups"
Expand Down Expand Up @@ -3340,7 +3343,9 @@ en:
setting_default_language: "Default language"
setting_default_projects_modules: "Default enabled modules for new projects"
setting_default_projects_public: "New projects are public by default"
setting_disable_password_login: "Disable password authentication"
setting_diff_max_lines_displayed: "Max number of diff lines displayed"
setting_omniauth_direct_login_provider: "Direct login SSO provider"
setting_display_subprojects_work_packages: "Display subprojects work packages on main projects by default"
setting_duration_format: "Duration format"
setting_duration_format_hours_only: "Hours only"
Expand Down Expand Up @@ -3444,6 +3449,14 @@ en:
setting_working_days: "Working days"

settings:
authentication:
single_sign_on: "Single Sign-On"
omniauth_direct_login_hint_html: >
If this option is active, login requests will redirect to the configured omniauth provider.
The login dropdown and sign-in page will be disabled.
<br/>
<strong>Note:</strong> Unless you also disable password logins, with this option enabled,
users can still log in internally by visiting the <code>%{internal_path}</code> login page.
attachments:
whitelist_text_html: >
Define a list of valid file extensions and/or mime types for uploaded files.
Expand Down
9 changes: 9 additions & 0 deletions lib/open_project/static/links.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,15 @@ def static_links
sysadmin_docs: {
saml: {
href: "https://www.openproject.org/docs/system-admin-guide/authentication/saml/"
},
oidc: {
href: "https://www.openproject.org/docs/installation-and-operations/misc/custom-openid-connect-providers/"
},
oidc_claims: {
href: "https://www.openproject.org/docs/installation-and-operations/misc/custom-openid-connect-providers/#claims"
},
oidc_acr_values: {
href: "https://www.openproject.org/docs/installation-and-operations/misc/custom-openid-connect-providers/#non-essential-claims"
}
},
storage_docs: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ See COPYRIGHT and LICENSE files for more details.

++#%>

<% OpenProject::Plugins::AuthPlugin.providers.each do |pro| %>
<% OpenProject::Plugins::AuthPlugin.providers.each do |provider| %>
<%
opts = { script_name: OpenProject::Configuration.rails_relative_url_root }

Expand All @@ -36,8 +36,8 @@ See COPYRIGHT and LICENSE files for more details.
end
%>
<a
href="<%= omni_auth_start_path(pro[:name], opts) %>"
class="auth-provider auth-provider-<%= pro[:name] %> <%= pro[:icon] ? 'auth-provider--imaged' : '' %> button">
<span class="auth-provider-name"><%= pro[:display_name] || pro[:name] %></span>
href="<%= omni_auth_start_path(provider[:name], opts) %>"
class="auth-provider auth-provider-<%= provider[:name] %> <%= provider[:icon] ? 'auth-provider--imaged' : '' %> button">
<span class="auth-provider-name"><%= provider[:display_name] || provider[:name] %></span>
</a>
<% end %>
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ See COPYRIGHT and LICENSE files for more details.

++#%>

<% OpenProject::Plugins::AuthPlugin.providers.each do |pro| %>
<% if pro[:icon] %>
<% OpenProject::Plugins::AuthPlugin.providers.each do |provider| %>
<% if provider[:icon] %>
<style type="text/css">
#content .login-auth-providers a.auth-provider.auth-provider-<%= pro[:name] %> {
background-image: url('<%= asset_path(pro[:icon]) %>');
#content .login-auth-providers a.auth-provider.auth-provider-<%= provider[:name] %> {
background-image: url('<%= asset_path(provider[:icon]) %>');
}
.op-app-header #nav-login-content .login-auth-providers a.auth-provider.auth-provider-<%= pro[:name] %> {
background-image: url('<%= asset_path(pro[:icon]) %>') ;
.op-app-header #nav-login-content .login-auth-providers a.auth-provider.auth-provider-<%= provider[:name] %> {
background-image: url('<%= asset_path(provider[:icon]) %>') ;
}
.login-auth-providers a.auth-provider.auth-provider-<%= pro[:name] %> {
background-image: url('<%= asset_path(pro[:icon]) %>') ;
.login-auth-providers a.auth-provider.auth-provider-<%= provider[:name] %> {
background-image: url('<%= asset_path(provider[:icon]) %>') ;
}
</style>
<% end -%>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def edit_link
end

def users
User.where("identity_url LIKE ?", "#{provider.slug}%").count.to_s
provider.user_count.to_s
end

def creator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ProvidersController < ::ApplicationController

before_action :require_admin
before_action :check_ee
before_action :find_provider, only: %i[show edit import_metadata update destroy]
before_action :find_provider, only: %i[show edit import_metadata update confirm_destroy destroy]
before_action :check_provider_writable, only: %i[update import_metadata]
before_action :set_edit_state, only: %i[create edit update import_metadata]

Expand Down Expand Up @@ -85,7 +85,7 @@ def create
def update
call = Saml::Providers::UpdateService
.new(model: @provider, user: User.current)
.call(options: update_params)
.call(update_params)

if call.success?
flash[:notice] = I18n.t(:notice_successful_update) unless @edit_mode
Expand All @@ -96,6 +96,8 @@ def update
end
end

def confirm_destroy; end

def destroy
call = ::Saml::Providers::DeleteService
.new(model: @provider, user: User.current)
Expand Down
10 changes: 5 additions & 5 deletions modules/auth_saml/app/forms/saml/providers/mapping_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class MappingForm < BaseForm
form do |f|
f.text_area(
name: :mapping_login,
label: I18n.t("saml.providers.label_mapping_for", attribute: User.human_attribute_name(:login)),
label: I18n.t("label_mapping_for", attribute: User.human_attribute_name(:login)),
caption: I18n.t("saml.instructions.mapping_login"),
required: true,
disabled: provider.seeded_from_env?,
Expand All @@ -41,7 +41,7 @@ class MappingForm < BaseForm
)
f.text_area(
name: :mapping_mail,
label: I18n.t("saml.providers.label_mapping_for", attribute: User.human_attribute_name(:mail)),
label: I18n.t("label_mapping_for", attribute: User.human_attribute_name(:mail)),
caption: I18n.t("saml.instructions.mapping_mail"),
required: true,
disabled: provider.seeded_from_env?,
Expand All @@ -50,7 +50,7 @@ class MappingForm < BaseForm
)
f.text_area(
name: :mapping_firstname,
label: I18n.t("saml.providers.label_mapping_for", attribute: User.human_attribute_name(:first_name)),
label: I18n.t("label_mapping_for", attribute: User.human_attribute_name(:first_name)),
caption: I18n.t("saml.instructions.mapping_firstname"),
required: true,
disabled: provider.seeded_from_env?,
Expand All @@ -59,7 +59,7 @@ class MappingForm < BaseForm
)
f.text_area(
name: :mapping_lastname,
label: I18n.t("saml.providers.label_mapping_for", attribute: User.human_attribute_name(:last_name)),
label: I18n.t("label_mapping_for", attribute: User.human_attribute_name(:last_name)),
caption: I18n.t("saml.instructions.mapping_lastname"),
required: true,
disabled: provider.seeded_from_env?,
Expand All @@ -68,7 +68,7 @@ class MappingForm < BaseForm
)
f.text_field(
name: :mapping_uid,
label: I18n.t("saml.providers.label_mapping_for", attribute: I18n.t("saml.providers.label_uid")),
label: I18n.t("label_mapping_for", attribute: I18n.t("saml.providers.label_uid")),
caption: I18n.t("saml.instructions.mapping_uid"),
disabled: provider.seeded_from_env?,
rows: 8,
Expand Down
4 changes: 4 additions & 0 deletions modules/auth_saml/app/models/saml/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class Provider < AuthProvider

def self.slug_fragment = "saml"

def human_type
"SAML"
end

def seeded_from_env?
(Setting.seed_saml_provider || {}).key?(slug)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<%#-- copyright
OpenProject is an open source project management software.
Copyright (C) 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.

++#%>
<%= styled_form_tag(saml_provider_path(@provider),
class: 'danger-zone',
method: :delete) do %>
<section class="form--section">
<h3 class="form--section-title">
<%= t('saml.delete_title') %>
</h3>
<p>
<%= t('provider.delete_warning.provider', name: content_tag(:strong, @provider.display_name)).html_safe %>
</p>
<ul class="mb-3">
<li> <%= t('provider.delete_warning.delete_result_1') %>
<li> <%= t('provider.delete_warning.delete_result_user_count', count: @provider.user_count) %>
<% if Setting.omniauth_direct_login_provider == @provider.slug %>
<li> <%= t('provider.delete_warning.delete_result_direct') %>
<% end %>
</ul>
<p class="danger-zone--warning">
<span class="icon icon-error"></span>
<span><%= t('provider.delete_warning.irreversible_notice') %></span>
</p>
<p>
<%= t('provider.delete_warning.input_delete_confirmation', name: "<em class=\"danger-zone--expected-value\">#{h(@provider.display_name)}</em>").html_safe %>
</p>
<div class="danger-zone--verification">
<%= text_field_tag :delete_confirmation %>
<%= styled_button_tag title: t(:button_delete), class: '-primary', disabled: true do
concat content_tag :i, '', class: 'button--icon icon-delete'
concat content_tag :span, t(:button_delete), class: 'button--text'
end %>
<%= link_to saml_providers_path,
title: t(:button_cancel),
class: 'button -with-icon icon-cancel' do %>
<%= t(:button_cancel) %>
<% end %>
</div>
</section>
<% end %>
Loading
Loading