THREESCALE-11928 - Permissions policy headers by akostadinov · Pull Request #4264 · 3scale/porta

akostadinov · 2026-03-31T17:49:11Z

supersedes #4207

Now we store admin portal and dev portal permissions-policy headers in the new AccountSetting class.
We check in FrontendController the value (or use the default) and set the header on request.
Dev portal inherits from FrontendController for some reason so we override the method to use the dev portal setting instead.
bot/spam protection settings pages are updated to add ability to set the header values respectively for dev and admin portals
I will leave API update support for master where we will have settings compatibility layer. But if you think this is important for 2.16, I can edit it in but I prefer not to for simplicity and safety.

Review tips:

filter files with the model keyword and review
filter files with keyword controller.rb and review these files
review app/services/account_settings/cached_retrieval_service.rb
filter by views then review
filter by config and review
You can then look at the tests and any remaining files ~~(TESTS ARE WIP STILL)~~

akostadinov · 2026-04-07T10:28:41Z

-
-    login_as buyer.first_admin
+    # No login needed — controller skips login_required
+    host! @provider.internal_admin_domain


the tracking controller is an admin controller and does not require login

akostadinov · 2026-04-07T10:30:43Z

    def setup
      @provider = FactoryBot.create :provider_account
-      host! @provider.external_domain
+      host! @provider.internal_admin_domain


The Buyers::AccountPlansController is an admin portal controller. The forbidden tests seem to have worked because both - admin and dev portal would forbid the calls. But the error message should be from the provider portal, not from the dev portal.

akostadinov · 2026-04-07T10:33:21Z

    @provider = FactoryBot.create :provider_account
    @service = @provider.default_service
-    host! @provider.external_domain
+    host! @provider.internal_admin_domain


same - this is an admin portal controller. We can see that we login as provider admins, not buyer admins. Thus login redirect needs to be to /p/login, not /login

akostadinov · 2026-04-07T10:34:04Z

  test 'setting_name <-> class_for_setting roundtrip' do
    # Eager load all AccountSetting subclasses
-    Rails.autoloaders.main.eager_load_dir(File.join(Rails.root, 'app', 'models', 'account_setting'))
+    Rails.autoloaders.main.eager_load_dir(Rails.root.join('app', 'models', 'account_setting'))


just better style

jlledom

I didn't review the tests much thoroughly. I hope you reviewed them...

jlledom · 2026-04-07T09:59:22Z


  before_action :validate_enforcing_sso_allowed, only: [:update]

+  # TODO: Add support for Permissions-Policy headers via AccountSettings:


Why tell Claude to add a TODO when you can tell Claude to create a Jira?

Tests are WIP as I wrote in "Review tips" 👼

I'm heavily refactoring them.

this is not tests, ok, I'll also create a JIRA

jlledom · 2026-04-07T10:12:32Z

+
+  def update
+    settings_params = params.fetch(:settings, {}).dup
+    setting_name = @permissions_policy_admin_portal.setting_name


The variable is probably not needed if you are going top use it only once.

I've got tired of micro-managing it and when I make a manual change, it rushes to revert it on my next request :( but yeah, you can see in history how horrible this code was.

You can tell it to not change it, or not accept changes if you don't like. It's not an inevitable fate.

I don't accept individual changes. Also it is not trivial for every manual tune up to describe on the next prompt that it shouldn't just revert it. And even so it continued. For example I had a lot of troubles with the nonzero? check line. Also btw depends on the agent and model in use.

No need to explain the changes to the agent, just tell it "I made some changes, please read the file again"

I remember this particular line had issues even after explaining it. But with claude it worked better, in fact it tried to change something I told it not to change, then had an inner dialog that I asked her not to change, so re-reverted and had no issues after. That's why I said it also depends on agent.

jlledom · 2026-04-07T10:15:20Z

+  def edit; end
+
+  def update
+    settings_params = params.fetch(:settings, {}).dup


Maybe we could extract this to a private method that enforces strong params? As we usually do in all other controllers.

