go-kivik / kivik

Common interface to CouchDB or CouchDB-like databases for Go and GopherJS
Other
316 stars 44 forks source link

Support Cloudant security document. #716

Closed darksnow closed 9 months ago

darksnow commented 1 year ago

I've been using this library to develop an app against a local install of CouchDB and it's been working perfectly.

I'm not looking to deploy the app to Cloudant and again, this library has been working great, until I tried to access a _security document for a database.

It seems that couch, and therefore this library, expect to see admin and members lists in the _security document but Cloudant doesn't populate or use those keys.

Cloundant appears to use a cloudant key with a map of user name, from the _users database in my case, and an array of roles.

In this library those keys are skipped by the JSON parser so I end up with nothing at all.

I think the fix to this is to add the following to Security type:

Cloudant map[string][]string `json:"cloudant,omitempty"`

So, we'd end up with:

// Security represents a database security document.
type Security struct {
    Admins  Members `json:"admins,omitempty"`
    Members Members `json:"members,omitempty"`
    Cloudant map[string][]string `json:"cloudant,omitempty"`
    CouchdbAuthOnly *bool `json:"couchdb_auth_only,omitempty"`
}

This small change, combined with change to func (*d DB) Security and func (db *DB) SetSecurity would at least expose the Cloudant _security document to this library, allowing users to decide what to do based on which fields are populated.

Would this be of interest to anyone? Am I missing anything? Does this sound as easy as it appears to me? Is there another way to access the _security document without these changes?

Thanks.

flimzy commented 1 year ago

Can you include a complete JSON response from Cloudant for the _security endpoint for comparison?

darksnow commented 1 year ago

I've added a new user via the cloudant admin UI and checked all available permissions boxes. This is what I'm getting back from the HTTPS API.

{
    "cloudant": {
        "nobody": [],
        "random_user_name": [
            "_admin",
            "_reader",
            "_replicator",
            "_writer"
        ]
    }
}

Posting this JSON structure back, with alterations, does change the permissions in Cloudant. The generic map type for the Cloudant key, along with the CouchdbAuthOnly key, are taken from the official Go library from IBM https://github.com/IBM/cloudant-go-sdk/blob/main/cloudantv1/cloudant_v1.go#L17445

flimzy commented 1 year ago

Thanks for the info. This looks like it should be pretty straight forward to add. Are you using v3 or v4 of the library?

darksnow commented 1 year ago

Thanks Flimsy. I'm on v4.

flimzy commented 1 year ago

Great. I've merged a simple patch, that I hope will be sufficient. Would you test it, and let me know if it's enough?

flimzy commented 1 year ago

(And my appologies for the recent hectic changes to v4... I'm preparing to make a stable release in the next week or so, so getting a batch of breaking changes in all at once)

darksnow commented 1 year ago

Thanks very much for the great work and very quick response. I was not set up to test this and produce the patch myself so I've been learning a lot about setting up GO environments to allow me to fully test this code.

The small change you made worked as expected but the new values didn't make it all the way back through the code to my test code. I think this small change will fix it:

diff --git a/db.go b/db.go
index 92d3f4a..a359a9d 100644
--- a/db.go
+++ b/db.go
@@ -516,9 +516,12 @@ func (db *DB) Security(ctx context.Context) (*Security, error) {
    if err != nil {
        return nil, err
    }
+
    return &Security{
-       Admins:  Members(s.Admins),
-       Members: Members(s.Members),
+       Admins:          Members(s.Admins),
+       Members:         Members(s.Members),
+       Cloudant:        s.Cloudant,
+       CouchdbAuthOnly: s.CouchdbAuthOnly,
    }, err
 }

I thought I'd include the patch here rather than forking the code to do a small PR. I hope this is all OK.

Thanks again, and is there an ETA for a new full release?

flimzy commented 1 year ago

Oh, good call. I'll have that merged today.

I'll probably be tagging an rc later this week, perhaps next week.

darksnow commented 1 year ago

I spoke too soon.

I'm my initial testing I was just PUTing the updated security document back into the _security document directly. When I reverted some code I'd used for testing and tried to use SetSecurity on the kivik.DB, it failed. I tracked this down to the marshalling in the driver. Though, if PUT _security works with the document as is, it does raise a question about the need for the temporary security document, why not just marshal the Security struct directly?

Again a small change so I'll include it here. I've done more extensive testing and this appears to work but there's a caveat.

Sometimes, both Couch and Cloudant can return a valid empty response, so there's no guaranteed way to determine which to use. I've got round this my setting an envvar in my app. I'm guessing this library is trying to keep a light touch s it's not a problem in here, I think.

Anyway, here's the change. If there's anything more complicated I'll likely do a proper fork of the code.


diff --git a/driver/driver.go b/driver/driver.go
index bb5633c..ed9240e 100644
--- a/driver/driver.go
+++ b/driver/driver.go
@@ -158,8 +158,10 @@ type Security struct {
 // MarshalJSON satisfies the json.Marshaler interface.
 func (s Security) MarshalJSON() ([]byte, error) {
        var v struct {
-               Admins  *Members `json:"admins,omitempty"`
-               Members *Members `json:"members,omitempty"`
+               Admins          *Members            `json:"admins,omitempty"`
+               Members         *Members            `json:"members,omitempty"`
+               Cloudant        map[string][]string `json:"cloudant,omitempty"`
+               CouchdbAuthOnly *bool               `json:"couchdb_auth_only,omitempty"`
        }
        if len(s.Admins.Names) > 0 || len(s.Admins.Roles) > 0 {
                v.Admins = &s.Admins
@@ -167,6 +169,12 @@ func (s Security) MarshalJSON() ([]byte, error) {
        if len(s.Members.Names) > 0 || len(s.Members.Roles) > 0 {
                v.Members = &s.Members
        }
+       if len(s.Cloudant) > 0 {
+               v.Cloudant = s.Cloudant
+       }
+       if s.CouchdbAuthOnly != nil {
+               v.CouchdbAuthOnly = s.CouchdbAuthOnly
+       }
        return json.Marshal(v)
 }
darksnow commented 1 year ago

@flimzy just checking that you got that last message from me. I think it'll need another small change. If you'd like me to produce a PR for this I can.

I've been using this change for a while on my project and it's working as expected.

flimzy commented 1 year ago

@darksnow Thanks for the ping. I'll merge the change you proposed.

darksnow commented 10 months ago

I am so sorry to open this ticket again @flimzy.

I've recently changed my app to use the new release version of Kivik instead of my modified local version and Cloudant security does not work as expected.

Reviewing the changes I've suggested above it seems I missed something. I've tested it again, and below is the only change that I missed.

index fd700c1..3169d4f 100644
--- a/db.go
+++ b/db.go
@@ -622,8 +622,10 @@ func (db *DB) SetSecurity(ctx context.Context, security *Security) error {
        }
        defer endQuery()
        sec := &driver.Security{
-               Admins:  driver.Members(security.Admins),
-               Members: driver.Members(security.Members),
+               Admins:          driver.Members(security.Admins),
+               Members:         driver.Members(security.Members),
+               Cloudant:        security.Cloudant,
+               CouchdbAuthOnly: security.CouchdbAuthOnly,
        }
        return secDB.SetSecurity(ctx, sec)
 }

Again, I'm so, so sorry to post changes like this and generally be a pain. Next time I have a bright idea I will fork the code and submit changes properly.

flimzy commented 9 months ago

Thanks, I've applied that small patch and tagged 4.2.1. I hope that's it :wink: