Uploaded image for project: 'Couchbase Gateway'
  1. Couchbase Gateway
  2. CBG-946

Repeated change entries on access grant to doc's previous channel

    XMLWordPrintable

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 2.7.0
    • 2.8.0
    • SyncGateway
    • Security Level: Public
    • None
    • CBG Sprint 55
    • 2

    Description

      N.B. The downstream effects of this are still to be investigated, but the behaviour in and of itself is incorrect so raising this now

      Consider the following scenario:

      • User jflath has access to channel foo.
      • Doc test is created in channels [ foo , bar ].
      • Doc test is updated to remove it from channel bar, leaving just foo.
      • User jflath is granted access to channel bar.
      • User jflath starts a changes feed with no channel filter.

      Expected behaviour

      • jflath receives test in the changes feed with Removed:[bar]

      Observed behaviour

      • jflath receives test in the changes feed without Removed:[bar]
      • jflath also receives test in the changes feed with Removed:[bar]

      Repro

      • Create the user (here done in config):

      "users": {
        "GUEST": {
          "admin_channels": [
            "*"
          ],
          "disabled": true
        },
        "jflath": {
          "admin_channels": ["foo"],
          "password": "pass"
        }
      }
      

      • Add doc in both channels:

      $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo", "bar"]}'
       
      {"id":"test","ok":true,"rev":"1-5467c30804fb9448df4873941c0c8ed8"}
      
      

      • Remove doc from bar:

      $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo"], "_rev":"1-5467c30804fb9448df4873941c0c8ed8"}'
       
      {"id":"test","ok":true,"rev":"2-61b71bf116a8ee0051649282b29d276d"}
      

      • Grant jflath access to bar:

      $ curl localhost:4985/db/_user/jflath -X PUT -d '{"admin_channels": ["foo", "bar"]}'
      

      • First check changes from admin perspective - everything looks normal:

      $ curl localhost:4985/db/_changes
       
      {"results":[
      {"seq":1,"id":"_user/","changes":[{"rev":""}]}
      ,{"seq":4,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
      ,{"seq":5,"id":"_user/jflath","changes":[{"rev":""}]}
      ],
      "last_seq":"5"}
      

      • Get changes as jflath, receive two entries for the same doc:

      $ curl localhost:4984/db/_changes -u jflath:pass
       
      {"results":[
      {"seq":4,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
      ,{"seq":"5:4","id":"test","removed":["bar"],"changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
      ,{"seq":5,"id":"_user/jflath","changes":[]}
      ],
      "last_seq":"5"}
      

      Problem

      The changes response gives us a good clue - the same doc/rev is listed under 4 and 5:4 - we get the 4 from jflath's access to foo and the 5:4 from the access to bar. We do have handling for the case where a doc has been removed from one of the channels being requested but not others, and check here:

      https://github.com/couchbase/sync_gateway/blob/master/db/changes.go#L634-L650

      				for i, cur := range current {
      					if cur != nil && cur.Seq == minSeq {
      						current[i] = nil
      						// Track whether this is a removal from all user's channels
      						if cur.Removed == nil && minEntry.allRemoved == true {
      							minEntry.allRemoved = false
      						}
      						// Also concatenate the matching entries' Removed arrays:
      						if cur != minEntry && cur.Removed != nil {
      							if minEntry.Removed == nil {
      								minEntry.Removed = cur.Removed
      							} else {
      								minEntry.Removed = minEntry.Removed.Union(cur.Removed)
      							}
      						}
      					}
      				}
      

      The trouble is the check of cur.Seq == minSeq doesn't work when comparing 4 and 5:4, so they aren't treated as the same entry. My naive fix would be to tweak the check to compare cur.Seq.Seq == minSeq.Seq (i.e. the "real" seq of the doc only, not factoring in the TriggeredBy or LowSeq), but I don't think this would completely fix things based on how we pull items off the current array. It does work for this test case though...

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            James Flather James Flather added a comment - - edited

            Update: the naive fix doesn't work in the case that there are more changes in bar between the doc being created in [ foo , bar ] and being removed from bar, e.g.:

            $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo", "bar"]}'
            {"id":"test","ok":true,"rev":"1-5467c30804fb9448df4873941c0c8ed8"}
             
            $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}'
            {"id":"1dc6c61b51da941054699b8881fc1a36","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}
             
            $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}'
            {"id":"1ae9cde78c6ad5fcb5a5a30289c41807","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}
             
            $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}'
            {"id":"94d7dcb573ca42a10e45d9858e2ae0e8","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}
             
            $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}'
            {"id":"bf2c016764bf9114a48b3df6c036fa81","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}
             
            $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}'
            {"id":"913d4cd4a52c895eb12f0a5b90320274","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}
             
            $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo"], "_rev":"1-5467c30804fb9448df4873941c0c8ed8"}'
            {"id":"test","ok":true,"rev":"2-61b71bf116a8ee0051649282b29d276d"}
             
            $ curl localhost:4985/db/_user/jflath -X PUT -d '{"admin_channels": ["foo", "bar"]}'
             
            $ curl localhost:4984/db/_changes -u jflath:pass
            {"results":[
            {"seq":9,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
            ,{"seq":"10:4","id":"1dc6c61b51da941054699b8881fc1a36","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:5","id":"1ae9cde78c6ad5fcb5a5a30289c41807","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:6","id":"94d7dcb573ca42a10e45d9858e2ae0e8","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:7","id":"bf2c016764bf9114a48b3df6c036fa81","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:8","id":"913d4cd4a52c895eb12f0a5b90320274","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:9","id":"test","removed":["bar"],"changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
            ,{"seq":10,"id":"_user/jflath","changes":[]}
            ],
            "last_seq":"10"}
             
            $ curl localhost:4984/db/_changes -u jflath:pass
            {"results":[
            {"seq":9,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
            ,{"seq":"10:4","id":"1dc6c61b51da941054699b8881fc1a36","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:5","id":"1ae9cde78c6ad5fcb5a5a30289c41807","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:6","id":"94d7dcb573ca42a10e45d9858e2ae0e8","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:7","id":"bf2c016764bf9114a48b3df6c036fa81","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:8","id":"913d4cd4a52c895eb12f0a5b90320274","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]}
            ,{"seq":"10:9","id":"test","removed":["bar"],"changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]}
            ,{"seq":10,"id":"_user/jflath","changes":[]}
            ],
            "last_seq":"10"}
            
            

            James Flather James Flather added a comment - - edited Update: the naive fix doesn't work in the case that there are more changes in bar between the doc being created in [ foo , bar ] and being removed from bar , e.g.: $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo", "bar"]}' {"id":"test","ok":true,"rev":"1-5467c30804fb9448df4873941c0c8ed8"}   $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}' {"id":"1dc6c61b51da941054699b8881fc1a36","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}   $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}' {"id":"1ae9cde78c6ad5fcb5a5a30289c41807","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}   $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}' {"id":"94d7dcb573ca42a10e45d9858e2ae0e8","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}   $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}' {"id":"bf2c016764bf9114a48b3df6c036fa81","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}   $ curl localhost:4985/db/ -X POST -H 'Content-type: application/json' -d '{"channels":["bar"]}' {"id":"913d4cd4a52c895eb12f0a5b90320274","ok":true,"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}   $ curl localhost:4985/db/test -X PUT -H 'Content-type: application/json' -d '{"channels":["foo"], "_rev":"1-5467c30804fb9448df4873941c0c8ed8"}' {"id":"test","ok":true,"rev":"2-61b71bf116a8ee0051649282b29d276d"}   $ curl localhost:4985/db/_user/jflath -X PUT -d '{"admin_channels": ["foo", "bar"]}'   $ curl localhost:4984/db/_changes -u jflath:pass {"results":[ {"seq":9,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]} ,{"seq":"10:4","id":"1dc6c61b51da941054699b8881fc1a36","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:5","id":"1ae9cde78c6ad5fcb5a5a30289c41807","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:6","id":"94d7dcb573ca42a10e45d9858e2ae0e8","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:7","id":"bf2c016764bf9114a48b3df6c036fa81","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:8","id":"913d4cd4a52c895eb12f0a5b90320274","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:9","id":"test","removed":["bar"],"changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]} ,{"seq":10,"id":"_user/jflath","changes":[]} ], "last_seq":"10"}   $ curl localhost:4984/db/_changes -u jflath:pass {"results":[ {"seq":9,"id":"test","changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]} ,{"seq":"10:4","id":"1dc6c61b51da941054699b8881fc1a36","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:5","id":"1ae9cde78c6ad5fcb5a5a30289c41807","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:6","id":"94d7dcb573ca42a10e45d9858e2ae0e8","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:7","id":"bf2c016764bf9114a48b3df6c036fa81","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:8","id":"913d4cd4a52c895eb12f0a5b90320274","changes":[{"rev":"1-e1c8e07ec76a71e96d115e649b12dd6c"}]} ,{"seq":"10:9","id":"test","removed":["bar"],"changes":[{"rev":"2-61b71bf116a8ee0051649282b29d276d"}]} ,{"seq":10,"id":"_user/jflath","changes":[]} ], "last_seq":"10"}

            A bit extra, but less problematic, is that you can trigger this with a simple [ foo, bar ] -> [ bar ] change:

            $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["foo", "bar"]}'
             
            {"id":"test2","ok":true,"rev":"1-5467c30804fb9448df4873941c0c8ed8"}
             
            $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["bar"], "_rev":"1-5467c30804fb9448df4873941c0c8ed8", "extra": "so extra"}'
             
            {"id":"test2","ok":true,"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}
             
            $ curl 'localhost:4984/db/test2' -u jflath:pass
             
            {"_id":"test2","_rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59","channels":["bar"],"extra":"so extra"}
             
            $ curl 'localhost:4984/db/_changes' -u jflath:pass
             
            {"results":[
            {"seq":3,"id":"_user/jflath","changes":[]}
            ,{"seq":5,"id":"test2","removed":["foo"],"changes":[{"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}]}
            ],
            "last_seq":"5"}
             
            $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["bar"], "_rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59", "extra": "so extra"}'
             
            {"id":"test2","ok":true,"rev":"3-989061051071c99469ac36657e86bdf3"}
             
            $ curl 'localhost:4984/db/_changes' -u jflath:pass
             
            {"results":[
            {"seq":3,"id":"_user/jflath","changes":[]}
            ,{"seq":5,"id":"test2","removed":["foo"],"changes":[{"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}]}
            ,{"seq":6,"id":"test2","changes":[{"rev":"3-989061051071c99469ac36657e86bdf3"}]}
            ],
            "last_seq":"6"}
            

            I'm saying this is less problematic because the ordering should mean that you end up in a consistent end state. That said:

            • there's no guarantee that a consumer will process the feed serially/in order received, so the ordering isn't guaranteed to help
            • this is not the expected behaviour, regardless of the impact
            James Flather James Flather added a comment - A bit extra, but less problematic, is that you can trigger this with a simple [ foo , bar ] -> [ bar ] change: $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["foo", "bar"]}'   {"id":"test2","ok":true,"rev":"1-5467c30804fb9448df4873941c0c8ed8"}   $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["bar"], "_rev":"1-5467c30804fb9448df4873941c0c8ed8", "extra": "so extra"}'   {"id":"test2","ok":true,"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}   $ curl 'localhost:4984/db/test2' -u jflath:pass   {"_id":"test2","_rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59","channels":["bar"],"extra":"so extra"}   $ curl 'localhost:4984/db/_changes' -u jflath:pass   {"results":[ {"seq":3,"id":"_user/jflath","changes":[]} ,{"seq":5,"id":"test2","removed":["foo"],"changes":[{"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}]} ], "last_seq":"5"}   $ curl localhost:4985/db/test2 -X PUT -H 'Content-type: application/json' -d '{"channels":["bar"], "_rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59", "extra": "so extra"}'   {"id":"test2","ok":true,"rev":"3-989061051071c99469ac36657e86bdf3"}   $ curl 'localhost:4984/db/_changes' -u jflath:pass   {"results":[ {"seq":3,"id":"_user/jflath","changes":[]} ,{"seq":5,"id":"test2","removed":["foo"],"changes":[{"rev":"2-c4d10e1effc2e6bef6ac072e69c9ac59"}]} ,{"seq":6,"id":"test2","changes":[{"rev":"3-989061051071c99469ac36657e86bdf3"}]} ], "last_seq":"6"} I'm saying this is less problematic because the ordering should mean that you end up in a consistent end state. That said: there's no guarantee that a consumer will process the feed serially/in order received, so the ordering isn't guaranteed to help this is not the expected behaviour, regardless of the impact
            adamf Adam Fraser added a comment - - edited

            It sounds like the initial underlying issue is just that _removed notifications shouldn't be sent during channel backfill.  Given that the motivation of backfill is to do a first-time send of the full channel to newly granted user,  I can't think of a rationale to include removals in that initial send.  Backfill is generated as a snapshot, so there shouldn't be any chance of a revision for a document being sent in the backfill and then a subsequent removal needing to be sent in the same backfill.

            adamf Adam Fraser added a comment - - edited It sounds like the initial underlying issue is just that _removed notifications shouldn't be sent during channel backfill.  Given that the motivation of backfill is to do a first-time send of the full channel to newly granted user,  I can't think of a rationale to include removals in that initial send.  Backfill is generated as a snapshot, so there shouldn't be any chance of a revision for a document being sent in the backfill and then a subsequent removal needing to be sent in the same backfill.
            adamf Adam Fraser added a comment -

            The second (extra) case is working as intended, I believe.  We're merging multiple channel feeds when serving changes, and since it's a streaming response there's no option for deduplication based on docid for this kind of case. 

            I don't agree that we need to support consumers processing the feed in anything other than the order received (in sequence order), otherwise they have the potential to roll back mutations.  

            I agree it's unexpected, but the implicit contract with 'removed' is 'removed from **the channels indicated', not 'lost all access'.

             

            adamf Adam Fraser added a comment - The second (extra) case is working as intended, I believe.  We're merging multiple channel feeds when serving changes, and since it's a streaming response there's no option for deduplication based on docid for this kind of case.  I don't agree that we need to support consumers processing the feed in anything other than the order received (in sequence order), otherwise they have the potential to roll back mutations.   I agree it's unexpected, but the implicit contract with 'removed' is 'removed from **the channels indicated', not 'lost all access'.  

            Build sync_gateway-2.8.0-319 contains sync_gateway commit d33348f with commit message:
            CBG-946: Repeated change entries on access grant to doc's previous channel (#4714)

            build-team Couchbase Build Team added a comment - Build sync_gateway-2.8.0-319 contains sync_gateway commit d33348f with commit message: CBG-946 : Repeated change entries on access grant to doc's previous channel (#4714)

            People

              sarath.kumarsivan Sarath Kumar Sivan (Inactive)
              James Flather James Flather
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes

                  PagerDuty