mochi-mqtt / server

The fully compliant, embeddable high-performance Go MQTT v5 server for IoT, smarthome, and pubsub
MIT License
1.29k stars 222 forks source link

OnACLCheck BUG #356

Closed mrhb6006 closed 10 months ago

mrhb6006 commented 10 months ago

image here in the third input at aclCheck function should be "true" (mqtt package - server.go)

Naturally, this parameter is expected to have the correct value ("true") during publish It caused a bug in my program at acl check

thedevop commented 10 months ago

@mrhb6006 , you're correct, the write parameter should be set to true. Would you like to create a PR?

mrhb6006 commented 10 months ago

@mrhb6006 , you're correct, the write parameter should be set to true. Would you like to create a PR?

tnx Done https://github.com/mochi-mqtt/server/pull/357

mrhb6006 commented 10 months ago

how long do you think my pull request will take to merge? i need it in my project and my project doesn't work correctly now it is a bad bug https://github.com/mochi-mqtt/server/pull/357

thedevop commented 10 months ago

@mrhb6006 , I have approved the PR. But @mochi-co is needed for final approve and merge. He is very busy lately, so I'm not able to offer an ETA.

In the meantime, you can vendor this module and make modification locally.

mochi-co commented 10 months ago

I maybe wrong, but I'm not 100% sure this assumption is correct.

publishToClient is used to write out packets to subscribers - in this case the ACL check should be for the subscribed client to read from the topic they are subscribed to (some ACL configurations allow a user to write to a topic but not read from it, such as when issuing sensitive user information that other nodes shouldn't be able to see).

The ACL check for sending messages to a topic is handled in processPublish:

if !cl.Net.Inline && !s.hooks.OnACLCheck(cl, pk.TopicName, true) {

Incidentally there is an additional ACL read check that occurs in processSubscribe to prevent users without read access from subscribing to specific topics.

@mrhb6006 could you describe the exact problem you are seeing? And perhaps provide an example of your ACL configuration? This way we may be able to narrow down the cause.

thedevop commented 10 months ago

@mrhb6006 , my bad. @mochi-co is correct. The ACL check in publishToClient is the subscriber and not the publisher.

mrhb6006 commented 10 months ago

@mochi-co @thedevop image

publishToClient in this case called at subscription loop , therefore the client is subscribed to that topic before and acl check before! why does it check again ACL for subscribe?

in my case, when someone subscribe to a topic after a while, he may not be allowed to subscribe to that particular topic, but my service will continue to publish for him for some time . (free subscription) And after some time, a crown unsubscribes the user

thedevop commented 10 months ago

That's an interesting use case, so in your situation, although the permission can change for the duration of the connection, but you want that permission to be locked in at the time of subscription.

@mochi-co , this seems to be in conflict with issue #286, any thoughts?

mochi-co commented 10 months ago

@thedevop I'm not sure what the best way to accomodate this would be. It would be possible for @mrhb6006 to create a new Auth hook and track the ACL changes in the new hook, but the packaged hook is fairly simple and doesn't account for this sort of persistent checking.

Perhaps the ACL hook could maintain a map of client ids and subscriptions and check against that, to 'lock in' the ACL status.

dgduncan commented 10 months ago

@mochi-co I agree with you. I think that this is fairly unique circumstance and should be accommodated in the custom hook implementation itself and not by the server itself.

mrhb6006 commented 10 months ago

ok , tnx a lot .