I wouldn't create extra methods for this. I will manually refactor it now although it has to be changed once we migrate all settings.

jlledom · 2026-04-07T10:16:36Z

+    settings_updated = @settings.update(settings_params.permit(:admin_bot_protection_level))
+    policy_updated = update_permissions_policy_setting(permissions_policy_value)
+
+    if settings_updated && policy_updated


I don't kinda like this. Maybe we should create a service that calls a transaction

Because if one succeeds and the other fails, it will go through the failure path, the user will see some errors and could assume nothing were saved. I think it's confusing. Better all or nothing.

As a user I would prefer my changes to be saved as much as possible. The settings are unrelated so I would prefer to redo only what failed instead of everything.

And as as user you would prefer to know what is saved and what is not, right?

That would be preferable though, yes. On the other hand I don't want to invest in heavy logic given we plan on having a settings wrapper.

jlledom · 2026-04-07T10:24:04Z

+  def update
+    settings_params = params.fetch(:settings, {}).dup
+    setting_name = @permissions_policy_developer_portal.setting_name
+    permissions_policy_value = settings_params.delete(setting_name)
+
+    # TODO: Once Settings is fully migrated to AccountSettings, handle all settings uniformly
+    # instead of separating legacy Settings model updates from AccountSettings updates
+    settings_updated = @settings.update(settings_params.permit(:spam_protection_level))
+    policy_updated = update_permissions_policy_setting(permissions_policy_value)
+
+    if settings_updated && policy_updated
+      redirect_to edit_admin_site_security_url, success: t('.success')
+    else
+      flash.now[:danger] = t('.error')
+      render :action => 'edit'
+    end
+  end


This is basically a duplicate of the admin controller. Another reason to extract this to a service.

This is a temporary approach until we have everything account_settings and not a mix. I don't want to complicate things.

refactored a little perhaps not fully as you like

It is not an exact duplicate. Also if we remove the setting for admin portal, then these will be very different. Also we may have different stuff in the two controllers in the future. It doesn't fit in my view to try to DRY these 2 controllers. I would rather unDRY the edit form.

I think we need the two edit forms, which is you idea for UX if not?

If we have one hardcoded and one not, then we will need two forms unless we prefer things to be confusing.

yes, if we opt for the hardcoded path, then of course one form must be removed

jlledom · 2026-04-07T11:35:55Z

+When('I visit the provider security settings page') do
+  visit edit_provider_admin_security_path
+end
+
+When('I visit the developer portal security settings page') do
+  visit edit_admin_site_security_path
+end


Those need to be added to features/support/paths.rb AFAIK, instead of here.

jlledom · 2026-04-07T12:02:38Z

+    # Remove default settings created by test helper
+    @provider.account_settings.where(type: 'AccountSetting::PermissionsPolicyHeaderAdmin').delete_all


Have you removed them?

I don't understand the question. Perhaps this removal is not needed but is an acceptable defensive strategy.

removed that redundant line anyway

Sorry, I interpreted the comment as a TODO. Like "we should remove...", so I asked if they were removed... 🤦

jlledom · 2026-04-07T12:03:23Z

+  # REMOVE: Weak test — asserts header not nil without checking specific value.
+  # Covered better by PermissionsPolicyHeadersTest 'admin portal uses default' and 'admin portal sets header'.
+  test 'shows Permissions-Policy header in response' do
+    get edit_provider_admin_security_path
+    assert_response :success
+    assert_not_nil response.headers['Permissions-Policy']
+    # Header should be set (either default or custom value from test helper)
+  end
+
+  # REMOVE: Duplicate of 'updates Permissions-Policy header' test above — same params and assertions.
+  test 'updates Permissions-Policy header setting' do
+    policy_value = 'camera=(), microphone=()'
+
+    put provider_admin_security_path, params: {
+      settings: {
+        admin_bot_protection_level: 'none',
+        permissions_policy_header_admin: policy_value
+      }
+    }
+
+    assert_redirected_to edit_provider_admin_security_path
+
+    @provider.reload
+    setting = @provider.account_settings.find_by(type: 'AccountSetting::PermissionsPolicyHeaderAdmin')
+    assert_not_nil setting
+    assert_equal policy_value, setting.value
+  end


