Setting isEncrypted:false in notify: verb doesn't actually set it to false.

[JeremyTubongbanua]

In the above image, I sent notification with isEncrypted:false

Then in the receiving notification has isEncrypted:true

Intended behaviour:

Running notify:isEncrypted:false:<...>

Should have a receiving notification of:

notification:{..., "isEncrypted":false, ...}

[XavierChanth]

• P1 to look at it, then reevaluate the priority of the underlying issue.

[srieteja]

Could not get to this in the previous sprint, will be taking this up first in PR88.

[gkc]

Hi @srieteja I've been looking at this as it came up in another context. I'll pick it up this sprint.

Summary of the current behaviour for 'update' notifications

• at_secondary_server NotifyVerbHandler._getIsEncrypted ignores whatever it has been sent and always sets isEncrypted to 'true'
• at_client NotificationServiceImpl.notify always encrypts the value but does not set isEncrypted in the metadata. Thus, usually (unless app code explicitly sets metadata.isEncrypted to true), metadata.isEncrypted is apparently false (default value) but the value has in fact been encrypted
• at_commons NotifyVerbBuilder.buildCommand only includes isEncrypted in the generated command if metadata.isEncrypted is true (which it ~never is, right now)
• at_secondary_server ResourceManager.prepareNotifyCommandBody only includes isEncrypted in the generated command if isEncrypted is true

Summary of the changes I have in mind:

• at_secondary_server: these are the most important changes. Once implemented, the problem Jeremy encountered will be resolved
  • NotifyVerbHandler._getIsEncrypted
    • if a value for isEncrypted was set in the received command, respect it, otherwise default it to 'true' (current behaviour)
  • ResourceManager.prepareNotifyCommandBody
    • always include isEncrypted in the generated command
• at_client
  • NotificationServiceImpl.notify
    • add an optional bool parameter encryptValue default it to true
    • set the metadata.isEncrypted to the value of encryptValue
  • NotificationRequestTransformer.transform
    • if metadata.isEncrypted is true then encrypt the value, otherwise do not
• at_commons
  • NotifyVerbBuilder.buildCommand
    • always include isEncrypted in the command (currently it is only sent if isEncrypted is true, which it typically never is, right now)

In the distant future, once all of the client-side sdks are doing the right thing with their generated notify commands, we can default isEncrypted to 'false' if it is not set in the command but for now we have to continue to default it to 'true' so we don't break the world.

I need to think a little more about the sequencing of the at_commons and at_client changes; what we want to ensure is that we only ever send a notify command with isEncrypted:false when that is genuinely the case. We will likely need to do an at_commons major version change, although we may be able to do something fiddly in at_client to ensure the right behaviour

[gkc]

See also atsign-foundation/at_client_sdk#1332

[gkc]

• at_server part is complete, pending review and merge
• at_client part will be completed this sprint

[gkc]

• at_server part is complete, have requested review
• at_client part will be merged once the at_server part has been rolled out

[murali-shris]

@JeremyTubongbanua please verify this issue with latest published version of at_client

[JeremyTubongbanua]

Sure, will pick this up this sprint @murali-shris