django-wiki / django-nyt

Notification system for Django with batteries included: Email digests, user settings, JSON API
Apache License 2.0
144 stars 47 forks source link

Invalid group name #29

Closed mizi closed 7 years ago

mizi commented 7 years ago

The last in ws_disconnect causes the following exception:

TypeError: Group name must be a valid unicode string containing only alphanumerics, hyphens, or periods.

because settings.NOTIFICATION_CHANNEL is a format string.

I think this line isn't needed and was left there by mistake in 251bfc0

@channel_session_user
def ws_disconnect(message):
    """
    Connected to websocket.disconnect
    """
    logger.debug("Removing connection for user {} (disconnect)".format(message.user))
    for subscription in models.Subscription.objects.filter(settings__user=message.user):
        Group(
            settings.NOTIFICATION_CHANNEL.format(
                notification_key=subscription.notification_type.key
            )
        ).discard(message.reply_channel)
    Group(settings.NOTIFICATION_CHANNEL).discard(message.reply_channel)
benjaoming commented 7 years ago

The group is supposed to contain the notification key such that people can subscribe to only specific channels (notifications).

That should be fine, however there could be an issue with your notification key - could we somehow allow notification keys that don't conform to only alphanumerics, hyphens, or periods. ?

ztomaz commented 7 years ago

The problem is the second Group(...).discard function: Group(settings.NOTIFICATION_CHANNEL).discard(message.reply_channel), because it uses non formated settings.NOTIFICATION_CHANNEL (value is: "nyt_all-{notification_key:s}") which contains non allowed characters. I also think it was left there by mistake

benjaoming commented 7 years ago

I totally missed the point in this issue, thanks @ztomaz :)

mizi commented 7 years ago

Thanks @benjaoming, thanks @ztomaz :)