CouchDB
  1. CouchDB
  2. COUCHDB-1391

Implement _security as _local doc with revision trees

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Database Core
    • Labels:
      None
    • Skill Level:
      Dont Know

      Description

      We had a discussion [1] a while back about updating the _security object so that it could be replicated (internally) in a cluster or similar environment. The basic gist was "update _local docs to have a revision tree, update _security to be a _local doc with docid "_local/_security" and keep the current _security API for a version or two for backwards compatibility (or forever, what color is your bike shed?)"

      So I did that.

      Basic patch progression is:

      1. Refactor revision merging logic so that we can split it out of couch_db_updater's code path for updating normal docs.
      2. Implement _local docs with #full_doc_info{} records (and thus revision trees)
      3. Implement _security as _local/_security

      These things are done. Tests should theoretically pass after each patch but I haven't gone back and tried. They definitely pass (minus auth_cache which I just submitted a fix for) now except for replication.js appears to fail for random reasons. I can't quite decide if I've introduced this or if it just fails randomly. Rather than run it a lot more times and continue to be confused I'm starting this ticket so I can have other people test and tell me their results.

      Also, the test suite is rather wonky on trunk with segfaults. We should really look into that more.

      Patches forth coming. I've also pushed the branch to [2].

      [1] http://grokbase.com/t/couchdb.apache.org/dev/2011/08/the-security-object-should-be-versioned/17rfmmtlu3lagqvgyq7cay26dqk4
      [2] http://git-wip-us.apache.org/repos/asf?p=couchdb.git;a=log;h=refs/heads/new-security-object

        Activity

        Hide
        Paul Joseph Davis added a comment -

        Roast of patch with a cream sauce and a side of patch.

        Show
        Paul Joseph Davis added a comment - Roast of patch with a cream sauce and a side of patch.
        Hide
        Paul Joseph Davis added a comment -

        Just realized the subject of the third patch is silly. Updated on the Git branch. Too lazy to re upload here for a commit message change.

        Show
        Paul Joseph Davis added a comment - Just realized the subject of the third patch is silly. Updated on the Git branch. Too lazy to re upload here for a commit message change.
        Hide
        Randall Leeds added a comment -

        Can you explain couch_db.erl:705? Why did the list brackets appear around the doc? I'm guessing that's the format new_revs wants.

        For couch_db:open_doc_int/3 I would remove the first function clause entirely and then add a clause to get_doc_infos/2. That cleanly splits the logic at the choice of btree and introduces less redundant code for us to mess up down the line.

        In couch_db_updater:init_db/6 the line "Db1#db

        {security_ptr=local}

        ;" is redundant in the clause where security_ptr is already the atom local.
        Am I right that at these lines update a db as soon as it's opened? I was wondering why I didn't see more backwards-compatibility code anywhere. Just want to know I've got it right.

        couch_db_updater:init_security/2 needs to be fixed to keep the user context intact, I think. Right now it sets the user context to _admin so it can update the security document but then returns that modified #db record back to init_db without dropping those privileges. Unless I misunderstand, that seems wrong.

        Any reason why copy_docs can't be changed to take a btree argument? It looks mostly the same as copy_local_docs.

        What's the change in couch_replicator? The second clause doesn't make total sense to me. Is that to reset the leading number for the rev when the log is migrated to a new ID?

        Otherwise, nicely done! Looks great! Thanks.

        Show
        Randall Leeds added a comment - Can you explain couch_db.erl:705? Why did the list brackets appear around the doc? I'm guessing that's the format new_revs wants. For couch_db:open_doc_int/3 I would remove the first function clause entirely and then add a clause to get_doc_infos/2. That cleanly splits the logic at the choice of btree and introduces less redundant code for us to mess up down the line. In couch_db_updater:init_db/6 the line "Db1#db {security_ptr=local} ;" is redundant in the clause where security_ptr is already the atom local. Am I right that at these lines update a db as soon as it's opened? I was wondering why I didn't see more backwards-compatibility code anywhere. Just want to know I've got it right. couch_db_updater:init_security/2 needs to be fixed to keep the user context intact, I think. Right now it sets the user context to _admin so it can update the security document but then returns that modified #db record back to init_db without dropping those privileges. Unless I misunderstand, that seems wrong. Any reason why copy_docs can't be changed to take a btree argument? It looks mostly the same as copy_local_docs. What's the change in couch_replicator? The second clause doesn't make total sense to me. Is that to reset the leading number for the rev when the log is migrated to a new ID? Otherwise, nicely done! Looks great! Thanks.
        Hide
        Paul Joseph Davis added a comment -

        > Can you explain couch_db.erl:705? Why did the list brackets appear around the doc? I'm guessing that's the format new_revs wants.

        Lots of code I reused wants to have lists of docs with the same _id. Instead of rewriting everything else I just used the same structure.

        > add a clause to get_doc_infos/2.

        Do you mean get_full_doc_infos? If so, that seems like it should work quite nicely. I'll take a crack at that this afternoon.

        > the line "Db1#db

        {security_ptr=local}

        ;" is redundant

        Good catch. Will fix.

        > Am I right that at these lines update a db as soon as it's opened?

        Yep.

        > I was wondering why I didn't see more backwards-compatibility code anywhere.

        That's mostly all there is. I realized last night that I have to wrap the local docs btree split fun to upgrade docs on the fly, but that should be simple enough.

        > couch_db_updater:init_security/2 needs to be fixed

        Good catch. That should be passing Db0 to update_local_docs/2.

        > Any reason why copy_docs can't be changed to take a btree argument?

        Yes, copy_docs is working with the by_seq btree which doesn't work for local docs. I contemplated refactoring here, but for now I'd just erred on the side of caution. After having spelunked through this code I've decided that a real fix is going to require me groking couch_db.erl and couch_db_updater.erl and doing similar level refactor to what I did for the view engine.

        > What's the change in couch_replicator?

        This was me not understanding the replicator / thinking very hard. I'm going to rewrite the split fun for the local docs btree to automatically upgrade old _local docs records on the fly which should make this code unnecessary.

        Thanks for the review. I'll get a revised version up tonight or tomorrow depending on time.

        Show
        Paul Joseph Davis added a comment - > Can you explain couch_db.erl:705? Why did the list brackets appear around the doc? I'm guessing that's the format new_revs wants. Lots of code I reused wants to have lists of docs with the same _id. Instead of rewriting everything else I just used the same structure. > add a clause to get_doc_infos/2. Do you mean get_full_doc_infos? If so, that seems like it should work quite nicely. I'll take a crack at that this afternoon. > the line "Db1#db {security_ptr=local} ;" is redundant Good catch. Will fix. > Am I right that at these lines update a db as soon as it's opened? Yep. > I was wondering why I didn't see more backwards-compatibility code anywhere. That's mostly all there is. I realized last night that I have to wrap the local docs btree split fun to upgrade docs on the fly, but that should be simple enough. > couch_db_updater:init_security/2 needs to be fixed Good catch. That should be passing Db0 to update_local_docs/2. > Any reason why copy_docs can't be changed to take a btree argument? Yes, copy_docs is working with the by_seq btree which doesn't work for local docs. I contemplated refactoring here, but for now I'd just erred on the side of caution. After having spelunked through this code I've decided that a real fix is going to require me groking couch_db.erl and couch_db_updater.erl and doing similar level refactor to what I did for the view engine. > What's the change in couch_replicator? This was me not understanding the replicator / thinking very hard. I'm going to rewrite the split fun for the local docs btree to automatically upgrade old _local docs records on the fly which should make this code unnecessary. Thanks for the review. I'll get a revised version up tonight or tomorrow depending on time.
        Hide
        Randall Leeds added a comment -

        Cool. Good plans. You're right not to go crazy with refactoring yet.

        I think update_local_docs needs the privileged #db handle, you should just take the local_docs_btree from the #db it returns and put that into the original handle passed into init_security.

        Show
        Randall Leeds added a comment - Cool. Good plans. You're right not to go crazy with refactoring yet. I think update_local_docs needs the privileged #db handle, you should just take the local_docs_btree from the #db it returns and put that into the original handle passed into init_security.
        Hide
        Paul Joseph Davis added a comment -

        Yeah, I looked harder at init_security and realized what I needed was to just return the db but replace the security context like you had originally. I cribbed part of that function from a different place and then didn't think hard enough. Should be fixed in a commit I haven't pushed quiet yet.

        Show
        Paul Joseph Davis added a comment - Yeah, I looked harder at init_security and realized what I needed was to just return the db but replace the security context like you had originally. I cribbed part of that function from a different place and then didn't think hard enough. Should be fixed in a commit I haven't pushed quiet yet.

          People

          • Assignee:
            Unassigned
            Reporter:
            Paul Joseph Davis
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development