From e7964aacdfd876e718b64c278662d9adc33f7c94 Mon Sep 17 00:00:00 2001 From: Francis Charette Migneault Date: Mon, 27 Jul 2020 20:00:07 -0400 Subject: [PATCH] force non-default secret/admin-credentials (relates to #229) + update & fix doc (#337) + add checks for udpate special users (#164) --- HISTORY.rst | 2 ++ config/magpie.ini | 2 +- docs/configuration.rst | 30 ++++++++++++------- magpie/adapter/__init__.py | 2 +- magpie/api/management/user/user_views.py | 19 ++++++++++-- magpie/api/requests.py | 9 +++++- magpie/api/schemas.py | 4 +-- magpie/app.py | 6 +++- magpie/constants.py | 12 ++++---- magpie/ui/home/static/style.css | 2 +- .../ui/user/templates/edit_current_user.mako | 7 +++-- magpie/ui/utils.py | 9 +++--- magpie/utils.py | 16 ++++++---- 13 files changed, 83 insertions(+), 37 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 02c949ebe..e71b10662 100644 --- a/HISTORY.rst +++ b/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 `_). +* 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 ~~~~~~~~~~~~~~~~~~~~~ diff --git a/config/magpie.ini b/config/magpie.ini index ef2e1bca0..ab5a16bfa 100644 --- a/config/magpie.ini +++ b/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 = diff --git a/docs/configuration.rst b/docs/configuration.rst index e055b0f3f..3fcdf42e4 100644 --- a/docs/configuration.rst +++ b/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/`` 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.* diff --git a/magpie/adapter/__init__.py b/magpie/adapter/__init__.py index 7de30d670..54e2f3503 100644 --- a/magpie/adapter/__init__.py +++ b/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) diff --git a/magpie/api/management/user/user_views.py b/magpie/api/management/user/user_views.py index e69d41f00..40c8ee768 100644 --- a/magpie/api/management/user/user_views.py +++ b/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) diff --git a/magpie/api/requests.py b/magpie/api/requests.py index 61adbb5f1..44b03a7fc 100644 --- a/magpie/api/requests.py +++ b/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) diff --git a/magpie/api/schemas.py b/magpie/api/schemas.py index 8b30b581f..9e920c104 100644 --- a/magpie/api/schemas.py +++ b/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) diff --git a/magpie/app.py b/magpie/app.py index 75538baae..43af19416 100644 --- a/magpie/app.py +++ b/magpie/app.py @@ -13,7 +13,7 @@ from magpie.helpers.register_default_users import register_default_users from magpie.register import magpie_register_permissions_from_config, magpie_register_services_from_config from magpie.security import get_auth_config -from magpie.utils import get_logger, patch_magpie_url, print_log +from magpie.utils import get_logger, patch_magpie_url, print_log, raise_log LOGGER = get_logger(__name__) @@ -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) diff --git a/magpie/constants.py b/magpie/constants.py index 8ce1265bf..ba013aadd 100644 --- a/magpie/constants.py +++ b/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 diff --git a/magpie/ui/home/static/style.css b/magpie/ui/home/static/style.css index 6724e86b9..0bac9c48a 100644 --- a/magpie/ui/home/static/style.css +++ b/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; diff --git a/magpie/ui/user/templates/edit_current_user.mako b/magpie/ui/user/templates/edit_current_user.mako index 2d4eb64ec..e801c1731 100644 --- a/magpie/ui/user/templates/edit_current_user.mako +++ b/magpie/ui/user/templates/edit_current_user.mako @@ -11,13 +11,16 @@
- +