patternpythondjangoMinor
Django API Implementation
Viewed 0 times
implementationapidjango
Problem
I am building a backend to a mobile application that will be hosted on a Django site connected to a PostgreSQL database.
I have never built anything to accomplish this before and this is my first go at it. I have not studied this type of application before so please let me know if the below code follows best practices for this task, is secure, and doesn't have any obvious problems. If there is an industry standard for this that is very different, please let me know what it is and why it is preferred.
My client connects via the view readJSON:
I have a separate class that holds all the code to handle the incoming JSON requests. This is one such function.
```
def register(self, c, json_data):
import bcrypt as bc
salt = bc.gensalt()
pwrd = json_data['data']['password']
hash = bc.hashpw(pwrd, salt)
try:
c.execute("INSERT INTO profiles VALUES (DEFAULT, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s)",
(json_data['data']['email'], hash, salt, json_data['data']['date_created'],
json_data['data']['state_lvl'], True,
json_data['data']['first_name'], json_data['data']['last_name'],
json_data['data']['sex'], json_data['data']['age'],
json_data['data']['state'], json_data['data']['country'],
json_data['data']['language'], json_data['data']['device_type'],
I have never built anything to accomplish this before and this is my first go at it. I have not studied this type of application before so please let me know if the below code follows best practices for this task, is secure, and doesn't have any obvious problems. If there is an industry standard for this that is very different, please let me know what it is and why it is preferred.
My client connects via the view readJSON:
@csrf_exempt
def readJSON(request):
if request.method == 'POST':
try:
c = connections['postgres_db'].cursor()
json_data = json.loads(request.body)
if json_data['tag'] == 'register':
return HttpResponse(cc().register(c, json_data))
elif json_data['tag'] == 'syncprofile':
return HttpResponse(cc().syncProfile(c, json_data))
################ A long list of other possible tags
else:
raise Http404
finally:
c.close()
else:
raise Http404I have a separate class that holds all the code to handle the incoming JSON requests. This is one such function.
```
def register(self, c, json_data):
import bcrypt as bc
salt = bc.gensalt()
pwrd = json_data['data']['password']
hash = bc.hashpw(pwrd, salt)
try:
c.execute("INSERT INTO profiles VALUES (DEFAULT, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s)",
(json_data['data']['email'], hash, salt, json_data['data']['date_created'],
json_data['data']['state_lvl'], True,
json_data['data']['first_name'], json_data['data']['last_name'],
json_data['data']['sex'], json_data['data']['age'],
json_data['data']['state'], json_data['data']['country'],
json_data['data']['language'], json_data['data']['device_type'],
Solution
readJSON()
Returning HTTP 404 ("Not Found") is improper, unless you are deliberately sending obfuscatory responses as a kind of security-by-obscurity measure. If you require a POST, and the client sends anything but a POST, the proper response should HTTP 405 ("Method Not Allowed"). For a body that does not have an appropriate JSON tag, I think that the appropriate response would be a non-specific HTTP 400 ("Bad Request").
In if-else branches, prefer to put the branch with less code first, to get it out of the way and reduce mental load. Usually, that means putting the error handler first. As a bonus, you can eliminate one level of indentation.
Instead of the try-finally construct, use a
Assuming that there is an exact correspondence between the JSON tags and the method names of
register()
Imports should be listed at the top of the file. I wouldn't alias
There are a lot of columns to be inserted, and a long parameter list. I think it would be less error prone if you used named placeholders.
It's a bit odd that you let the client specify the profile creation date. I would just let the database issue a timestamp.
I think that the long parameter list is indicative of a problematic database schema. If you ever want to add another attribute to the user profile, that would require a schema alteration. The
Returning HTTP 404 ("Not Found") is improper, unless you are deliberately sending obfuscatory responses as a kind of security-by-obscurity measure. If you require a POST, and the client sends anything but a POST, the proper response should HTTP 405 ("Method Not Allowed"). For a body that does not have an appropriate JSON tag, I think that the appropriate response would be a non-specific HTTP 400 ("Bad Request").
In if-else branches, prefer to put the branch with less code first, to get it out of the way and reduce mental load. Usually, that means putting the error handler first. As a bonus, you can eliminate one level of indentation.
Instead of the try-finally construct, use a
with block for the database cursor.Assuming that there is an exact correspondence between the JSON tags and the method names of
cc(), you could dispatch dynamically. (Your syncProfile() method doesn't match 'syncprofile', but you could rename your methods accordingly.)@csrf_exempt
def readJSON(request):
if request.method != 'POST':
raise Http405
json_data = json.loads(request.body)
# Whitelist allowable function calls for security
if json_data['tag'] not in ['register', 'syncprofile', … ]:
raise Http400
cc_method = getattr(cc(), json_data['tag'])
with connections['postgres_db'].cursor() as c:
return HttpResponse(cc_method(c, json_data))register()
Imports should be listed at the top of the file. I wouldn't alias
bcrypt as bc, as the latter sacrifices clarity just to save a few characters.There are a lot of columns to be inserted, and a long parameter list. I think it would be less error prone if you used named placeholders.
import bcrypt
def register(self, c, json_data):
attr = dict(json_data['data'])
attr['salt'] = bcrypt.gensalt()
password = attr['password']
attr['hash'] = bcrypt.hashpw(password, attr['salt'])
attr['true'] = True
try:
c.execute("""INSERT INTO profiles VALUES (DEFAULT
, %(email)s
, %(hash)s
, %(salt)s
, %(date_created)s
, %(state_lvl)s
, %(true)s
, %(first_name)s
, %(last_name)s
, %(sex)s
, %(age)s
, %(state)s
, %(country)s
, %(language)s
, %(device_type)s
, %(device)s
, %(device_width)s
, %(device_height)s
, %(device_screen_density)s
)""", attr);
json_response = json.dumps({"success": 1, "errors": 0})
except Exception, e:
json_response = json.dumps({"success": 0, "errors": 1, "msg": str(e)})
return HttpResponse(json_response, mimetype="application/json")It's a bit odd that you let the client specify the profile creation date. I would just let the database issue a timestamp.
I think that the long parameter list is indicative of a problematic database schema. If you ever want to add another attribute to the user profile, that would require a schema alteration. The
profiles table should probably be split into an accounts table containing the essentials and an account_attributes table that stores arbitrary key-value pairs.CREATE TABLE accounts
( id SERIAL PRIMARY KEY
, email TEXT NOT NULL
, hash TEXT NOT NULL
, salt TEXT NOT NULL
, date_created DATE NOT NULL DEFAULT statement_timestamp()
);
CREATE TABLE account_attributes
( account_id INTEGER NOT NULL
, attribute TEXT NOT NULL
, value TEXT NOT NULL
, PRIMARY KEY (account_id, attribute)
, FOREIGN KEY (account_id) REFERENCES accounts (id)
);Code Snippets
@csrf_exempt
def readJSON(request):
if request.method != 'POST':
raise Http405
json_data = json.loads(request.body)
# Whitelist allowable function calls for security
if json_data['tag'] not in ['register', 'syncprofile', … ]:
raise Http400
cc_method = getattr(cc(), json_data['tag'])
with connections['postgres_db'].cursor() as c:
return HttpResponse(cc_method(c, json_data))import bcrypt
def register(self, c, json_data):
attr = dict(json_data['data'])
attr['salt'] = bcrypt.gensalt()
password = attr['password']
attr['hash'] = bcrypt.hashpw(password, attr['salt'])
attr['true'] = True
try:
c.execute("""INSERT INTO profiles VALUES (DEFAULT
, %(email)s
, %(hash)s
, %(salt)s
, %(date_created)s
, %(state_lvl)s
, %(true)s
, %(first_name)s
, %(last_name)s
, %(sex)s
, %(age)s
, %(state)s
, %(country)s
, %(language)s
, %(device_type)s
, %(device)s
, %(device_width)s
, %(device_height)s
, %(device_screen_density)s
)""", attr);
json_response = json.dumps({"success": 1, "errors": 0})
except Exception, e:
json_response = json.dumps({"success": 0, "errors": 1, "msg": str(e)})
return HttpResponse(json_response, mimetype="application/json")CREATE TABLE accounts
( id SERIAL PRIMARY KEY
, email TEXT NOT NULL
, hash TEXT NOT NULL
, salt TEXT NOT NULL
, date_created DATE NOT NULL DEFAULT statement_timestamp()
);
CREATE TABLE account_attributes
( account_id INTEGER NOT NULL
, attribute TEXT NOT NULL
, value TEXT NOT NULL
, PRIMARY KEY (account_id, attribute)
, FOREIGN KEY (account_id) REFERENCES accounts (id)
);Context
StackExchange Code Review Q#44244, answer score: 2
Revisions (0)
No revisions yet.