If these have the # REMOVE comment, why not just removing them?

Not removed anymore, just wanted to have notes from AI and then validate that its decision made sense.

jlledom · 2026-04-07T12:03:47Z

+  # REMOVE: Duplicate of PermissionsPolicyHeadersTest 'developer portal sets Permissions-Policy header from AccountSetting'.
+  test 'shows Permissions-Policy header in developer portal' do
+    @provider.account_settings.create!(
+      type: 'AccountSetting::PermissionsPolicyHeaderDeveloper',
+      value: 'camera=(), geolocation=()'
+    )
+
+    buyer = FactoryBot.create(:buyer_account, provider_account: @provider)
+    user = FactoryBot.create(:user, account: buyer)
+    user.activate!
+
+    host! @provider.internal_domain
+    login_with user.username, 'superSecret1234#'
+
+    get '/admin'
+    assert_response :success
+    assert_equal 'camera=(), geolocation=()', response.headers['Permissions-Policy']
+  end
+
+  # REMOVE: Duplicate of PermissionsPolicyHeadersTest 'developer portal does not set header when setting is blank (default)'.
+  test 'uses default (empty) Permissions-Policy for developer portal when no setting exists' do
+    buyer = FactoryBot.create(:buyer_account, provider_account: @provider)
+    user = FactoryBot.create(:user, account: buyer)
+    user.activate!
+
+    host! @provider.internal_domain
+    login_with user.username, 'superSecret1234#'
+
+    get '/admin'
+    assert_response :success
+    # Default for developer portal is empty (no restrictions)
+    assert_nil response.headers['Permissions-Policy']
+  end


Same, just remove it instead of adding a comment

jlledom · 2026-04-07T12:05:22Z


    before_action :disable_for_suspended_provider_account

+    private


Better place private section after protected section

we need review guidelines and agent that does it , idk how you even noticed it was before protected. Maybe you used an agent.

yes claude told me. We can create a subagent for guideline, yeah.

jlledom · 2026-04-07T12:43:53Z

 # frozen_string_literal: true

-class AccountSetting::PermissionsPolicyHeaderDeveloper < AccountSetting::HttpHeaders; end
+class AccountSetting::PermissionsPolicyHeaderDeveloper < AccountSetting::HttpHeaders


I'm thinking... there's no reason to allow the clients to set the permissions policy for the admin portal. The admin portal is ours, we should define the policy there and allow the clients to only set the policy for the developer portal

This was your original design. Do you want me to remove it and hardcode it in the built-in middleware? But this will likely require a good amount of additional tuning.

This was your original design

Yeah you're right. The difference is in my design there was a single setting for the whole installation. Now each provider can set their own permissions for the admin portal, which is strange because the admin portal is the same for all providers. Maybe the api docs screen might require different policy? IDK.

Do you want me to remove it and hardcode it in the built-in middleware? But this will likely require a good amount of additional tuning.

IMO the best approach for the admin portal is the config file. With the current approach, everybody is going to get some default headers anyway, and if those are wrong, we can tell them to change them, of course, but will we tell everybody in SaaS to change the setting?, other solutions would be writing a script for everybody in SaaS or deploying a quick hotfix with different defaults. All of them are worse than using a config file, because that can be updated for everybody from Openshift.

For both headers, this and CSP, I think it's better to use a config file for the admin portal and an account setting for the dev portal.

Config files are not easy, rather the opposite. And once the customer deploys a config file, it is likely it will stay so once we inevitably have to do some changes to the defaults, such customers will not receive the update.

I think we should make the defaults solid and use rolling updates so that in case of a trouble, customers can disable the feature. But it will be an on/off switch. Rolling updates changes are only merged to the defaults, not a full configuration file override.

