From 8141bdeed5c3283c5e04e3eb5a7a9a7b2d53546c Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Fri, 30 May 2025 11:24:19 -0600 Subject: [PATCH 1/4] Fix handling of `attrs[:managed]` on Org updates This change resolves issue #3528 by preventing the updating of `@org.managed` when `attrs[:managed]` is missing. Prior to this change, the `if attrs.key?(:managed)` was not present. But it was necessary, because that key is not present when a super user clicks "Save" on the "Request Feedback" page. - As a result, if `attrs[:managed]` was not present, then `attrs[:managed] = (attrs[:managed] == '1')` would evaluate false, and `if @org.update(attrs)` (line 81) would subsequently update `@org.managed` to false in the db. --- app/controllers/orgs_controller.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/controllers/orgs_controller.rb b/app/controllers/orgs_controller.rb index ed0e3f78df..43b4d49532 100644 --- a/app/controllers/orgs_controller.rb +++ b/app/controllers/orgs_controller.rb @@ -44,7 +44,7 @@ def admin_update # Only allow super admins to change the org types and shib info if current_user.can_super_admin? identifiers = [] - attrs[:managed] = attrs[:managed] == '1' + attrs = handle_managed_flag(attrs) # Handle Shibboleth identifier if that is enabled if Rails.configuration.x.shibboleth.use_filtered_discovery_service @@ -236,6 +236,13 @@ def search_params params.require(:org).permit(:name, :type) end + def handle_managed_flag(attrs) + # NOTE: `:managed` is controlled by a check_box in the form + # `app/views/orgs/_profile_form.html.erb`. + attrs[:managed] = (attrs[:managed] == '1') if attrs.key?(:managed) + attrs + end + def shib_login_url shib_login = Rails.configuration.x.shibboleth.login_url "#{request.base_url.gsub('http:', 'https:')}#{shib_login}" From eb3b77cfadbdfdc945f93c1d033cfa8365ad8208 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 2 Jun 2025 12:33:01 -0600 Subject: [PATCH 2/4] Refactor: handle logo and shibboleth w/ helpers This refactor is meant to serve as a small step in simplifying `OrgsController#admin_update` controller action, which currently contains a lot of rubocop offences. - Moved `attrs[:logo]` handling logic into `handle_logo(attrs)` - Moved Shibboleth identifier handling into `handle_shibboleth_identifier(attrs)` --- app/controllers/orgs_controller.rb | 61 +++++++++++++++++------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/app/controllers/orgs_controller.rb b/app/controllers/orgs_controller.rb index 43b4d49532..3539c45b2d 100644 --- a/app/controllers/orgs_controller.rb +++ b/app/controllers/orgs_controller.rb @@ -33,41 +33,17 @@ def admin_update @org = Org.find(params[:id]) authorize @org - # If a new logo was supplied then use it, otherwise retain the existing one - attrs[:logo] = attrs[:logo].present? ? attrs[:logo] : @org.logo - # Remove the logo if the user checked the box - attrs[:logo] = nil if attrs[:remove_logo] == '1' + attrs = handle_logo(attrs) tab = (attrs[:feedback_enabled].present? ? 'feedback' : 'profile') @org.links = ActiveSupport::JSON.decode(params[:org_links]) if params[:org_links].present? # Only allow super admins to change the org types and shib info if current_user.can_super_admin? - identifiers = [] attrs = handle_managed_flag(attrs) + attrs = handle_shibboleth_identifier(attrs) - # Handle Shibboleth identifier if that is enabled - if Rails.configuration.x.shibboleth.use_filtered_discovery_service - shib = IdentifierScheme.by_name('shibboleth').first - - if shib.present? && attrs[:identifiers_attributes].present? - key = attrs[:identifiers_attributes].keys.first - entity_id = attrs[:identifiers_attributes][:"#{key}"][:value] - # rubocop:disable Metrics/BlockNesting - if entity_id.present? - identifier = Identifier.find_or_initialize_by( - identifiable: @org, identifier_scheme: shib, value: entity_id - ) - @org = process_identifier_change(org: @org, identifier: identifier) - else - # The user blanked out the entityID so delete the record - @org.identifier_for_scheme(scheme: shib)&.destroy - end - # rubocop:enable Metrics/BlockNesting - end - attrs.delete(:identifiers_attributes) - end - + identifiers = [] # See if the user selected a new Org via the Org Lookup and # convert it into an Org lookup = org_from_params(params_in: attrs) @@ -236,6 +212,14 @@ def search_params params.require(:org).permit(:name, :type) end + def handle_logo(attrs) + # If a new logo was supplied then use it, otherwise retain the existing one + attrs[:logo] = attrs[:logo].present? ? attrs[:logo] : @org.logo + # Remove the logo if the user checked the box + attrs[:logo] = nil if attrs[:remove_logo] == '1' + attrs + end + def handle_managed_flag(attrs) # NOTE: `:managed` is controlled by a check_box in the form # `app/views/orgs/_profile_form.html.erb`. @@ -243,6 +227,29 @@ def handle_managed_flag(attrs) attrs end + # Updates the @org's Shibboleth identifier(s) if the required conditions are met + def handle_shibboleth_identifier(attrs) + return attrs unless Rails.configuration.x.shibboleth.use_filtered_discovery_service + + shib = IdentifierScheme.by_name('shibboleth').first + + if shib.present? && attrs[:identifiers_attributes].present? + key = attrs[:identifiers_attributes].keys.first + entity_id = attrs[:identifiers_attributes][:"#{key}"][:value] + if entity_id.present? + identifier = Identifier.find_or_initialize_by( + identifiable: @org, identifier_scheme: shib, value: entity_id + ) + @org = process_identifier_change(org: @org, identifier: identifier) + else + # The user blanked out the entityID so delete the record + @org.identifier_for_scheme(scheme: shib)&.destroy + end + end + attrs.delete(:identifiers_attributes) + attrs + end + def shib_login_url shib_login = Rails.configuration.x.shibboleth.login_url "#{request.base_url.gsub('http:', 'https:')}#{shib_login}" From 1256b63db12c28838a958c3c46bf15dc4f4baabf Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 2 Jun 2025 13:10:26 -0600 Subject: [PATCH 3/4] Disable rubocop warnings --- app/controllers/orgs_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/orgs_controller.rb b/app/controllers/orgs_controller.rb index 3539c45b2d..07168570d7 100644 --- a/app/controllers/orgs_controller.rb +++ b/app/controllers/orgs_controller.rb @@ -228,6 +228,7 @@ def handle_managed_flag(attrs) end # Updates the @org's Shibboleth identifier(s) if the required conditions are met + # rubocop:disable Metrics/AbcSize def handle_shibboleth_identifier(attrs) return attrs unless Rails.configuration.x.shibboleth.use_filtered_discovery_service @@ -249,6 +250,7 @@ def handle_shibboleth_identifier(attrs) attrs.delete(:identifiers_attributes) attrs end + # rubocop:enable Metrics/AbcSize def shib_login_url shib_login = Rails.configuration.x.shibboleth.login_url From 5da1ba854c0a8a3a9a4b7ad4fa6ca54c71e090d4 Mon Sep 17 00:00:00 2001 From: aaronskiba Date: Mon, 2 Jun 2025 13:16:09 -0600 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5637083df..92a23e4cf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## Unreleased +- Fix Handling of `attrs[:managed]` + Refactor `OrgsController#admin_update` [#3529](https://github.com/DMPRoadmap/roadmap/pull/3529) + ## v5.0.2 - Bump Ruby to v3.1.4 and use `.ruby-version` in CI - [#3566](https://github.com/DMPRoadmap/roadmap/pull/3566)