Add 'create access token' operation to security events#43226
Conversation
| securityEventStore := NewDBWith(s.logger, s).SecurityEventLogs() | ||
| securityEventStore.LogEvent(ctx, &SecurityEvent{ | ||
| Name: SecurityEventAccessTokenCreated, | ||
| URL: "", // TODO need? - ask in the PR |
There was a problem hiding this comment.
@sourcegraph/repo-management wasn't sure whether we care about capturing the URL here. Y'all originally added this ~1.5 years ago. Does anyone still remember the original intention behind the URL field?
There was a problem hiding this comment.
I'll let @sourcegraph/repo-management give the definitive answer, but I think this URL only needs to be set if there is an associated URL. Look at the references of .URL.
In this case we don't need it, I think, because the URL doesn't give us anything.
There was a problem hiding this comment.
I can't remember to be honest. It looks like it's not populated in that many places in the code and where it is, we just populate the URL path.
I'm happy to remove it if you're not finding it valuable.
|
@filiphaftek could we use you as a reviewer for the tasks captured in https://github.com/sourcegraph/sourcegraph/issues/37264 or should we tag someone else? |
|
@willdollman will follow-up w/ a PR for deletions; |
I can do it on Monday, but if you need it faster, maybe @unknwon could have a look? He knows SG code better than me 🙂 |
| _, _, err = db.AccessTokens().CreateInternal(ctx, subject.ID, []string{"a", "b"}, "n0", creator.ID) | ||
| assertSecurityEventAccessTokenCreatedCount(t, db, 0) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Shouldn't that check come right after CreateInternal?
|
|
||
| assertSecurityEventAccessTokenCreatedCount(t, db, 0) | ||
| tid0, tv0, err := db.AccessTokens().Create(ctx, subject.ID, []string{"a", "b"}, "n0", creator.ID) | ||
| assertSecurityEventAccessTokenCreatedCount(t, db, 1) |
There was a problem hiding this comment.
Same as below. I think you'll need to move that check below.
| securityEventStore := NewDBWith(s.logger, s).SecurityEventLogs() | ||
| securityEventStore.LogEvent(ctx, &SecurityEvent{ | ||
| Name: SecurityEventAccessTokenCreated, | ||
| URL: "", // TODO need? - ask in the PR |
There was a problem hiding this comment.
I'll let @sourcegraph/repo-management give the definitive answer, but I think this URL only needs to be set if there is an associated URL. Look at the references of .URL.
In this case we don't need it, I think, because the URL doesn't give us anything.
| Name: SecurityEventAccessTokenCreated, | ||
| URL: "", // TODO need? - ask in the PR | ||
| UserID: uint32(creatorUserID), | ||
| AnonymousUserID: "", |
There was a problem hiding this comment.
You don't need to explicitly set default values. Suggest to remove it.
Description
Expand security events with a new
AccessTokenCreatedentry and hook it up to the correct place.Test plan