Move settings out of the MozReviewExtension.settings object into a custom dict

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Due to the problem reported in Bug 1245396 we need to create an admin page for the MozReview extension which doesn't leak our passwords :)
updating summary for clarity - to prevent the problem in bug 1245396 we need to stop using MozReviewExtension.settings to store settings.  part of this fix involves creating an admin page.

we'll also need to ensure the configuration can still be set easily during automation/vm configuration, and update the code/test scripts that get/set values via mre.settings.
Summary: Add an admin page for MozReview extension settings → Move settings out of the MozReviewExtension.settings object into a custom dict

Comment 2

3 years ago
Could we put the credentials in the settings_local.py file and manage them via our normal system administration mechanisms? Ditto for reading them directly from e.g. a JSON file on disk.

It feels like overkill to me to design a web interface to modify these settings when a simple static file on disk will do.
Flags: needinfo?(smacleod)
(In reply to Gregory Szorc [:gps] from comment #2)
> Could we put the credentials in the settings_local.py file and manage them
> via our normal system administration mechanisms? Ditto for reading them
> directly from e.g. a JSON file on disk.
> 
> It feels like overkill to me to design a web interface to modify these
> settings when a simple static file on disk will do.

Yes, either of those would work as well.

The one thing these solutions are missing (including the django admin site one) which the built-in settings provided was when the settings are changed in one webhead the change would automatically propagate and re-sync on the other webheads.
Flags: needinfo?(smacleod)

Comment 4

3 years ago
We don't change these credentials very often. So I don't think instantaneous syncing is a huge concern. We can always put them on a file on the network mount if we need the change to be atomic.
recently we've had to disable features quickly due to security issues; this is easy and quick to do with the existing settings implementation.  if we move to a static file i think it would be prudent to ensure that all members of the mozreview team have permissions to edit that file, and the process is clearly documented.
I suppose the static file will only contain private info and everything else will still be configurable via the admin panel.
(Assignee)

Comment 7

3 years ago
(In reply to Mauro Doglio [:mdoglio] from comment #6)
> I suppose the static file will only contain private info and everything else
> will still be configurable via the admin panel.

Disabling autoland will still be possible through the repositories admin panel. I think static analysis bots will be configured per repository, so we would be able to disable them there as well.

We should be careful in the future to make sure each feature has an easy way to disable it. Right now when we disable something, we end up breaking ui (e.g. bug 1243530).
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
Created attachment 8715873 [details] [diff] [review]
Move settings to json file

This moves the settings to a local json file.
Attachment #8715873 - Flags: review?(gps)

Comment 9

3 years ago
Comment on attachment 8715873 [details] [diff] [review]
Move settings to json file

Review of attachment 8715873 [details] [diff] [review]:
-----------------------------------------------------------------

Can we not put this content in settings_local.py for now? You should be able to access this Python module via `from django.conf import settings`. I'd store all our custom settings in a single dict in that file and have that dict exposed via the get_settings() you added. This feels like the easier approach.

::: pylib/mozreview/mozreview/extension.py
@@ +57,5 @@
>  from mozreview.resources.review_request_summary import (
>      review_request_summary_resource,)
>  
>  
> +SETTINGS_PATH = os.path.join('/', 'reviewboard', 'mozreview-settings.json')

This *always* evaluates to '/reviewboard/mozreview-settings.json', no? This would be incorrect in production, as reviewboard runs from /data/www/reviewboard.mozilla.org or something like that.

@@ +319,5 @@
> +
> +        global SETTINGS
> +        global LAST_SETTINGS_MTIME
> +
> +        mtime = os.stat(SETTINGS_PATH).st_mtime

The mtime foo is correct. But if feels like overkill. Let's just restart the app servers if we need to change the config.
Attachment #8715873 - Flags: review?(gps)
(Assignee)

Comment 10

3 years ago
Created attachment 8716428 [details] [diff] [review]
Move settings to json file

As discussed in irc, I hit enough problems trying to populate settings_local.py properly in the test environment that I think it's worth while filing a follow on bug (maybe to implement an admin page for this as originally suggested.)
Attachment #8715873 - Attachment is obsolete: true
Attachment #8716428 - Flags: review?(gps)
Comment on attachment 8716428 [details] [diff] [review]
Move settings to json file

Review of attachment 8716428 [details] [diff] [review]:
-----------------------------------------------------------------

::: pylib/mozreview/mozreview/extension.py
@@ +85,5 @@
>      initialize_signal_handlers,
>  )
>  
>  
> +SETTINGS_PATH = os.path.join('/', 'mozreview-settings.json')

I'm not keen on creating a /mozreview-settings.json file in production. But if this is short lived, I'm OK with it.
Attachment #8716428 - Flags: review?(gps) → review+
(Assignee)

Comment 12

3 years ago
Yeah, I'm not happy about /mozreview-settings.json either :/

Pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/f101eefdb703
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Group: mozilla-employee-confidential
mcote: please remove the security group from this bug.
Flags: needinfo?(mcote)

Updated

3 years ago
Group: webtools-security
Flags: needinfo?(mcote)
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.