Add MERCs 5 and 6
Signed-off-by: Roberto Rosario <Roberto.Rosario.Gonzalez@gmail.com>
This commit is contained in:
@@ -86,6 +86,7 @@
|
|||||||
user edit permission are now required.
|
user edit permission are now required.
|
||||||
* Add keyword arguments to messages uses.
|
* Add keyword arguments to messages uses.
|
||||||
* Add keyword arguments to the reverse use in views.
|
* Add keyword arguments to the reverse use in views.
|
||||||
|
* Add MERCs 5 and 6.
|
||||||
|
|
||||||
3.1.11 (2019-04-XX)
|
3.1.11 (2019-04-XX)
|
||||||
===================
|
===================
|
||||||
|
|||||||
@@ -31,9 +31,9 @@ for Mayan EDMS. Most MERCs will be Feature MERCs.
|
|||||||
2. An **Informational** MERC describes a Mayan EDMS design issue, or
|
2. An **Informational** MERC describes a Mayan EDMS design issue, or
|
||||||
provides general guidelines or information to the Mayan EDMS community,
|
provides general guidelines or information to the Mayan EDMS community,
|
||||||
but does not propose a new feature. Informational MERCs do not
|
but does not propose a new feature. Informational MERCs do not
|
||||||
necessarily represent a community consensus or
|
necessarily represent a community consensus or recommendation, so users
|
||||||
recommendation, so users and implementers are free to ignore
|
and implementers are free to ignore Informational MERCs or follow their
|
||||||
Informational MERCs or follow their advice.
|
advice.
|
||||||
|
|
||||||
3. A **Process** MERC describes a process surrounding Mayan EDMS, or
|
3. A **Process** MERC describes a process surrounding Mayan EDMS, or
|
||||||
proposes a change to (or an event in) a process. Process MERCs are
|
proposes a change to (or an event in) a process. Process MERCs are
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
=====================
|
====================
|
||||||
MERC 2: Test writing
|
MERC 2: Test writing
|
||||||
=====================
|
====================
|
||||||
|
|
||||||
:MERC: 2
|
:MERC: 2
|
||||||
:Author: Michael Price
|
:Author: Michael Price
|
||||||
|
|||||||
149
docs/mercs/0005-explicit-arguments.rst
Normal file
149
docs/mercs/0005-explicit-arguments.rst
Normal file
@@ -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<pk>[0-9]+)/versions/(?P<document_version_pk>[0-9]+)/pages/(?P<document_page_pk>[0-9]+)/image/$',
|
||||||
|
name='documentpage-image', view=APIDocumentPageImageView.as_view()
|
||||||
|
),
|
||||||
81
docs/mercs/0006-lower-information-disclose.rst
Normal file
81
docs/mercs/0006-lower-information-disclose.rst
Normal file
@@ -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)
|
||||||
@@ -20,6 +20,8 @@ Accepted
|
|||||||
../mercs/0001-merc-process
|
../mercs/0001-merc-process
|
||||||
../mercs/0002-test-writing
|
../mercs/0002-test-writing
|
||||||
../mercs/0003-using-javascript-libraries
|
../mercs/0003-using-javascript-libraries
|
||||||
|
../mercs/0005-explicit-arguments
|
||||||
|
../mercs/0006-lower-information-disclose
|
||||||
|
|
||||||
Draft
|
Draft
|
||||||
-----
|
-----
|
||||||
@@ -49,3 +51,5 @@ Feature
|
|||||||
|
|
||||||
../mercs/0002-test-writing
|
../mercs/0002-test-writing
|
||||||
../mercs/0003-using-javascript-libraries
|
../mercs/0003-using-javascript-libraries
|
||||||
|
../mercs/0005-explicit-arguments
|
||||||
|
../mercs/0006-lower-information-disclose
|
||||||
|
|||||||
@@ -63,5 +63,5 @@ Changes needed:
|
|||||||
the Role model's permissions many to many field.
|
the Role model's permissions many to many field.
|
||||||
4. Update the ``AccessControlList`` models roles field to point to the group
|
4. Update the ``AccessControlList`` models roles field to point to the group
|
||||||
models.
|
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.
|
``AccessControlList`` model manager methods.
|
||||||
|
|||||||
@@ -118,6 +118,7 @@ Other changes
|
|||||||
user edit permission are now required.
|
user edit permission are now required.
|
||||||
* Add keyword arguments to messages uses.
|
* Add keyword arguments to messages uses.
|
||||||
* Add keyword arguments to the reverse use in views.
|
* Add keyword arguments to the reverse use in views.
|
||||||
|
* Add MERCs 5 and 6.
|
||||||
|
|
||||||
Removals
|
Removals
|
||||||
--------
|
--------
|
||||||
|
|||||||
Reference in New Issue
Block a user