Enforce permissions check in ApplicationSetting::UpdateService

Problems with the existing design

• We have places in the code where we modify the ApplicationSetting without checking in-depth for user permissions. Currently we always check permissions outside the update logic and in some other places where we don't check for admin permissions is because that code-path can only be used by admins. However, rather than relying on assumptions we should have defence in-depth and have a dedicated service to update settings that always checks that. Allow unsafe_execute! method to be used in exceptional cases and where we know the context is safe.
• We should have clear distinction between reading settings (very frequently and from everywhere) and writing settings (very rare and from admins). Currently these 2 operations are blurred. Reading setting should never allow the code to potentially update it.
• We use Gitlab::CurrentSettings.current_application_settings everywhere in the codebase. This is bad because it returns an ActiveRecord object that can be modified. We should ban the use of Gitlab::CurrentSettings.current_application_settings and ApplicationSetting.current* methods and instead read the attribute directly, relying on method_missing implementation. Remove all code paths that are not admin workflows where we return ApplicationSetting record, so that we don't leak it anywhere.
  • at the very minimum the AR object returned should be set as readonly!.
• After updating the settings in a SSoT service, the cache should be invalidated consistently. We use in-memory application settings especially in specs and this doesn't behave consistently because the cache is ignored and the modified version of the settings is not retained in memory. When using in-memory settings, any updates persisted in the database should be reflected in the in-memory representation. The in-memory should be a default source when the ApplicationSetting record doesn't exist in the database, to ensure we always have some settings.
• There may be still some cyclical dependencies with ApplicationSetting and Gitlab::CurrentSettings.

Proposal

When enditing ApplicationSetting record we should always ensure that the current_user has update_application_setting permissions. Today is implicit and this could be a problem long term. By introducing a new service we can have explicit calls to unsafe_execute! when we don't have the current_user.