fix: include organization_id in where clause of update queries #3439
Conversation
dnephin
requested review from
BruceMacD,
jmorganca and
pdevine
as code owners
| if isOrgMember(model) { | ||
| db = ByOrgID(tx.OrganizationID())(db) | ||
| } |
There was a problem hiding this comment.
This is the bit we were missing. setOrg makes it look like we're doing the right thing, but that only sets the org_id of the new row, it doesn't force the query to only operate on an ID of the correct org.
I looked back to see if this was accidentally removed at some point, but I didn't find it, so I think it was something we missed while adding org support.
There was a problem hiding this comment.
right. setting the org is enough for inserts, but not for updates. As is it would effectively allow you to steal records from another org to your org.
So that if an ID is guessed or accidentally shared publicly, only requests authorized for that org can perform the update. Also a bunch more godoc for query.go Also remove unnecessary column aliases from a function
dnephin
force-pushed
the
dnephin/data-update-include-org-id
branch
from
8b45a79
to
b22f6dd
Compare
| @@ -103,12 +122,21 @@ func update(tx WriteTxn, item Updatable) error { | |||
| query.B(item.Table()) | |||
| query.B("SET") | |||
| query.B(columnsForUpdate(item), item.Values()...) | |||
| query.B("WHERE deleted_at is null AND id = ?;", item.Primary()) | |||
| query.B("WHERE deleted_at is null") | |||
There was a problem hiding this comment.
Could hiding deleted at on update cause issues with restoring deleted rows?
There was a problem hiding this comment.
We don't have a way to restore deleted rows through the API, right? If we wanted to do that we'd have to write a migration or do it directly in the database?
My rational for deleted_at is null is that we don't really want someone to be able to updated the deleted_at field of a row that was already deleted.
If we ever support un-delete from the API we will definitely need to change this.
If someone is able to guess the ID of a row or the ID is accidentally shared it's possible a non-authorized user could update the row using the ID.
This PR fixes the bug by setting
WHERE organization_id = ?so that only a user authorized for that org can perform updates.Also adds a bunch more godoc for
query.go.