Complete rewrite with a focus on security #232
Conversation
|
Sorry @Mogost and @utapyngo. Let me explain: I took the whole weekend to dive into the security aspects of this package. There were many problems. None of which were severe, but needed to be addressed. After some contemplation and some trial an error I ended up removing everything it writing it from scratch. I tried to keep the existing functionality, while making simpler. A good start to review is the documentation. I put a lot of time into that. However, if you don't feel like reviewing this, please say so. I don't really know how to split this into separate commits, but I can certainly try. That being said, a lot of the new lines come from the NPM package lock. Best, PS: I would create an alpha release, should we decide to merge this, to allow early adopters to play around a little easier. |
Codecov Report
@@ Coverage Diff @@
## build #232 +/- ##
==========================================
+ Coverage 88.51% 91.13% +2.61%
==========================================
Files 7 11 +4
Lines 209 282 +73
==========================================
+ Hits 185 257 +72
- Misses 24 25 +1
Continue to review full report at Codecov.
|
codingjoe
force-pushed
the
v4-alpha
branch
5 times, most recently
from
0f25b0d
to
07f6be9
Compare
|
I split the distribution part from this PR to make it easiert to review. |
|
@codingjoe This version is not compatible with django-hijack-admin |
|
OK, but that would certainly be another commit ;) I'll try to get into it, this weekend :P |
Then a few more words about rebasing commits. On the one hand it's good that the history looks nice, but on the other hand, when the rebase happens during a review, it's a little hard to track down the changes. |
codingjoe
force-pushed
the
build
branch
4 times, most recently
from
388177e
to
cca3183
Compare
codingjoe
force-pushed
the
build
branch
2 times, most recently
from
d7d7b0c
to
cabe64f
Compare
|
@Mogost, I rebased this once and pointed the PR back to the main branch. I will stop rebasing from now on until it's time to merge. |
The old design was based on a lot of untested behavior that has since been included in Django itself, including proper testing and security oversight. This refactoring uses those new tools and aims to greatly simplify the overall design. This simplification should keep potential exposior to a minimum. As a result almost all settings have been dropped, infavor of a simple permission callback and a notification template. Both can be overriden in a users application to customize behavior as need. The documenation is completly rewritten too. It may server as a good starting point to understand this change. Changes in a nutshell: * Add Material style snackback notification * Use permission callbacks instead of settings * Provide permission callback for convenience * Render and inject notification via middleware * Use Django class based views and mixins for permission handling * Update the documentation to reflect new design * Compile gettext messages during release * Switch to SCSS and compile during release * Add msgcheck linter for translations * Add styleling as a SCSS linter * Update translations Co-Authored-By: Pavel Torbeev <p.torbeev@gmail.com>
Co-authored-by: Alexandr Artemyev <mogost@gmail.com>
|
Awesome work! @codingjoe |
The old design was based on a lot of untested behavior that has
since been included in Django itself, including proper testing
and security oversight.
This refactoring uses those new tools and aims to greatly simplify
the overall design. This simplification should keep potential
exposior to a minimum.
As a result almost all settings have been dropped, infavor
of a simple permission callback and a notification template.
Both can be overriden in a users application to customize
behavior as need.
The documenation is completly rewritten too. It may server
as a good starting point to understand this change.
Changes in a nutshell: