From 45ceab024d1131a8b47907da7371c7002f08fddf Mon Sep 17 00:00:00 2001 From: Roberto Rosario Date: Sun, 30 Dec 2018 14:14:44 -0400 Subject: [PATCH] Add two new MERC proposals Add the explicit arguments and lower information disclose MERCS. Signed-off-by: Roberto Rosario --- docs/mercs/draft-explicit-arguments.rst | 149 ++++++++++++++++++ .../draft-lower-information-disclose.rst | 81 ++++++++++ docs/mercs/index.rst | 2 + 3 files changed, 232 insertions(+) create mode 100644 docs/mercs/draft-explicit-arguments.rst create mode 100644 docs/mercs/draft-lower-information-disclose.rst diff --git a/docs/mercs/draft-explicit-arguments.rst b/docs/mercs/draft-explicit-arguments.rst new file mode 100644 index 0000000000..0fbfc57bc3 --- /dev/null +++ b/docs/mercs/draft-explicit-arguments.rst @@ -0,0 +1,149 @@ +=========================== +MERC XX: Explicit arguments +=========================== + +:MERC: XX +:Author: Roberto Rosario +:Status: Draft +:Type: Process +:Created: 2018-12-30 +:Last-Modified: 2018-12-30 + +.. contents:: Table of Contents + :depth: 3 + :local: + + +Abstract +======== + +This MERC proposes the adoption of a new methodology when performing calls. +It seeks to reduce the use of positional arguments in favor of keyword +arguments in as many places as possible. + + +Motivation +========== + +As the project grows, legibility of code becomes more important. Keyword +argument help document the use of services, clases and functions. Refactors +that affect the interface of services are also easier to find and update and +fix. Positional argument can cause a call to continue working as long as the +datatype of the argument remains the same. Usage of keyword arguments will +automatically raise and error that will prevent such situations. Keyword +argument further eliminate the relevance of position or the arguments, and +the arguments can be sorted alphabetically for easier visual scanning or by +semantic significance improving code readability. + + +Specification +============= + +Adoption of this MERC will require an audit of existing calls and the use +of the method proposed for new calls. Every call regardless of the type or +origin of the source callable will name each argument used. By type it is +meant: classes, functions, methods. Origin means: local from the project, +from the framework, third party libraries or the standard library. + + +Backwards Compatibility +======================= + +No backwards compatibility issues are expected. New errors arising from the use +if keyword arguments could be interpreted as existing latent issues that +have not been uncovered. + + +Reference Implementation +======================== + +Example: + +Before: + +.. code-block:: python + + from mayan.apps.common.classes import Template + + Template( + 'menu_main', 'appearance/menu_main.html' + ) + + +After: + +.. code-block:: python + + from mayan.apps.common.classes import Template + + Template( + name='menu_main', template_name='appearance/menu_main.html' + ) + + +When calls use a mixture or positional and keyword arguments, the keywords +arguments can only be found after the positional arguments. Complete use +of keyword arguments allow the reposition of arguments for semantic +purposes. + +Example: + +Before: + +.. code-block:: python + + from django.conf.urls import url + + from .views import AboutView, HomeView, RootView + + urlpatterns = [ + url(r'^$', RootView.as_view(), name='root'), + url(r'^home/$', HomeView.as_view(), name='home'), + url(r'^about/$', AboutView.as_view(), name='about_view'), + ] + + +After: + +.. code-block:: python + + from django.conf.urls import url + + from .views import AboutView, HomeView, RootView + + urlpatterns = [ + url(regex=r'^$', name='root', view=RootView.as_view()), + url(regex=r'^home/$', name='home', view=HomeView.as_view()), + url(regex=r'^about/$', name='about_view', view=AboutView.as_view()), + ] + + +Keyword arguments should also be used for callables that pass those to others +down the line like Django's ``reverse`` function. Any change to the name of +the ``pk`` URL parameter will raise an exception in this code alerting to +any posible incompatible use. + + +Example: + +.. code-block:: python + + def get_absolute_url(self): + return reverse( + viewname='documents:document_preview', kwargs={'pk': self.pk} + ) + + +This becomes even more important when multiple URL parameters are used. Since +the API documentation is auto generated from the code itself, it would make +sense to rename the first URL parameter from ``pk`` to ``document_pk``. Such +change will cause all address to view resolutions to break forcing their +update and allowing all consumers' interface usage to remain synchonized to the +callable's interface. + +.. code-block:: python + + url( + regex=r'^documents/(?P[0-9]+)/versions/(?P[0-9]+)/pages/(?P[0-9]+)/image/$', + name='documentpage-image', view=APIDocumentPageImageView.as_view() + ), diff --git a/docs/mercs/draft-lower-information-disclose.rst b/docs/mercs/draft-lower-information-disclose.rst new file mode 100644 index 0000000000..324ba00c21 --- /dev/null +++ b/docs/mercs/draft-lower-information-disclose.rst @@ -0,0 +1,81 @@ +=================================== +MERC XX: Lower information disclose +=================================== + +:MERC: XX +:Author: Michael Price +:Status: Draft +:Type: Process +:Created: 2018-12-30 +:Last-Modified: 2018-12-30 + +.. contents:: Table of Contents + :depth: 3 + :local: + +Abstract +======== + +This MERC proposes the use of errors that don't disclose the existance of a +resource in the event that the requester doesn't have the required credentials. + +Motivation +========== + +When an user tries to perform an action like opening a view to a document for +which the required permission is missing, a permission required or access +denied error is presented. This is semantically correct, but from the stand +point of security it is still failing because it is letting the user know +that such document exists in the first place. This MERC proposes changing the +error message for existing resource to one that doesn't divulge any information +to unauthorized parties, like "Not Found". + +Specification +============= + +Out of the 4 basic CRUD operations, Read, Update and Delete should return an +HTTP 404 error instead of an HTTP 403 error. Only the Create operation will +continue returning the current HTTP 403 error, unless it is creating a +new resource that is related to an existing resource. + +Since most view use the internal custom CRUD classes making a change to the +``ObjectPermissionCheckMixin`` class to raise an HTTP 404 on object access +failure will fulfill the proposal of this MERC. + +Adding the ``object_permission_raise_404`` class attribute and setting it +to default to False will allow fulfullin the goal of this MERC while +keeping the existing functionality intact. + + +Example: + +.. code-block:: python + + class ObjectPermissionCheckMixin(object): + """ + If object_permission_raise_404 is True an HTTP 404 error will be raised + instead of the normal 403. + """ + object_permission = None + object_permission_raise_404 = False + + def get_permission_object(self): + return self.get_object() + + def dispatch(self, request, *args, **kwargs): + if self.object_permission: + try: + AccessControlList.objects.check_access( + permissions=self.object_permission, user=request.user, + obj=self.get_permission_object(), + related=getattr(self, 'object_permission_related', None) + ) + except PermissionDenied: + if self.object_permission_raise_404: + raise Http404 + else: + raise + + return super( + ObjectPermissionCheckMixin, self + ).dispatch(request, *args, **kwargs) diff --git a/docs/mercs/index.rst b/docs/mercs/index.rst index 7366c9f6dc..f57c6ef174 100644 --- a/docs/mercs/index.rst +++ b/docs/mercs/index.rst @@ -27,6 +27,8 @@ Draft .. toctree:: :maxdepth: 1 + ../mercs/draft-explicit-arguments + ../mercs/draft-lower-information-disclose ../mercs/merging-roles-and-groups