Return correct codes an fix bugs #325
Conversation
kqueen/auth/common.py
Outdated
| password = str(_password).encode('utf-8') | ||
| encrypted = bcrypt.hashpw(password, bcrypt.gensalt(rounds)).decode('utf-8') | ||
| return encrypted | ||
| if _password: |
There was a problem hiding this comment.
why we need this?
seems previous case failed with valid traceback in case of empty pass
There was a problem hiding this comment.
encrypted successfully generated in case of None
There was a problem hiding this comment.
if so, its better to set:
def encrypt_password(_password=None):
There was a problem hiding this comment.
and you still get encrypted password for None
There was a problem hiding this comment.
maybe its better to deny None-passwords at all?
i mean like that
@@ -512,7 +512,7 @@ def user_password_update(pk):
if not isinstance(data, dict):
abort(400)
- obj.password = encrypt_password(data.get('password'))
+ obj.password = encrypt_password(data['password'])
try:
obj.save()```
There was a problem hiding this comment.
your fix is good, but seems problem is deeper
There was a problem hiding this comment.
you'll get key error and request will not finish as expected, now 400 - bad request is returned during modal validation - password is None but it is required in Model
There was a problem hiding this comment.
what about that?
diff --git a/kqueen/blueprints/api/views.py b/kqueen/blueprints/api/views.py
index cc63da4..69194a4 100644
--- a/kqueen/blueprints/api/views.py
+++ b/kqueen/blueprints/api/views.py
@@ -512,6 +512,9 @@ def user_password_update(pk):
if not isinstance(data, dict):
abort(400)
+ if data.get('password') is None:
+ abort(400, description='Password required')
+
obj.password = encrypt_password(data.get('password'))
try:
(END)
| @@ -278,6 +278,11 @@ def dispatch_request(self, *args, **kwargs): | |||
| try: | |||
| self.save_object() | |||
| self.after_save() | |||
| except ValueError as e: | |||
| if 'unique' in str(e): | |||
There was a problem hiding this comment.
IMO its bad case to manage error codes by string-checking, traceback can change when package updates
and IMO both this cases can be managed by 400 code
There was a problem hiding this comment.
ok, I'll think how it will be better to handle that
| @@ -450,4 +450,4 @@ def namespace(self): | |||
| Returns: | |||
| str: Namespace (from organization) | |||
| """ | |||
| return self.organization.namespace | |||
| return self.organization.namespace if self.organization else None | |||
There was a problem hiding this comment.
Maybe we remove this method? it contains ~10 lines, but useful part is self.organization.namespace
There was a problem hiding this comment.
It is a property so that all objects are consistent and have namespace attribute
katyafervent
force-pushed
the
return-correct-codes
branch
from
9817fe2
to
64eaadc
Compare
Do not log exception on the expected situation.
katyafervent
force-pushed
the
return-correct-codes
branch
from
64eaadc
to
80471d4
Compare
No description provided.