Incorrect event fired when clear_topic_permissions executed

[bedla]

Describe the bug

Hi,

I found out that when I delete topic permissions permission.deleted event is fired instead of topic.permission.deleted.
I am not Erlang developer, but it looks like that problem is in this method https://github.com/rabbitmq/rabbitmq-server/blob/v3.13.0-rc.2/deps/rabbit/src/rabbit_auth_backend_internal.erl#L626-L643

And it should be fixed to something similar to this snippet:

clear_topic_permissions(Username, VirtualHost, Exchange, ActingUser) ->
    rabbit_log:debug("Asked to clear topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts'",
                     [Exchange, Username, VirtualHost]),
    try
        R = rabbit_db_user:clear_topic_permissions(
              Username, VirtualHost, Exchange),
        rabbit_log:info("Successfully cleared topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts'",
                        [Exchange, Username, VirtualHost]),
        rabbit_event:notify(topic_permission_deleted, [{user,  Username},
                                                       {vhost, VirtualHost},
                                                       {exchange,  Exchange},
                                                       {user_who_performed_action, ActingUser}]),
        R
    catch
        Class:Error:Stacktrace ->
            rabbit_log:warning("Failed to clear topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts': ~tp",
                               [Exchange, Username, VirtualHost, Error]),
            erlang:raise(Class, Error, Stacktrace)
    end.

What do you think? Is it bug or am I missing something?

Thx

Ivos

Reproduction steps

1. Clear topic permissions with exchange name specified
2. Wrong event is fired - see descriptiong for details

Expected behavior

topic.permission.deleted event is fired

Additional context

No response

[michaelklishin]

Your findings are correct, in fact, another function head already uses topic_permission_deleted:

clear_topic_permissions(Username, VirtualHost, ActingUser) ->
    rabbit_log:debug("Asked to clear topic permissions for user '~ts' in virtual host '~ts'",
                     [Username, VirtualHost]),
    try
        R = rabbit_db_user:clear_topic_permissions(Username, VirtualHost, '_'),
        rabbit_log:info("Successfully cleared topic permissions for user '~ts' in virtual host '~ts'",
                        [Username, VirtualHost]),
        rabbit_event:notify(topic_permission_deleted, [{user,  Username},
            {vhost, VirtualHost},
            {user_who_performed_action, ActingUser}]),
        R
    catch
        Class:Error:Stacktrace ->
            rabbit_log:warning("Failed to clear topic permissions for user '~ts' in virtual host '~ts': ~tp",
                               [Username, VirtualHost, Error]),
            erlang:raise(Class, Error, Stacktrace)
    end.

clear_topic_permissions(Username, VirtualHost, Exchange, ActingUser) ->
    rabbit_log:debug("Asked to clear topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts'",
                     [Exchange, Username, VirtualHost]),
    try
        R = rabbit_db_user:clear_topic_permissions(
              Username, VirtualHost, Exchange),
        rabbit_log:info("Successfully cleared topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts'",
                        [Exchange, Username, VirtualHost]),
        rabbit_event:notify(permission_deleted, [{user,  Username},
                                                 {vhost, VirtualHost},
                                                 {user_who_performed_action, ActingUser}]),
        R
    catch
        Class:Error:Stacktrace ->
            rabbit_log:warning("Failed to clear topic permissions on exchange '~ts' for user '~ts' in virtual host '~ts': ~tp",
                               [Exchange, Username, VirtualHost, Error]),
            erlang:raise(Class, Error, Stacktrace)
    end.

Thank you!

[bedla]

ahh, cool. thx :)

[bedla]

just a note, is there reason why you did not put exchange name into event data?

[michaelklishin]

@bedla no particular reason

[bedla]

could you add there that info? or I can prepare PR (but not having toolchain ready to test it)