Conversation
| return user, None | ||
| return user, profile_res.data | ||
| except Exception: | ||
| return None, None |
There was a problem hiding this comment.
I recommend raising a unique exception, like AuthRequiredError or something, rather than ever returning (None, None).
There are several advantages to doing this.
First, you can register an error handler function with the Flask instance. The uncaught AuthRequiredError can then be handled via exactly one code path, rather than handling (None, None) in every individual route. See the Flask .register_error_handler() docs for more info.
Second, eliminating (None, None) as a return value makes the types returned by this function very consistent. Callers can know with certainty what they're getting back, and this can be enforced with type checkers.
| orgs = [] | ||
| for perm in (perms_res.data or []): | ||
| org = perm.get('organization') | ||
| if org and org.get('deleted_at') is None: | ||
| org['permission_level'] = perm.get('permission_level') | ||
| orgs.append(org) |
There was a problem hiding this comment.
This loop is filtering out orgs that haven't been deleted, but that should happen in the database query, not in a Python loop.
I recommend updating the query to only include orgs whose deleted_at value IS NULL.
| if perms_res.data and len(perms_res.data) > 0: | ||
| return True, perms_res.data[0].get('permission_level') | ||
|
|
||
| return False, None |
There was a problem hiding this comment.
As noted in a previous comment, this function should raise an exception.
This standardizes handling and types.
| return jsonify({"error": "Creating an organization requires Program Manager or Admin privileges"}), 403 | ||
|
|
||
| data = request.json or {} | ||
| organization_name = (data.get('organization_name') or '').strip() |
There was a problem hiding this comment.
This can crash if the organization_name is provided by the user but isn't a string. For example:
data = {"organization_name": 1}
(data.get("organization_name") or '').strip() # AttributeErrorAll of the code needs to check not only the existence, but also the type of the value.
This can be accomplished using e.g. a helper function, or -- and I strongly recommend doing this -- a tool like pydantic or marshmallow to validate the incoming data.
| key_res = client.table('organization_key').insert({ | ||
| 'import_key': import_key, | ||
| 'created_by_user_id': profile['id'], | ||
| 'updated_by_user_id': profile['id'] | ||
| }).execute() | ||
|
|
||
| if not key_res.data: | ||
| raise ValueError("Failed to create organization_key record") |
There was a problem hiding this comment.
Does .execute() happen in a transaction? It looks like this code path is running without a transaction, which suggests that if this first INSERT succeeds but something else fails later, there could potentially be dead records in the database.
Please double-check me on that, but that's what it looks like to me.
|
|
||
| data = request.json or {} | ||
| organization_name = (data.get('organization_name') or '').strip() | ||
| organization_slug = (data.get('organization_slug') or '').strip().lower() |
There was a problem hiding this comment.
It doesn't look like the organization_slug is getting validated. This must be done server-side.
I see that it's getting updated by Javascript in the template, but it's possible for a request to come through that contains unacceptable characters, and that needs to be guarded here.
|
Thanks @kurtmckee ! I'll aim to take some time to address this tomorrow. |
Co-authored-by: Kurt McKee <contactme@kurtmckee.org>
Merge remote-tracking branch 'origin/main' into users
2 participants
No description provided.