fine for me

Please answer your preferences like "neutral, weak preference, strong feeling" the following questions:

* should we use rolling updates to disable these headers altogether * should we allow customizing admin portal headers at all? * should we make such customization user available?

Whatever yo do is ok for me.

Ok, in last commit I fixed an issue with variable name changed that I introduced in the previous commit (and missed that the tests actually failed as they had to). Just a little bit further simplified (c7d0362) by not even passing the variable to the partial. So it is fully erady to be approved, hopefully tests will pass.

Now one last thing I forgot. We were discussing whether we should invalidate the cache on change. And whether it should happen in some kind of a service or in a model callback. I opted for just waiting the 10 minutes to pass but not really a strong preference. If I have to implement the invalidation, I'd rather do in a model callback.

P.S. One thing I want to mention is that the rails cache is not neccessary to be global, it could be local per rails server. Maybe nobody runs like that. In such case though, the invalidation would anyway help only a single cache instance.

I think it's better to invalidate the cache, don't care much if it's via callback or any other mechanism.

If you don't want to invalidate. Then I think we should heads up the customer somehow, otherwise they will save the form, get a success message and don't see the changes applied. It'd be super confusing.

Now one last thing I forgot. We were discussing whether we should invalidate the cache on change. And whether it should happen in some kind of a service or in a model callback. I opted for just waiting the 10 minutes to pass but not really a strong preference. If I have to implement the invalidation, I'd rather do in a model callback.

P.S. One thing I want to mention is that the rails cache is not neccessary to be global, it could be local per rails server. Maybe nobody runs like that. In such case though, the invalidation would anyway help only a single cache instance.

I think in all standard installations (including local) we use memcached, so I guess the cache would be global for all porta instances running in parallel?...

Also, we could potentially use Redlock (aka system redis) for caching the settings, to ensure they are applied globally.

In any case, on settings update I would certainly invalidate the cache, otherwise I think it would be very confusing for the user.

Ok, you asked for it, updated in ce47f21

Now cache is updated whenever header settings change. Also I updated how the form is generated to allow for the setting to be completely removed, so that the account will follow server default. This update will also allow us to add the CSP header with a trivial code update.

P.S. you shall not find any flaws anymore; also don't forget to precompile assets due to the small js added.

jlledom

I still think we should either invalidate the cache or heads up the user that the changes will take effect in 10 minutes

jlledom

I'm sorry to make more comments, because I'd like this to be merged as soon as possible, but I think the UI is confusing and should be redesigned.

jlledom · 2026-04-15T09:00:35Z

+  Scenario: Uncheck override deletes existing Permissions-Policy setting
+    Given the provider has admin portal Permissions-Policy "camera=(), microphone=()"
+    When I go to the provider security settings page
+    And I uncheck "override_permissions_policy_header_admin"
+    And I press "Update Security Settings"
+    Then I should see "Security settings updated"
+    And the admin portal should not have Permissions-Policy header


I don't understand this scenario: If we uncheck "Override default" I expect to not be overriding the defaults, so I expect the come back to the defaults, right?

This means that the setting is gone. If you want check the step and suggest a better name. I agree the name is not ideal. But I would like to one-shot fix it according to your suggestion intsead of going through multiple turnarounds.

But if the setting is gone, then we would have a header with the default value. The step says "should not have Permissions-Policy header", like the header is not present at all. I think the step name is confusing.

This is what I said and asked you to give me a name.

Either:

And the admin portal should not have Permissions-Policy setting

Or

And the admin portal should have the default Permissions-Policy header

hopefully fixed with a988caf

The other new commit is trivial 765c481

jlledom · 2026-04-15T09:09:02Z

+  - @account_settings.each do |account_setting|
+    - setting_name = account_setting.setting_name
+    = form.inputs account_setting.display_name do
+      li
+        label
+          input type="checkbox" id="override_#{setting_name}" data-toggle-target=setting_name checked=account_setting.persisted?
+          |  Override default
+      = form.input setting_name.to_sym,
+              label: account_setting.display_name,
+              hint: "Default: #{account_setting.default_value.presence || 'none'}",
+              input_html: { id: setting_name, placeholder: account_setting.default_value, value: account_setting.value, disabled: account_setting.new_record? }


I find the UI confusing. It's hard to understand the result of the actions here. At least for me. I think the UI should be like this:

Instead of "Override default" the check should be simply "Enabled"

false: no header

The input field should be disabled by HTML, greyed.

true: header with the given value

If there's no value in the DB, then use default

Blank shouldn't be allowed

And from the controller:

disabled means we set the value so not header is added

enabled means we set the value to whatever the user introduced

if the given value is the default, remove the setting

WDYT?

We have 3 states:

option not present => use the server default (which might as well be nil or empty value)

empty string value => Header is not set

non-empty string => set it literary (even if it is a single space which would mean an empty header)

To represent all 3 values, we need the checkbox so that user can opt to use the server default, even if that default changes in the future (which is IMO a wise decision in general).

If we don't want to allow the user choose to follow server default, then we don't need the checkbox. In which case we represent no-header with an empty value, and anything else set as the literal header value.

So the purpose of the checkbox is precisely to indicate that it is a custom value and not whatever server set as a default.

Maybe we can rename to "Override server default" if that would make it clearer.

Or do you see other appropriate approaches?

in an API call, we can pass a nil value, I assume. But in a web form, we need an extra field (checkbox) for this.

I think in first place we shouldn't accept the single space value, in which scenario would the user want that? What's the purpose of an existing but empty header? I think it's better to remove that option at all and simplify the logic.

Or do you see other appropriate approaches?

Yes, my suggestion is what I mentioned above:

Enable/disabled to decide whether we show the header or not

If enabled, the value there is the header value

Fill the field with the default value by default

When the controller receives the default value, it removes the setting instead of updating it.

blank? Not allowed, just disable the header for that.

It's more simple I think.

I considered this - when default value is there to remove but I think this would be super unexpected if I was a user. Also maybe I want to set this exact header (which happens to match current default) and not allow the pesky developers change my perfect value underneath my feet.

To me it is much more understandable that 1. you use the default or 2. you use your chosen value, whatever it is.

wrt value of only spaces, sending an empty header is a supported value in general. I don't want to put artificial restrictions. Maybe somebody has their own reasons to set the header to empty. Who are we to restrict that and tell them what to put in there? We may have an additional checkbox for enabled disabled to remove all heuristics from the behavior but to me that would seem too much. I don't expect the majority of the users to set a blank header but to want it to still be present. If somebody puts a space by mistake, it should also not hurt that the header is there but empty. This is an advanced option, if somebody sets it, they need to know what they're doing.

FYI it was something like this before the checkbox addition:

diff --git a/app/views/provider/admin/securities/_form.html.slim b/app/views/provider/admin/securities/_form.html.slim index 974d0bc04..beae75fc1 100644 --- a/app/views/provider/admin/securities/_form.html.slim +++ b/app/views/provider/admin/securities/_form.html.slim @@ -1,17 +1,26 @@ -= semantic_form_for settings, url: url, html: {:id => 'security-settings' } do |form| - = form.inputs selector_label do - = form.input field, - label: false, - hint: t("provider.admin.securities.edit.captcha_hint_#{Recaptcha.captcha_configured?.to_s}"), - as: :radio, - collection: ThreeScale::BotProtection::LEVELS, - input_html: { disabled: !Recaptcha.captcha_configured? } +div class="pf-c-card" + div class="pf-c-card__body" + = semantic_form_for settings, url: url, builder: Fields::PatternflyFormBuilder, + html: { id: 'security-settings', class: 'pf-c-form pf-m-limit-width' } do |form| + section class="pf-c-form__section" role="group" + div class="pf-c-form__section-title" aria-hidden="true" = selector_label + div class="pf-c-form__group" role="radiogroup" aria-labelledby="bot-protection-radio-group" + div class="pf-c-form__group-control pf-m-stack" + - ThreeScale::BotProtection::LEVELS.each do |label, value| + - id = "settings_#{field}_#{value}" + - checked = settings.public_send(field).to_s == value.to_s + div class="pf-c-radio" + input class="pf-c-radio__input" name="settings[#{field}]" id=id type="radio" value=value checked=checked disabled=(!Recaptcha.captcha_configured?) + label class="pf-c-radio__label" for=id = label + p class="pf-c-form__helper-text" = t("provider.admin.securities.edit.captcha_hint_#{Recaptcha.captcha_configured?.to_s}") - = form.inputs 'Permissions-Policy Header' do - = form.input permissions_policy_setting.setting_name.to_sym, - label: 'Permissions-Policy Header', - hint: "Format: directive1=(), directive2=(self). Default: #{permissions_policy_setting.default_value.presence || 'none (permissive)'}", - input_html: { placeholder: permissions_policy_setting.default_value, value: permissions_policy_setting.value } + section class="pf-c-form__section" role="group" + div class="pf-c-form__section-title" aria-hidden="true" Permissions-Policy Header + = form.input permissions_policy_setting.setting_name.to_sym, + as: :patternfly_input, + label: 'Permissions-Policy Header', + hint: "Format: directive1=(), directive2=(self). Default: #{permissions_policy_setting.default_value.presence || 'none (permissive)'}", + input_html: { placeholder: permissions_policy_setting.default_value, value: permissions_policy_setting.value } - = form.actions do - = form.commit_button 'Update Security Settings' + = form.actions do + = form.commit_button 'Update Security Settings' diff --git a/app/views/provider/admin/securities/edit.html.slim b/app/views/provider/admin/securities/edit.html.slim index 2486c20c1..2adc82f88 100644 --- a/app/views/provider/admin/securities/edit.html.slim +++ b/app/views/provider/admin/securities/edit.html.slim @@ -1,4 +1,7 @@ - content_for :title, 'Security' - content_for :page_header_title, 'Security' +- content_for :javascripts do + = stylesheet_packs_chunks_tag 'pf_form' + = render partial: 'form', locals: { url: provider_admin_security_path, settings: @settings, field: :admin_bot_protection_level, selector_label: t('.selector_label'), permissions_policy_setting: @permissions_policy_admin_portal } diff --git a/app/views/sites/securities/edit.html.slim b/app/views/sites/securities/edit.html.slim index 3b0012e91..ede00a10c 100644 --- a/app/views/sites/securities/edit.html.slim +++ b/app/views/sites/securities/edit.html.slim @@ -1,4 +1,7 @@ - content_for :title, 'Security' - content_for :page_header_title, 'Security' +- content_for :javascripts do + = stylesheet_packs_chunks_tag 'pf_form' + = render partial: 'provider/admin/securities/form', locals: { url: admin_site_security_path, settings: @settings, field: :spam_protection_level, selector_label: t('.selector_label'), permissions_policy_setting: @permissions_policy_developer_portal }

I would suggest to merge with minimal acceptable improvements, like rename the field to Override server default value and migrate to patternfly in a future PR. I asked agent to update to patternfly and it was a little long (not terrible) but I didn't want to burden the PR with even more details.

c81e2dd

