CBG-4046 add attachment and create, update, delete events #6988

torcolvin · 2024-07-19T17:02:40Z

I added tests for REST api and didn't add tests for blip code.

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2613/

…vents

adamcfraser

Overall approach looks good, some questions about attachment name. Might merit unit tests to verify auditing of name is consistent across create/update/delete and via different pathways.

db/blip_handler.go

db/crud.go

…vents

adamcfraser

Generally looks fine. Some comment and code style comments, but can leave those to come back to in future.

db/attachment.go

…vents

db/crud.go

adamcfraser · 2024-07-23T03:49:34Z

db/crud.go

+	if doc.IsDeleted() {
+		base.Audit(ctx, base.AuditIDDocumentDelete, base.AuditFields{
+			base.AuditFieldDocID:      docid,
+			base.AuditFieldDocVersion: newRevID,


Tombstones can have channels - feels like we should be including base.AuditFieldChannels here too.

adamcfraser · 2024-07-23T03:50:52Z

base/audit_events.go

 		},
-
+		optionalFieldGroups: []fieldGroup{
+			fieldGroupRequest, // this will be present everywhere except tests,


I expect this comment should match the one for Delete, since exceptions would be the same?

adamcfraser · 2024-07-23T03:52:42Z

base/audit_events.go

 			fieldGroupKeyspace,
 		},
+		optionalFieldGroups: []fieldGroup{
+			fieldGroupRequest, // this is not present on ISGR or import,


Should this be included on AuditIDDocumentUpdate and AuditIDDocumentCreate (or omitted here)?

adamcfraser · 2024-07-23T04:02:02Z

db/database.go

+		return base.UserLogCtx(ctx, "rosmar_noauth", base.UserDomainBuiltin, nil)
+	}
+	username, _, _ := dbCtx.BucketSpec.Auth.GetCredentials()
+	return base.UserLogCtx(ctx, username, base.UserDomainBuiltin, nil) // FIXME, should I pick up roles?


@bbrks what's your take on this question?

Not needed. Roles are passed for filtering only. In the case of builtin users, people can use the username based filtering if they don't want these audit events.

db/util_testing.go

adamcfraser · 2024-07-23T04:09:52Z

rest/utilities_testing.go

@@ -536,7 +536,8 @@ func (rt *RestTester) GetUserAdminAPI(username string) auth.PrincipalConfig {
 // GetSingleTestDatabaseCollection will return a DatabaseCollection if there is only one. Depending on test environment configuration, it may or may not be the default collection.
 func (rt *RestTester) GetSingleTestDatabaseCollection() (*db.DatabaseCollection, context.Context) {
 	c := db.GetSingleDatabaseCollection(rt.TB(), rt.GetDatabase())
-	return c, c.AddCollectionContext(rt.Context())
+	ctx := base.UserLogCtx(c.AddCollectionContext(rt.Context()), "gotest", base.UserDomainBuiltin, nil)


The UserLogCtx feels like it is getting set arbitrarily in a lot of places. Can you help me understand the general approach we're using to decide whether to set "gotest"? I would have thought a DatabaseCollection shouldn't have a user context (since it's not a DatabaseCollectionWithUser).

This is for code that works directly in a test: collection.Put(ctx, ..) needs a user and this provides a user. I wanted to make it clear that this wasn't a real user. This is for cases like import tests where we directly call functions outside of REST calls.

adamcfraser · 2024-07-23T04:12:05Z

rest/doc_api.go

@@ -644,6 +648,10 @@ func (h *handler) handleGetLocalDoc() error {
 	}
 	value[db.BodyId] = "_local/" + docid
 	h.writeJSON(value)
+	base.Audit(h.ctx(), base.AuditIDDocumentRead, base.AuditFields{
+		base.AuditFieldDocID:      docid,
+		base.AuditFieldDocVersion: value[db.BodyRev], // this value might not be populated if this is a doc that starts with _sync


I agree, I don't think BodyRev is a required field for local docs. Can we just remove this?

adamcfraser · 2024-07-23T04:12:45Z

rest/audit_test.go

+	events := jsonLines(rt.TB(), output)
+	countFound := 0
+	for _, event := range events {
+		// skip events that are not document read events


Suggested change

// skip events that are not document read events

// skip events that are not the target eventID

adamcfraser · 2024-07-23T04:13:42Z

rest/audit_test.go

@@ -974,6 +1120,22 @@ func requireDocumentReadEvents(rt *RestTester, output []byte, docID string, docV
 	require.Equal(rt.TB(), docVersions, docVersionsFound)
 }

+// requireAttachmentEvents validates that create attachment events


Is the idea that this expects that eventID is one of the attachment crud event IDs?

…vents

-run

bbrks · 2024-07-23T16:24:08Z

rest/bulk_api.go

+			if atts, ok := body[db.BodyAttachments]; ok && atts != nil {
+				attsMap, ok := atts.(db.AttachmentsMeta)
+				if !ok {
+					base.WarnfCtx(h.ctx(), "Unexpected format of attachments in the body %+v", atts)
+				}
+				for attachment := range attsMap {
+					base.Audit(h.ctx(), base.AuditIDAttachmentRead, base.AuditFields{
+						base.AuditFieldDocID:        docid,
+						base.AuditFieldDocVersion:   revid,
+						base.AuditFieldAttachmentID: attachment,
+					})
+				}
+			}


This is only reading metadata of the attachment, unless includeAttachments is set.

Does this need to be conditional?

adamcfraser · 2024-07-23T16:25:07Z

db/raw_doc_with_inline_sync.json

@@ -0,0 +1,14 @@
+{


I think I'd rather be more deliberate about switching to the use of embed and standalone json files for testing, instead of just adding this single standalone file - would like to think about organization, packaging, etc. Any reason this can't be inline with the test file, like we do in other spots?

adamcfraser · 2024-07-23T16:30:10Z

db/crud.go

@@ -2355,7 +2399,7 @@ func (db *DatabaseCollectionWithUser) DeleteDoc(ctx context.Context, docid strin
 }

 // Purges a document from the bucket (no tombstone)
-func (db *DatabaseCollectionWithUser) Purge(ctx context.Context, key string) error {
+func (db *DatabaseCollectionWithUser) Purge(ctx context.Context, key string, needsAudit bool) error {


I'm not seeing any cases that call this with needsAudit=false. Are there such cases?

The single case is

sync_gateway/db/database.go

Line 1497 in 1fff8bb

purgeErr := collection.Purge(ctx, tombstonesRow.Id)

torcolvin added 4 commits July 18, 2024 15:06

CBG-4046 add document read event

34d46f7

Address PR comments

2fddace

add attachment events

600d052

CBG-4046 implement attachment audit events

d037861

torcolvin force-pushed the CBG-4046-attachment-events branch from a066f72 to d037861 Compare July 19, 2024 17:14

Base automatically changed from CBG-4046-read-events to main July 19, 2024 19:38

Merge remote-tracking branch 'origin/main' into CBG-4046-attachment-e…

3d8eb54

…vents

adamcfraser requested changes Jul 19, 2024

View reviewed changes

db/blip_handler.go Outdated Show resolved Hide resolved

db/crud.go Outdated Show resolved Hide resolved

db/crud.go Show resolved Hide resolved

adamcfraser assigned torcolvin Jul 19, 2024

torcolvin added 3 commits July 22, 2024 09:01

make the attachment storage slightly more efficient

afe1449

rearrange attachment test code for readabilty

235d38b

Merge remote-tracking branch 'origin/main' into CBG-4046-attachment-e…

14476c7

…vents

torcolvin assigned adamcfraser and unassigned torcolvin Jul 22, 2024

torcolvin requested a review from adamcfraser July 22, 2024 19:01

adamcfraser previously approved these changes Jul 22, 2024

View reviewed changes

db/attachment.go Outdated Show resolved Hide resolved

db/attachment.go Outdated Show resolved Hide resolved

db/attachment.go Outdated Show resolved Hide resolved

torcolvin added 2 commits July 22, 2024 17:03

Merge remote-tracking branch 'origin/main' into CBG-4046-attachment-e…

6cac754

…vents

Support multiple attachments with the same name

18d8ba7

torcolvin dismissed adamcfraser’s stale review via 18d8ba7 July 22, 2024 21:16

adamcfraser assigned torcolvin and unassigned adamcfraser Jul 23, 2024

torcolvin added 7 commits July 22, 2024 21:12

CBG-4046 Add CUD audit events

913b850

Add contexts

da4d699

Make sure context is set before StartOnlineProcesses

dc5652f

update required fields

e8a2b48

update events

a1b8f46

Merge branch 'CBG-4046-cud' into CBG-4046-attachment-events

b68e070

move request to optional

0022a18

torcolvin changed the title ~~CBG-4046 add attachment events~~ CBG-4046 add attachment and create, update, delete events Jul 23, 2024

adamcfraser requested changes Jul 23, 2024

View reviewed changes

bbrks assigned bbrks and unassigned torcolvin Jul 23, 2024

torcolvin added 4 commits July 23, 2024 08:16

Merge remote-tracking branch 'origin/main' into CBG-4046-attachment-e…

fbce2ca

…vents

PR comments

a363695

Remove local events done in a separate PR

21a4032

Add purged events

d35e038

torcolvin mentioned this pull request Jul 23, 2024

CBG-4046 Add CUD audit events #6998

Closed

6 tasks

bbrks reviewed Jul 23, 2024

View reviewed changes

-run Outdated Show resolved Hide resolved

bbrks assigned torcolvin and unassigned bbrks Jul 23, 2024

torcolvin added 4 commits July 23, 2024 10:54

Correct channels format for audit logging

eed5cf1

add missing file

116fa9f

Remove channels from logging

f463975

remove unncessary files

fd19b07

bbrks reviewed Jul 23, 2024

View reviewed changes

adamcfraser requested changes Jul 23, 2024

View reviewed changes

remove goembed, use attachment read conditionally in bulk docs

6a647e0

torcolvin assigned adamcfraser and unassigned torcolvin Jul 23, 2024

adamcfraser previously approved these changes Jul 23, 2024

View reviewed changes

Fix attachment names

2394e93

torcolvin dismissed adamcfraser’s stale review via 2394e93 July 23, 2024 17:32

bbrks approved these changes Jul 23, 2024

View reviewed changes

adamcfraser merged commit 0f05fda into main Jul 23, 2024
38 checks passed

adamcfraser deleted the CBG-4046-attachment-events branch July 23, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-4046 add attachment and create, update, delete events #6988

CBG-4046 add attachment and create, update, delete events #6988

torcolvin commented Jul 19, 2024 •

edited

Loading

adamcfraser left a comment

adamcfraser left a comment

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

bbrks Jul 23, 2024

adamcfraser Jul 23, 2024

torcolvin Jul 23, 2024

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

bbrks Jul 23, 2024

adamcfraser Jul 23, 2024

adamcfraser Jul 23, 2024

torcolvin Jul 23, 2024

	// skip events that are not document read events
	// skip events that are not the target eventID

CBG-4046 add attachment and create, update, delete events #6988

CBG-4046 add attachment and create, update, delete events #6988

Conversation

torcolvin commented Jul 19, 2024 • edited Loading

Pre-review checklist

adamcfraser left a comment

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torcolvin commented Jul 19, 2024 •

edited

Loading