force non-default secret/admin-credentials (relates to #229) + update & fix doc (#337) + add checks for udpate special users (#164)

Author: fmigneault

.pylintrc

@@ -93,7 +93,10 @@ disable=C0111,missing-docstring,
 # either give multiple identifier separated by comma (,) or put this option
 # multiple time (only on the command line, not in the configuration file where
 # it should appear only once). See also the "--disable" option for examples.
-enable=c-extension-no-member
+enable=
+    c-extension-no-member
+    W0122
+    W0123
 
 
 [REPORTS]

HISTORY.rst

@@ -23,6 +23,8 @@ Features / Changes
   specified with ``MAGPIE_CONFIG_PATH`` environment variable or ``magpie.config_path`` setting (example in ``configs``).
 * Add disabled checkboxes for UI rendering of non-editable items
   (relates to `#164 <https://github.com/Ouranosinc/Magpie/issues/164>`_).
+* Configuration parameters ``MAGPIE_SECRET``, ``MAGPIE_ADMIN_USER`` and ``MAGPIE_ADMIN_PASSWORD`` now require explicit
+  definitions (either by environment variable or INI settings) to avoid using defaults for security purposes.
 
 Bug Fixes
 ~~~~~~~~~~~~~~~~~~~~~

config/magpie.ini

@@ -30,7 +30,7 @@ magpie.port = 2001
 magpie.url = http://localhost:2001
 magpie.max_restart = 5
 # This secret should be the same in Twitcher !
-magpie.secret = seekrit
+magpie.secret =
 magpie.push_phoenix = true
 magpie.config_path =
 

docs/configuration.rst

@@ -288,9 +288,9 @@ remain available as described at the start of the `Configuration`_ section.
   | Secret value employed to encrypt user authentication tokens.
   | **Important Note:**
   | Changing this value at a later time will cause previously created user tokens to be invalidated.
-    It is **strongly** recommended to change this value before proceeding to user accounts and permissions creation
-    in your `Magpie` instance.
-  | (Default: ``"seekrit"``)
+    This value **MUST** be defined before starting the application in order to move on to user accounts and permissions
+    creation in your `Magpie` instance. The application will quit with an error if this value cannot be found.
+  | (Default: None)
 
 - | ``MAGPIE_COOKIE_NAME``
   | Identifier of the cookie that will be used for reading and writing in the requests from login and for
@@ -303,15 +303,25 @@ remain available as described at the start of the `Configuration`_ section.
 
 - | ``MAGPIE_ADMIN_USER``
   | Name of the default 'administrator' generated by the application.
-  | **Note:**
-  | This user is required for initial launch of the application to avoid being 'looked out' as routes for creating new
+  | **Important Notes:**
+  | This user is required for initial launch of the application to avoid being 'locked out' as routes for creating new
     users require administrative permissions and access rights. It should be used as a first login method to setup other
-    accounts. It will also be used by other `Magpie` internal operations such as service synchronization and setup
-    during the application startup. If this user is missing, it is automatically re-created on following start.
+    accounts. It is afterwards recommended to employ other user accounts with ``MAGPIE_ADMIN_GROUP`` membership to
+    accomplish administrative management operations.
+  | This value **MUST** be defined before starting the application in order to move on any other operation in your
+    `Magpie` instance. The application will quit with an error if this value cannot be found. Also, no defaults are
+    applied to motivate the developer to configured new instances with server-specific and strong credentials.
+  | If this user is missing, it is automatically recreated on following start. The best way to invalidate this user's
+    credentials is therefore to completely remove its entry it from the database so it gets regenerated from updated
+    configuration values. Note also that modifying this value without actually updating the user entry in the database
+    could cause other operations to fail drastically since this special user will be employed by other `Magpie` internal
+    operations such as service synchronization and setup during the application startup.
   | (Default: ``"admin"``)
 
 - | ``MAGPIE_ADMIN_PASSWORD``
   | Password of the default 'administrator' generated by the application.
+  | **Important Notes:**
+  | This parameter is required in order for the `Magpie` instance to start. See details in above ``MAGPIE_ADMIN_USER``.
   | (Default: ``"qwerty"``)
 
 - | ``MAGPIE_ADMIN_EMAIL``
@@ -326,7 +336,7 @@ remain available as described at the start of the `Configuration`_ section.
     higher level permissions on this group to ease the management process of granted access to all their members.
   | (Default: ``"administrators"``)
 
-- | ``MAGPIE_ADMIN_PERMISSION``
+- | ``MAGPIE_ADMIN_PERMISSION`` [constant]
   | Name of the permission used to represent highest administration privilege in the application.
   | Except for some public routes, most API and UI paths will require the user to have this permission (either with
     direct permission or by inherited group permission) to be granted access to view and edit content.
@@ -372,7 +382,7 @@ remain available as described at the start of the `Configuration`_ section.
   | This value should not be greater then the token length used to identify a user to preserve some utility behaviour.
   | (Default: ``64``)
 
-- | ``MAGPIE_LOGGED_USER``
+- | ``MAGPIE_LOGGED_USER`` [constant]
   | Keyword used to define route resolution using the currently logged in user. This value allows, for example,
     retrieving the user details of the logged user with ``GET /users/${MAGPIE_LOGGED_USER}`` instead of having to
     find explicitly the ``GET /users/<my-user-id>`` variant. User resolution is done using the authentication cookie
@@ -384,7 +394,7 @@ remain available as described at the start of the `Configuration`_ section.
     unauthorized response if using is ID in the path if he doesn't have administrator privilege.
   | (Default: ``"current"``)
 
-- | ``MAGPIE_DEFAULT_PROVIDER``
+- | ``MAGPIE_DEFAULT_PROVIDER`` [constant]
   | Name of the provider used for local login. This represents the identifier that will be set to define who to
     differentiate between a local sign-in procedure and a dispatched one to one of the known `External Providers`_.
   | *The default is the value of the internal package used to manage user permissions.*

magpie/adapter/__init__.py

@@ -160,7 +160,7 @@ def owsproxy_config(self, container):
         config = self.configurator_factory(container)
         owsproxy_defaultconfig(config)  # let Twitcher configure the rest normally
 
-    def configurator_factory(self, container):  # noqa: N805
+    def configurator_factory(self, container):  # noqa: N805, R0201
         # type: (AnySettingsContainer) -> Configurator
         settings = get_settings(container)
         set_cache_regions_from_settings(settings)

magpie/api/management/user/user_views.py

@@ -73,9 +73,18 @@ def update_user_view(request):
     update_username = user.user_name != new_user_name and new_user_name is not None
     if update_username:
         logged_user_name = get_constant("MAGPIE_LOGGED_USER", request)
-        ax.verify_param(new_user_name, param_compare=logged_user_name, not_equal=True,
-                        http_error=HTTPBadRequest, param_name=user_key, content={user_key: logged_user_name},
-                        msg_on_fail=s.Service_PUT_BadRequestResponseSchema_ReservedKeyword.description)
+        always_forbidden_user_names = [
+            get_constant("MAGPIE_ADMIN_USER", request),
+            get_constant("MAGPIE_ANONYMOUS_USER", request)
+        ]
+        ax.verify_param(user.user_name, not_in=True, with_param=False,  # avoid leaking username details
+                        param_compare=always_forbidden_user_names, param_name=user_key,
+                        http_error=HTTPBadRequest, content={user_key: logged_user_name},
+                        msg_on_fail=s.User_PUT_ForbiddenResponseSchema.description)
+        ax.verify_param(new_user_name, not_in=True, with_param=False,  # avoid leaking username details
+                        param_compare=[logged_user_name] + always_forbidden_user_names, param_name=user_key,
+                        http_error=HTTPBadRequest, content={user_key: logged_user_name},
+                        msg_on_fail=s.User_PUT_ForbiddenResponseSchema.description)
     update_password = user.user_password != new_password and new_password is not None
     update_email = user.email != new_email and new_email is not None
     ax.verify_param(any([update_username, update_password, update_email]), is_true=True, http_error=HTTPBadRequest,
@@ -120,6 +129,10 @@ def delete_user_view(request):
     Delete a user by name.
     """
     user = ar.get_user_matchdict_checked_or_logged(request)
+    ax.verify_param(user.user_name, not_in=True, with_param=False,  # avoid leaking username details
+                    param_compare=[get_constant("MAGPIE_ADMIN_USER", request),
+                                   get_constant("MAGPIE_ANONYMOUS_USER", request)],
+                    http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description)
     ax.evaluate_call(lambda: request.db.delete(user), fallback=lambda: request.db.rollback(),
                      http_error=HTTPForbidden, msg_on_fail=s.User_DELETE_ForbiddenResponseSchema.description)
     return ax.valid_http(http_success=HTTPOk, detail=s.User_DELETE_OkResponseSchema.description)

magpie/api/requests.py

@@ -141,6 +141,13 @@ def get_user(request, user_name_or_token=None):
 
 def get_user_matchdict_checked_or_logged(request, user_name_key="user_name"):
     # type: (Request, Str) -> models.User
+    """
+    Obtains either the explicit or logged user user specified in the request path variable.
+
+    :returns found user.
+    :raises HTTPForbidden: if the requesting user does not have sufficient permission to execute this request.
+    :raises HTTPNotFound: if the specified user name or logged user keyword does not correspond to any existing user.
+    """
     logged_user_name = get_constant("MAGPIE_LOGGED_USER", settings_container=request)
     logged_user_path = s.UserAPI.path.replace("{" + user_name_key + "}", logged_user_name)
     if user_name_key not in request.matchdict and request.path_info.startswith(logged_user_path):
@@ -154,7 +161,7 @@ def get_user_matchdict_checked(request, user_name_key="user_name"):
 
     :returns: found user.
     :raises HTTPForbidden: if the requesting user does not have sufficient permission to execute this request.
-    :raises HTTPNotFound: if the specified user name or token does not correspond to any existing user.
+    :raises HTTPNotFound: if the specified user name does not correspond to any existing user.
     """
     user_name = get_value_matchdict_checked(request, user_name_key)
     return get_user(request, user_name)

magpie/api/schemas.py

@@ -1456,7 +1456,7 @@ class User_PUT_BadRequestResponseSchema(colander.MappingSchema):
 
 
 class User_PUT_ForbiddenResponseSchema(colander.MappingSchema):
-    description = "Failed user verification with db."
+    description = "User name update not allowed."
     header = HeaderResponseSchema()
     body = BaseResponseBodySchema(code=HTTPForbidden.code, description=description)
 
@@ -1513,7 +1513,7 @@ class User_DELETE_OkResponseSchema(colander.MappingSchema):
 
 
 class User_DELETE_ForbiddenResponseSchema(colander.MappingSchema):
-    description = "Delete user by name refused by db."
+    description = "User could not be deleted."
     header = HeaderResponseSchema()
     body = BaseResponseBodySchema(code=HTTPForbidden.code, description=description)
 

magpie/app.py

@@ -52,6 +52,10 @@ def main(global_config=None, **settings):  # noqa: F811
     # with a new engine class and logging settings don't get re-evaluated/applied
     db_session = get_db_session_from_config_ini(config_ini, settings_override=sa_settings)
 
+    print_log("Validate settings that require explicit definitions...", LOGGER)
+    for req_config in ["MAGPIE_SECRET", "MAGPIE_ADMIN_USER", "MAGPIE_ADMIN_PASSWORD"]:
+        get_constant(req_config, settings_container=settings, raise_missing=True, raise_not_set=True)
+
     print_log("Register default users...", LOGGER)
     register_default_users(db_session=db_session, settings=settings)
 

magpie/constants.py

@@ -83,11 +83,11 @@ def _get_default_log_level():
 # variables from magpie.env
 # ===========================
 MAGPIE_URL = os.getenv("MAGPIE_URL", None)
-MAGPIE_SECRET = os.getenv("MAGPIE_SECRET", "seekrit")
+MAGPIE_SECRET = os.getenv("MAGPIE_SECRET", "")
 MAGPIE_COOKIE_NAME = os.getenv("MAGPIE_COOKIE_NAME", "auth_tkt")
 MAGPIE_COOKIE_EXPIRE = os.getenv("MAGPIE_COOKIE_EXPIRE", None)
-MAGPIE_ADMIN_USER = os.getenv("MAGPIE_ADMIN_USER", "admin")
-MAGPIE_ADMIN_PASSWORD = os.getenv("MAGPIE_ADMIN_PASSWORD", "qwerty")
+MAGPIE_ADMIN_USER = os.getenv("MAGPIE_ADMIN_USER", "")
+MAGPIE_ADMIN_PASSWORD = os.getenv("MAGPIE_ADMIN_PASSWORD", "")
 MAGPIE_ADMIN_EMAIL = "{}@mail.com".format(MAGPIE_ADMIN_USER)
 MAGPIE_ADMIN_GROUP = os.getenv("MAGPIE_ADMIN_GROUP", "administrators")
 MAGPIE_ANONYMOUS_USER = os.getenv("MAGPIE_ANONYMOUS_USER", "anonymous")
@@ -176,10 +176,10 @@ def get_constant(constant_name,             # type: Str
     :param settings_name: alternative name for `settings` if specified
     :param default_value: default value to be returned if not found anywhere, and exception raises are disabled.
     :param raise_missing: raise exception if key is not found anywhere
-    :param print_missing: print message if key is not found anywhere, return `None`
-    :param raise_not_set: raise an exception if the found key is None, search until last case if previous are `None`
+    :param print_missing: print message if key is not found anywhere, return ``None``
+    :param raise_not_set: raise an exception if the found key is ``None``, search until last case if others are ``None``
     :returns: found value or `default_value`
-    :raises: according message based on options (by default raise missing/`None` value)
+    :raises: according message based on options (by default raise missing/``None`` value)
     """
     from magpie.utils import get_settings, raise_log, print_log  # pylint: disable=C0415  # avoid circular import error
 

magpie/ui/home/static/style.css

@@ -183,7 +183,7 @@ table tr:nth-child(even) {
 }
 
 .panel_heading {
-    background-color: #f5f5f5;
+    background-color: #F5F5F5;
     border-bottom: 1px solid transparent;
     border-top-left-radius: 3px;
     border-top-right-radius: 3px;

magpie/ui/user/templates/edit_current_user.mako

@@ -11,13 +11,16 @@
 
 
 <div class="panel_box">
-    <!-- # FIXME: should we have a route that allows user to unregister itself?
+    <!-- # FIXME: implement with better warning (alert), API route supports operation
+        (admin is immediate delete, but we should confirm user self-delete beforehand just in case)
+    -->
+    <!--
     <form id="delete_user" action="${request.path}" method="post">
         <div class="panel_heading">
             <span class="panel_title">User: </span>
             <span class="panel_value">${user_name}</span>
             <span class="panel_heading_button">
-                <input type="submit" value="Delete" name="delete" class="button delete">
+                <input type="submit" value="Delete Account" name="delete" class="button delete">
             </span>
         </div>
     </form>

magpie/ui/utils.py

@@ -85,23 +85,26 @@ def wrap(*args, **kwargs):
 
 class BaseViews(object):
     """Base methods for Magpie UI pages."""
+    MAGPIE_FIXED_GROUP_MEMBERSHIPS = []
+    MAGPIE_FIXED_GROUP_EDITS = []
 
     def __init__(self, request):
         self.request = request
         self.magpie_url = get_magpie_url(request.registry)
         self.logged_user = get_logged_user(request)
 
-        self.MAGPIE_FIXED_GROUP_MEMBERSHIPS = [
-            get_constant("MAGPIE_ANONYMOUS_GROUP", settings_container=request),
-        ]
-        """Special groups membership that cannot be edited."""
+        anonymous = get_constant("MAGPIE_ANONYMOUS_GROUP", settings_container=request)
+        admin = get_constant("MAGPIE_ADMIN_GROUP", settings_container=request)
+        self.MAGPIE_FIXED_GROUP_MEMBERSHIPS = [anonymous]   # special groups membership that cannot be edited
+        self.MAGPIE_FIXED_GROUP_EDITS = [anonymous, admin]  # special groups that cannot be edited
 
     def add_template_data(self, data=None):
         # type: (Optional[Dict[Str, Any]]) -> Dict[Str, Any]
         """Adds required template data for the 'heading' mako template applied to every UI page."""
         all_data = data or {}
         all_data.setdefault("MAGPIE_SUB_TITLE", "Administration")
         all_data.setdefault("MAGPIE_FIXED_GROUP_MEMBERSHIPS", self.MAGPIE_FIXED_GROUP_MEMBERSHIPS)
+        all_data.setdefault("MAGPIE_FIXED_GROUP_EDITS", self.MAGPIE_FIXED_GROUP_EDITS)
         magpie_logged_user = get_logged_user(self.request)
         if magpie_logged_user:
             all_data.update({"MAGPIE_LOGGED_USER": magpie_logged_user.user_name})

magpie/utils.py

@@ -27,6 +27,7 @@
 
 if TYPE_CHECKING:
     # pylint: disable=W0611,unused-import
+    from typing import NoReturn  # noqa: F401
     from magpie.typedefs import (  # noqa: F401
         Any, AnyKey, Str, List, Optional, Type, Union,
         AnyResponseType, AnyHeadersType, LoggerType, CookiesType, SettingsType, AnySettingsContainer,
@@ -64,6 +65,10 @@ def get_logger(name, level=None):
 
 def print_log(msg, logger=None, level=logging.INFO):
     # type: (Str, Optional[LoggerType], int) -> None
+    """
+    Logs the requested message to the logger and optionally enforce printing to the console according to configuration
+    value defined by ``MAGPIE_LOG_PRINT``.
+    """
     # pylint: disable=C0415     # cannot use 'get_constant', recursive call
     from magpie.constants import MAGPIE_LOG_PRINT
 
@@ -79,7 +84,8 @@ def print_log(msg, logger=None, level=logging.INFO):
 
 
 def raise_log(msg, exception=Exception, logger=None, level=logging.ERROR):
-    # type: (Str, Optional[Type[Exception]], Optional[LoggerType], int) -> None
+    # type: (Str, Optional[Type[Exception]], Optional[LoggerType], int) -> NoReturn
+    """Logs the provided message to the logger and raises the corresponding exception afterwards."""
     if not logger:
         logger = get_logger(__name__)
     logger.log(level, msg)
@@ -173,16 +179,16 @@ def fuzzy_name(name):
 def convert_response(response):
     # type: (AnyResponseType) -> Response
     """
-    Converts a ``response`` implementation (e.g.: ``requests.Response``) to an equivalent ``pyramid.response.Response``
-    version.
+    Converts a ``response`` implementation (e.g.: :class:`requests.Response`) to an equivalent
+    :class:`pyramid.response.Response` object.
     """
     if isinstance(response, Response):
         return response
     json_body = get_json(response)
     pyramid_response = Response(body=json_body, headers=response.headers)
     if hasattr(response, "cookies"):
         for cookie in response.cookies:
-            pyramid_response.set_cookie(name=cookie.name, value=cookie.value, overwrite=True)
+            pyramid_response.set_cookie(name=cookie.name, value=cookie.value, overwrite=True)  # noqa
     if isinstance(response, HTTPException):
         for header_name, header_value in response.headers._items:  # noqa # pylint: disable=W0212
             if header_name.lower() == "set-cookie":
@@ -217,7 +223,7 @@ def get_admin_cookies(container, verify=True, raise_message=None):
 def get_settings(container):
     # type: (AnySettingsContainer) -> SettingsType
     if isinstance(container, (Configurator, Request)):
-        return container.registry.settings
+        return container.registry.settings  # noqa
     if isinstance(container, Registry):
         return container.settings
     if isinstance(container, dict):