This is not a super-complete PF4-migration of the form (ideally, there should be a dedicated radio input in PatternflyFormBuilder, but this can easily be left for a future improvement. The important thing is that it does the trick with disabling the input visibly (it's much clearer in PF4 input than in standard one):

UPDATE: oh, I didn't see AI-provided solution before, I think it's similar!

I'm sorry @jlledom @akostadinov Not that I want to extend the discussion infinitely... but I though I'd still leave my (extremely valuable, of course) opinion in this public space 😅

Just to clarify, here I am fully abstracting from the implementation (as if I knew absolutely nothing about it), just focusing on how I, as a user, would find it comfortable to use.

So, I would expect the Permissions-Policy header to be non-present by default on the response (as it is right now).

So, the initial state would be:

If I wanted to set some value, I would enable the checkbox, and set my value in the input:

In fact, ideally (again, not caring about how this is stored on the server), I imagine it could be useful to have the possibility to disable the header completely (i.e. not include in the response), while keeping the value for future reference. So, something like this could happen:

Now, I understand (as @akostadinov explained above) that this doesn't handle the situation where we want to enforce some default value (currently hardcoded in the model). The question, however, is - Do we want/need to enforce some default value???
The Jira just requests that it should be possible to set the header.
To be honest, I don't really know, what is the implication of setting a wrong (or empty) Permissions-Policy header. In a patch version (if this is in the end gonna make it into 2.16.4), I would prefer to NOT set any header at all.

When the controller receives the default value, it removes the setting instead of updating it.

I think this is wrong... Indeed, what would happen, if we change the default (in the model)? This will change the behavior for the user, while they will have no idea, because they would be under impression that they set the value explicitly in the input field.

update, small adjustments and checkbox renamed!!!

Co-Authored-By: Claude <noreply@anthropic.com>

mayorova · 2026-04-16T17:14:19Z

+        end
+      else
+        Rails.cache.write(key, @value, expires_in: @expires_in)
+        @value


And actually... now looking at:

AccountSettings::CachedRetrievalService.call(account: account, setting_name: setting_name, value: cached_value)

Honestly, to me it is not at all clear that it would set the cache value instead of fetching it.

In general, I don't like this pattern (the service pattern that just receives #call) for cache, I already struggled with a similar thing for billing locks.

I think it would be much clearer to have #fetch, #set, #clear, so it's actually clear what they do.

But maybe it's just me being a bit close-minded, don't know... 🤷

akostadinov · 2026-04-16T18:42:21Z

The question, however, is - Do we want/need to enforce some default value???

We don't need, it makes sense though for the admin portal that we control. I think we can leave the default empty for 2.16.4 if that bothers you. For master I think it is better for us to set a default.

cc @mayorova

mayorova · 2026-04-16T19:35:23Z

  end

+  def set_permissions_policy_header
+    header_value = AccountSettings::SettingCache.fetch(


I find this interface for cache so much more clear! Thank you ❤️

mayorova · 2026-04-16T19:52:25Z

The question, however, is - Do we want/need to enforce some default value???

We don't need, it makes sense though for the admin portal that we control. I think we can leave the default empty for 2.16.4 if that bothers you. For master I think it is better for us to set a default.

cc @mayorova

Yeah, I would prefer to keep it empty in this case, at least for 2.16.4, to avoid any unexpected changes for the customers in a patch release.

So, those who don't touch anything - don't get any change in behavior.

akostadinov · 2026-04-17T07:24:35Z

Yeah, I would prefer to keep it empty in this case, at least for 2.16.4, to avoid any unexpected changes for the customers in a patch release.

Ok, @mayorova , I will set the default to empty when backporting to 2.16.4, please approve.

jlledom · 2026-04-17T07:27:44Z

Ok, @mayorova , I will set the default to empty when backporting to 2.16.4, please approve.

Why backporting? is this not going to be part of 2.16.4 directly?

akostadinov · 2026-04-17T07:33:39Z

Why backporting? is this not going to be part of 2.16.4 directly?

This PR is for master. To include the functionality in 2.16.4 I will have to cherry-pick the base AccountSetting PR as well as this one as well as the upcoming CSP one. I will just update the default values after the cherry-pick. Is this answering your question or there's something I don't get?

jlledom · 2026-04-17T08:11:47Z

+  Scenario: Uncheck override deletes existing Permissions-Policy setting
+    Given the provider has configured admin portal Permissions-Policy "camera=(), microphone=()"
+    When I go to the provider security settings page
+    And I uncheck "override_permissions_policy_header_admin"


Does this work after you renamed the check?

mayorova · 2026-04-17T08:14:30Z

Yeah, I would prefer to keep it empty in this case, at least for 2.16.4, to avoid any unexpected changes for the customers in a patch release.

Ok, @mayorova , I will set the default to empty when backporting to 2.16.4, please approve.

I am not into the idea of using different defaults for master/2.16...

I think there is no rush in enforcing a specific default - let's just keep it empty everywhere, release 2.16.4, hopefully gather feedback (or not), and we can test SaaS carefully with the explicitly set value, and after that we'll apply the new default (which could coincide with 2.17 release cycle).

I think it's a better way, and way less risky.

jlledom

Claude had a comment, feel free to ignore Claude.

jlledom · 2026-04-17T09:48:39Z

+      private
+
+      def cache_key(account, setting_name)
+        "account:#{account.id}:#{setting_name}"


From the Claude review, the only comment I think it could be worthy is about the cache key: it's too generic. Better add a prefix to avoid collisions.

Makes sense, maybe account_setting:#{account.id}:#{setting_name}

My line of logic is that it would read something like account:12345:permissions_policy_header_admin.

That is the account identity is 12345. While account_setting object does not have an identity of 12345.

But I see the logic behind this suggestion. So I will take your preferred naming. I suggest to merge this and update the key in the follow-up CSP PR. Changing of the key should be a safe operation at any point regardless of deployment status.

akostadinov requested a review from jlledom April 6, 2026 20:17

akostadinov commented Apr 7, 2026

View reviewed changes

jlledom reviewed Apr 7, 2026

View reviewed changes

akostadinov requested review from jlledom and mayorova April 9, 2026 14:46

akostadinov changed the title ~~WIP: THREESCALE-11928 - Permissions policy headers~~ THREESCALE-11928 - Permissions policy headers Apr 9, 2026

jlledom reviewed Apr 10, 2026

View reviewed changes

akostadinov force-pushed the permissions-policy branch from 0714b69 to ce47f21 Compare April 14, 2026 20:44

akostadinov requested a review from jlledom April 14, 2026 20:45

jlledom reviewed Apr 15, 2026

View reviewed changes

jlledom previously approved these changes Apr 15, 2026

View reviewed changes

jlledom and others added 5 commits April 15, 2026 23:56

Load Permissions-Policy from a config file

581e075

Co-Authored-By: Claude <noreply@anthropic.com>

Add tests for Permissions-Policy header

d61fea4

Co-Authored-By: Claude <noreply@anthropic.com>

use AccountSetting to store header values

8b30787

update step names

a988caf

frozen_string_literal

765c481

akostadinov dismissed jlledom’s stale review via 765c481 April 15, 2026 21:01

akostadinov force-pushed the permissions-policy branch from ce47f21 to 765c481 Compare April 15, 2026 21:01

mayorova and others added 3 commits April 16, 2026 19:21

PF4 inputs for Security settings form

3e7f464

small adjustments

fdb8667

make checkbox darn clear

fc16195

mayorova reviewed Apr 16, 2026

View reviewed changes

akostadinov added 2 commits April 16, 2026 20:15

revert to the simpler form.inputs that already does section

f5c2f20

cohesive caching service

dcbb89c

mayorova reviewed Apr 16, 2026

View reviewed changes

akostadinov requested review from jlledom and mayorova April 17, 2026 07:25

jlledom approved these changes Apr 17, 2026

View reviewed changes

akostadinov merged commit c0e75d1 into 3scale:master Apr 17, 2026
16 of 20 checks passed


		before_action :validate_enforcing_sso_allowed, only: [:update]

		# TODO: Add support for Permissions-Policy headers via AccountSettings:

		# Remove default settings created by test helper
		@provider.account_settings.where(type: 'AccountSetting::PermissionsPolicyHeaderAdmin').delete_all


		before_action :disable_for_suspended_provider_account

		private

Conversation

akostadinov commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

akostadinov commented Mar 31, 2026 •

edited

Loading

jlledom Apr 7, 2026 •

edited

Loading