Allow no notification address or type #197
Conversation
Don't try to send a push notifications when this happens and explicitly log when no push notification address is available
…notificationType being present
| // Filter bogus "null" and "(null)" entries that were put in the database by returning '' instead. | ||
| // There is no point in trying to send a notification to such addresses. | ||
| $notificationAddressLower = strtolower($notificationAddress); | ||
| if (($notificationAddressLower == 'null') || ($notificationAddressLower == '(null)')) { |
There was a problem hiding this comment.
First off, this is a very effective way to stop this value from being used as the notification address! One would hope the Tiqr server user storage would prevent this from happening.
Wouldn't it be better to let the source of the data guard the values being sent out? Its always a bit of a debate who is responsible for data integrity.
Finally. Have you actually seen a (null) string value? Or is this what you see in the database GUI when inspecting the field? I've never seen this happening before.
There was a problem hiding this comment.
(null) is how objective-C (used in the iOS app) converts null/nil values to string, so we see this one a lot.
The expectation of the server is that the notificationAddress is an opaque string, so that is why it did/does not enforce anything. This happening is unintended behaviour by the apps, and it has been happening in the past. We've been fixing this, but that does not change what is in the database currently. Also we're not willing to force people to update the app over this (which would mean locking them out if they don't) as it is merely an annoyance, not a security bug.
Yes, solving this in the userStorage would have been an option too. But the userStorage being an optional class, I did not want to change its interface, a bit academic, true. I actually "like" to see the wrong values appear in the data base, because this tells us when there is an issue how many accounts are affected. That is why I put the guard on the way out, not the way in.
Normally a client sends the notificationAddress and notificationType parameters with each registration and authentication response as HTTP POST parameters. In some situations however a tiqr client can not get a notification address. Different clients respond (and have responded) differently to this situation. The notificationAddress and notificationType parameters may be absent, or both or only notificationAddress maybe set to the string "(null)" or "NULL". This leads to several issues that this PR addresses:
Currently there are user entries in the user storage table that have notificationAddress set to '(null)' or 'NULL'. Stepup-tiqr tries to send a push notification to these addresses, and that leads to an error. We change this behaviour so that NULL or (null) means no push address / type so no notification is send. We explicitly log this situation.
When a client omits a notificationAddress or notificationType this currently leads to an exception and the registration will fail. We change this behaviour so that this is accepted and allow the registration and authentication to continue.