search

Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.06k stars 1.19k forks source link

[@azure/notification-hubs] Client sendNotification method considered harmful #30140

Closed gunzip closed 2 weeks ago

gunzip commented 3 months ago

The sendNotification method has an optional parameter. When omitted, ie. by mistake, causes a broadcast of all push notifications. This is very dangerous and could lead to unintended notifications being sent to all users, potentially millions.

I propose either: a) Creating a separate method, broadcastNotification, that handles broadcast notifications without optional parameters. b) Making the broadcasting opt-in by requiring the second parameter to be mandatory even for broadcasting.

This change would prevent accidental broadcasts and improve the safety of the notification sending process.

github-actions[bot] commented 3 months ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @tjsomasundaram.

mpodwysocki commented 3 months ago

@gunzip We state this very clearly in our documentation about the send operations and in particular about Broadcast Send. In addition, we have many samples on how to send using our existing methods such as the following:

Existing clients for Java and .NET do not disambiguate between these various methods and are polymorphic in nature such as allowing broadcast, direct send and tag based send through the same method. We do not see the need to change this when the documentation is clear about how to use which send operation.

gunzip commented 3 months ago

I'm aware of the documentation and samples. I'm here advocating for a safer design to avoid being an optional argument away from a epic disaster. Even informed developers can make mistakes. If other SDKs have similar error-prone designs, it might be worth considering update their public interfaces as well. The intention to send a push notification to millions of users should, imho, be deliberate and opt-in.

Notably, the previous NH SDK prioritizes the tag argument, which is clever: https://github.com/MarkPieszak/DefinitelyTyped/blob/master/azure-sb/azure-sb.d.ts#L30

Thank you for the quick response anyway!

mpodwysocki commented 3 months ago

@gunzip We understand the concern, however, to introduce more behavior at this point would be a breaking change for us which we would rather not impose upon our customers. We will keep this in mind should other breaking changes be necessary for this SDK.

xirzec commented 3 months ago

@gunzip I sympathize with your point, and I do think if this had been raised in the initial API review of this package (and its counterparts in other languages) it would be worth adjusting to avoid such accidents.

@mpodwysocki Would this be worth considering as part of the 2.0 changes for the LRO work in https://github.com/Azure/azure-sdk-for-js/pull/29850/files ?

mpodwysocki commented 2 weeks ago

Fixed as per #31102 and can be tested via our nightly releases