47ng / prisma-field-encryption

Transparent field-level encryption at rest for Prisma
https://github.com/franky47/prisma-field-encryption-sandbox
MIT License
246 stars 29 forks source link

'orderBy' conversion is wrong in case I also include a non encrypted value #65

Closed martijn-dev closed 1 year ago

martijn-dev commented 1 year ago

The situation is the following:

The prisma schema:

model Visitor {
  id             String            @id @default(uuid())
  key            String            @unique /// @encrypted
  keyHash        String?           @unique /// @encryption:hash(key)
  created_at     DateTime          @default(now())

  @@map("visitors")
}

I would like to do a findMany with a orderBy on the key and on the created_at.

I understand it is quite useless to sort on a encrypted value and I kinda understand the desision made in issue #43. If I only sort on a encrypted field I get the silent error: Error: RunningorderByon encrypted field Visitor.key is not supported (results won't be sorted).

But I will do the following:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               key: 'asc',
             },
             {
               created_at: 'asc'
             }
           ],
         })

which does not result in a silent error but in the following:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               key: 'v1.aesgcm256.xxxxxxx.8xxxxxxxxxxxxxxxxxxWhfw==',
             keyHash: 'xxxxxxxxx-hash-xxxxxxxxxxxx'
             },
             {
               created_at: 'asc'
             }
           ] 
         })

Which results in a Primsa error Argument orderBy: Got invalid value since you should only provide 1 property per object.

I would expect it to convert to:

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
             keyHash: 'asc'
             },
             {
               created_at: 'asc'
             }
           ] 
         })

or in case you want to prevent sorting on encrypted fields

this.prisma.visitor.findMany({
           take: 5,
           skip: 0,
           cursor: undefined,
           orderBy: [
             {
               created_at: 'asc'
             }
           ] 
         })

Please let me know if you need more information!

franky47 commented 1 year ago

I'm not familiar with the array syntax for orderBy, what is this supposed to do?

orderBy: [
 {
   keyHash: 'xxxxxxxxx-hash-xxxxxxxxxxxx'
 },
 {
   created_at: 'asc'
 }
] 
martijn-dev commented 1 year ago

First of all, sorry, I see I made a typo. My expected result would be:

orderBy: [
 {
   keyHash: 'asc'
 },
 {
   created_at: 'asc'
 }
]

I changed it in the original issue as well.

With an array syntax in orderBy you can order your result by multiple properties. The above example is a bit weird for this usecase.

But let's say the Visitors have a name and a created_at. And you would like to have your visitors sorted by name and after that by created_at. A query like this would work then:

orderBy: [
 {
   name: 'asc'
 },
 {
   created_at: 'desc'
 }
] 

If there are multiple visitors with the same name these will then be ordered by created_at.

https://www.prisma.io/docs/concepts/components/prisma-client/filtering-and-sorting#sorting

franky47 commented 1 year ago

I see, it makes more sense with the correction.

That being said, ordering on the hash will not give you the result you expect. While you would indeed have multiple visitors with the same name being neighbours in the resulting array, ordered by created_at, the actual names would not be ordered correctly.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 1.4.5 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

franky47 commented 1 year ago

Version 1.4.5 includes a "fix" only for the misdetection of the orderBy clause with an array syntax. I still recommend doing sorting at runtime post-query if the application requires a particular order.

martijn-dev commented 1 year ago

Yeah you're right. Sorting on the hash never results in a usefull outcome.

But should in this case the encrypted field then be taken out by the array? So your're left with just:

orderBy: [
             {
               created_at: 'asc'
             }
           ] 

or would you throw the same error as on a single orderBy query?

EDIT: Ah you're faster than light :) It became the last option. Thanks for your fast responses!

franky47 commented 1 year ago

Well I tried to get this:

orderBy: [
 {
   name: 'asc'
 },
 {
   created_at: 'desc'
 }
] 

to become this:

orderBy: [
 {
   created_at: 'desc'
 }
] 

but the overwriting mechanism is not (yet) smart enough to know how to prune an empty object or array, so I end up with this:

orderBy: [
 {
 },
 {
   created_at: 'desc'
 }
] 

which causes the query to throw, after displaying the error that orderBy is not supported on encrypted fields. I figure it's clear enough a message to not fiddle with the query further, and let the developer remove the unsupported orderBy clause.