Fix for #792

[derekcollison]

Allow deny clauses for subscriptions to still allow wildcard subscriptions but do not deliver the messages themselves.

Signed-off-by: Derek Collison derek@nats.io

• Link to issue, e.g. Resolves #792
• Documentation added (if applicable)
• Tests added
• Branch rebased on top of current master (git pull --rebase origin master)
• [ X] Changes squashed to a single commit (described here)
• Build is green in Travis CI
• You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #792

Changes proposed in this pull request:

/cc @nats-io/core

[coveralls]

Coverage decreased (-0.06%) to 90.933% when pulling 42b5e66 on deny_perms into 0d13c90 on master.

[kozlovic]

Either there are changes required or I did not understand what we are trying to do.

[kozlovic]

Should remove c.mperms == nil. We need to think of reload. On reload, we will call processSubsOnConfigReload() for each client and that calls canSubscribe(). If there was a deny before but not anymore, this would need to be cleared, etc..
But even then, it brings the question: if a client has already a mperms, then any other subscription for that client would not be making this check? Why? Say that you have deny clauses on "foo.bar" and "foo.baz", and for the first time create a subscription on ".bar", then only "foo.bar" would be added to the mperms. But if later I create a subscription on ">" or ".baz", etc.. then I should also have "foo.baz" in mperms.

[derekcollison]

I agree need to handle reload, I will think through that. My first thought is just clear mperms on processSubsOnConfigReload() which will then reprocess all the subs that we have as if they are being recreated.

For not checking, see comment above. Once we get triggered we load all deny filters at once, not one at a time.

[kozlovic]

Yes, clearing prior to call canSubscribe() would repopulate it I guess.

[derekcollison]

Will make that change..

[derekcollison]

Well I notice now that we release the client lock and then we process the subs checking canSubscribe(). Which means that if I clear c.mperms and then release the lock we could allow messages through that should not. Will think some more about how to do this.

[derekcollison]

Safe to hold the lock in your opinion there?

[kozlovic]

What I meant is in processSubsOnConfigReload(), you could do something like:

for _, sub := range subs {
    if checkPerms {
        c.mu.Lock()
        c.mperms = nil
        canSub := c.canSubscribe(sub.subject)
        c.mu.Unlock()
        if !canSub {
            removed = append(removed, sub)
	    c.unsubscribe(acc, sub, true)
        }
    }
    if checkAcc {
        c.mu.Lock()
...

[derekcollison]

Can't do that, need to clear it once before looping through subs.

[kozlovic]

Oh yes, it is per client, while this loop is for all subs for that client.

[derekcollison]

I created another solution that I think works well.

[kozlovic]

LGTM. We could cleanup clientHasMovedToDifferentAccount() if now users and client accounts are never nil, but should work the way it is.