From 0e1fe3968d221a0260e8d1a027a75d89b81a8b68 Mon Sep 17 00:00:00 2001 From: Roberto Rosario Date: Tue, 16 Apr 2019 19:26:49 -0400 Subject: [PATCH] Add MERCs 5 and 6 Signed-off-by: Roberto Rosario --- HISTORY.rst | 1 + docs/mercs/0001-merc-process.rst | 6 +- docs/mercs/0002-test-writing.rst | 4 +- docs/mercs/0005-explicit-arguments.rst | 149 ++++++++++++++++++ .../mercs/0006-lower-information-disclose.rst | 81 ++++++++++ docs/mercs/index.rst | 4 + docs/mercs/merging-roles-and-groups.rst | 2 +- docs/releases/3.2.rst | 1 + 8 files changed, 242 insertions(+), 6 deletions(-) create mode 100644 docs/mercs/0005-explicit-arguments.rst create mode 100644 docs/mercs/0006-lower-information-disclose.rst diff --git a/HISTORY.rst b/HISTORY.rst index a1cc524161..22642f8946 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -86,6 +86,7 @@ user edit permission are now required. * Add keyword arguments to messages uses. * Add keyword arguments to the reverse use in views. +* Add MERCs 5 and 6. 3.1.11 (2019-04-XX) =================== diff --git a/docs/mercs/0001-merc-process.rst b/docs/mercs/0001-merc-process.rst index 111a800e0e..85035fcfa0 100644 --- a/docs/mercs/0001-merc-process.rst +++ b/docs/mercs/0001-merc-process.rst @@ -31,9 +31,9 @@ for Mayan EDMS. Most MERCs will be Feature MERCs. 2. An **Informational** MERC describes a Mayan EDMS design issue, or provides general guidelines or information to the Mayan EDMS community, but does not propose a new feature. Informational MERCs do not -necessarily represent a community consensus or -recommendation, so users and implementers are free to ignore -Informational MERCs or follow their advice. +necessarily represent a community consensus or recommendation, so users +and implementers are free to ignore Informational MERCs or follow their +advice. 3. A **Process** MERC describes a process surrounding Mayan EDMS, or proposes a change to (or an event in) a process. Process MERCs are diff --git a/docs/mercs/0002-test-writing.rst b/docs/mercs/0002-test-writing.rst index 0e7a542863..d7fc4303e7 100644 --- a/docs/mercs/0002-test-writing.rst +++ b/docs/mercs/0002-test-writing.rst @@ -1,6 +1,6 @@ -===================== +==================== MERC 2: Test writing -===================== +==================== :MERC: 2 :Author: Michael Price diff --git a/docs/mercs/0005-explicit-arguments.rst b/docs/mercs/0005-explicit-arguments.rst new file mode 100644 index 0000000000..fbe919468d --- /dev/null +++ b/docs/mercs/0005-explicit-arguments.rst @@ -0,0 +1,149 @@ +========================== +MERC 5: Explicit arguments +========================== + +:MERC: 5 +:Author: Roberto Rosario +:Status: Accepted +:Type: Feature +:Created: 2018-12-30 +:Last-Modified: 2018-12-31 + +.. 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/0006-lower-information-disclose.rst b/docs/mercs/0006-lower-information-disclose.rst new file mode 100644 index 0000000000..25006f7086 --- /dev/null +++ b/docs/mercs/0006-lower-information-disclose.rst @@ -0,0 +1,81 @@ +================================== +MERC 6: Lower information disclose +================================== + +:MERC: 6 +:Author: Michael Price +:Status: Accepted +:Type: Feature +:Created: 2018-12-30 +:Last-Modified: 2018-12-31 + +.. 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..95830779d6 100644 --- a/docs/mercs/index.rst +++ b/docs/mercs/index.rst @@ -20,6 +20,8 @@ Accepted ../mercs/0001-merc-process ../mercs/0002-test-writing ../mercs/0003-using-javascript-libraries + ../mercs/0005-explicit-arguments + ../mercs/0006-lower-information-disclose Draft ----- @@ -49,3 +51,5 @@ Feature ../mercs/0002-test-writing ../mercs/0003-using-javascript-libraries + ../mercs/0005-explicit-arguments + ../mercs/0006-lower-information-disclose diff --git a/docs/mercs/merging-roles-and-groups.rst b/docs/mercs/merging-roles-and-groups.rst index 34bb4aea72..edc5e2e9e5 100644 --- a/docs/mercs/merging-roles-and-groups.rst +++ b/docs/mercs/merging-roles-and-groups.rst @@ -63,5 +63,5 @@ Changes needed: the Role model's permissions many to many field. 4. Update the ``AccessControlList`` models roles field to point to the group models. -5. Update the role checks in the ``check_access`` and ``filter_by_access`` +5. Update the role checks in the ``check_access`` and ``restrict_queryset`` ``AccessControlList`` model manager methods. diff --git a/docs/releases/3.2.rst b/docs/releases/3.2.rst index 70e4c23355..7ebec03ef3 100644 --- a/docs/releases/3.2.rst +++ b/docs/releases/3.2.rst @@ -118,6 +118,7 @@ Other changes user edit permission are now required. * Add keyword arguments to messages uses. * Add keyword arguments to the reverse use in views. +* Add MERCs 5 and 6. Removals --------