amitu / django-encrypted-id

Encrypted IDs for Django Models
BSD 2-Clause "Simplified" License
32 stars 16 forks source link

Incorporate a model's identity also in its ekey #8

Closed jatinderjit closed 7 years ago

jatinderjit commented 7 years ago

This will ensure that instances with same pk, but belonging to different models, won't share an ekey.

Also change key length from 24 to 32 (AES-196 to AES-256).

Mystic-Mirage commented 7 years ago

Hi! Why support for related field query reverted?

amitu commented 7 years ago

@jatinderjit, please explain to @Mystic-Mirage.

Hey @Mystic-Mirage, we are facing some problem at work, we will resolve it and put it back. Do

jatinderjit commented 7 years ago

@Mystic-Mirage During a chained lookup - Foo.objects.filter(bar__fieldname=value), only Foo's manager is used. Bar's manager or queryset functions are not called. When you do split by __, that happens in Foo's manager, even though it is Bar's field.

The tests were working for Foo.objects.filter(bar__ekey=value) because Foo and Bar shared the same manager. So Foo's manager knows that it has to decode and convert to id when it sees ekey.

But if we have a third model Baz with the default manager, then Baz.objects.filter(bar__ekey=value) will raise an error, because the column "ekey" doesn't exist. So this was not a generic solution. And with current API, a generic solution is probably not possible.

We've made another change to make ekey unique for different models (even for same id). This makes calling the decode function contextual - it is not model independent anymore. So decoding Bar's ekey from Foo's model is not that trivial anymore. We've to infer bar's model from Foo's queryset (though it could easily be implemented for a simple case when bar is a ForeignKey in Foo to Bar). But then again, it won't be a generic solution.

Mystic-Mirage commented 7 years ago

@jatinderjit, I see. Thanks for details!

amitu commented 7 years ago

@jatinderjit, it would be helpful if you can create a branch with a test that fails that me and @Mystic-Mirage, and probably others can take a shot at fixing.