feat(authN): Add to all DB entries 'Modified_by'... (#81) #230
Conversation
Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct Signed-off-by: Michal Krzyz <michalkrzyz@gmail.com>
michalkrzyz
force-pushed
the
mikrzyz/issue-81
branch
from
e084f8b
to
cb2f9c1
Compare
michalkrzyz
force-pushed
the
mikrzyz/issue-81
branch
5 times, most recently
from
b4e30a8
to
024bc5b
Compare
Add metadata for user Add tests
michalkrzyz
force-pushed
the
mikrzyz/issue-81
branch
2 times, most recently
from
94c222f
to
2cdfe8c
Compare
internal/e2e/metadata_test.go
Outdated
| Expect(err).Should(BeNil()) | ||
| Expect(createdAt).Should(BeTemporally("~", time.Now().UTC(), 3*time.Second)) | ||
|
|
||
| Expect(*user.Metadata.UpdatedBy).To(BeEmpty()) |
There was a problem hiding this comment.
Do we really check for BeEmpty()? in the UpdatedBy field?
There was a problem hiding this comment.
I wanted to highlight that UpdatedBy is not assigned when the object is created. In some solutions of such problems we see that modified/updated time is assigned by created time at creation of an object (eg. Linux File)
Add metadata for activity, IssueVariant and IssueRepository
Add metadata for component
Add metadata for componentInstance, componentVersion
michalkrzyz
force-pushed
the
mikrzyz/issue-81
branch
from
2cdfe8c
to
b8d5d0a
Compare
Add metadata for evidence
Issue Metadata rename to IssueMetadata, because now metadata means data related to creation time, creation user, update time and update user Add Metadata for Issue
Add metadata for IssueMatch
Add Metadata for IssueMatchChange, Service and SupportGroup
| @@ -132,6 +132,7 @@ func (a *activityHandler) ListActivities(filter *entity.ActivityFilter, options | |||
| } | |||
|
|
|||
| func (a *activityHandler) CreateActivity(activity *entity.Activity) (*entity.Activity, error) { | |||
| activity.CreatedBy = "Creator" | |||
There was a problem hiding this comment.
I guess that's a placeholder. Based on the comment I made we need to decide if that should really be a freetext field or rather a foreign key reference to a User
| CreatedAt: &createdAt, | ||
| CreatedBy: &em.CreatedBy, |
There was a problem hiding this comment.
As commented this might need to be a sub-resolver for users?!
There was a problem hiding this comment.
Most queries don't include the createdBy and updatedBy fields. Do we want to add them?
| @@ -10,8 +10,7 @@ type IssueVariant implements Node { | |||
| issueRepository: IssueRepository | |||
| issueId: String | |||
| issue: Issue | |||
| created_at: DateTime | |||
| updated_at: DateTime | |||
| metadata: Metadata | |||
| @@ -105,6 +105,7 @@ func (cs *componentHandler) ListComponents(filter *entity.ComponentFilter, optio | |||
| } | |||
|
|
|||
| func (cs *componentHandler) CreateComponent(component *entity.Component) (*entity.Component, error) { | |||
| component.CreatedBy = "Creator" | |||
| @@ -139,6 +140,7 @@ func (cs *componentHandler) CreateComponent(component *entity.Component) (*entit | |||
| } | |||
|
|
|||
| func (cs *componentHandler) UpdateComponent(component *entity.Component) (*entity.Component, error) { | |||
| component.UpdatedBy = "Updater" | |||
| @@ -111,6 +111,7 @@ func (ci *componentInstanceHandler) ListComponentInstances(filter *entity.Compon | |||
| } | |||
|
|
|||
| func (ci *componentInstanceHandler) CreateComponentInstance(componentInstance *entity.ComponentInstance) (*entity.ComponentInstance, error) { | |||
| componentInstance.CreatedBy = "Creator" | |||
| @@ -131,6 +132,7 @@ func (ci *componentInstanceHandler) CreateComponentInstance(componentInstance *e | |||
| } | |||
|
|
|||
| func (ci *componentInstanceHandler) UpdateComponentInstance(componentInstance *entity.ComponentInstance) (*entity.ComponentInstance, error) { | |||
| componentInstance.UpdatedBy = "Updater" | |||
| @@ -109,6 +109,7 @@ func (cv *componentVersionHandler) ListComponentVersions(filter *entity.Componen | |||
| } | |||
|
|
|||
| func (cv *componentVersionHandler) CreateComponentVersion(componentVersion *entity.ComponentVersion) (*entity.ComponentVersion, error) { | |||
| componentVersion.CreatedBy = "Creator" | |||
| @@ -115,6 +115,13 @@ func (cs *componentHandler) CreateComponent(component *entity.Component) (*entit | |||
| "filter": f, | |||
| }) | |||
|
|
|||
| var err error | |||
| component.CreatedBy, err = common.GetUserId(cs.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -144,7 +151,14 @@ func (cs *componentHandler) UpdateComponent(component *entity.Component) (*entit | |||
| "object": component, | |||
| }) | |||
|
|
|||
| err := cs.database.UpdateComponent(component) | |||
| var err error | |||
| component.UpdatedBy, err = common.GetUserId(cs.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -116,6 +116,13 @@ func (ci *componentInstanceHandler) CreateComponentInstance(componentInstance *e | |||
| "object": componentInstance, | |||
| }) | |||
|
|
|||
| var err error | |||
| componentInstance.CreatedBy, err = common.GetUserId(ci.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -136,7 +143,14 @@ func (ci *componentInstanceHandler) UpdateComponentInstance(componentInstance *e | |||
| "object": componentInstance, | |||
| }) | |||
|
|
|||
| err := ci.database.UpdateComponentInstance(componentInstance) | |||
| var err error | |||
| componentInstance.UpdatedBy, err = common.GetUserId(ci.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -114,6 +114,13 @@ func (cv *componentVersionHandler) CreateComponentVersion(componentVersion *enti | |||
| "object": componentVersion, | |||
| }) | |||
|
|
|||
| var err error | |||
| componentVersion.CreatedBy, err = common.GetUserId(cv.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -134,7 +141,14 @@ func (cv *componentVersionHandler) UpdateComponentVersion(componentVersion *enti | |||
| "object": componentVersion, | |||
| }) | |||
|
|
|||
| err := cv.database.UpdateComponentVersion(componentVersion) | |||
| var err error | |||
| componentVersion.UpdatedBy, err = common.GetUserId(cv.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -109,6 +109,13 @@ func (e *evidenceHandler) CreateEvidence(evidence *entity.Evidence) (*entity.Evi | |||
| "object": evidence, | |||
| }) | |||
|
|
|||
| var err error | |||
| evidence.CreatedBy, err = common.GetUserId(e.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -127,7 +134,14 @@ func (e *evidenceHandler) UpdateEvidence(evidence *entity.Evidence) (*entity.Evi | |||
| "object": evidence, | |||
| }) | |||
|
|
|||
| err := e.database.UpdateEvidence(evidence) | |||
| var err error | |||
| evidence.UpdatedBy, err = common.GetUserId(e.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -170,6 +170,13 @@ func (is *issueHandler) CreateIssue(issue *entity.Issue) (*entity.Issue, error) | |||
| "filter": f, | |||
| }) | |||
|
|
|||
| var err error | |||
| issue.CreatedBy, err = common.GetUserId(is.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -198,7 +205,14 @@ func (is *issueHandler) UpdateIssue(issue *entity.Issue) (*entity.Issue, error) | |||
| "object": issue, | |||
| }) | |||
|
|
|||
| err := is.database.UpdateIssue(issue) | |||
| var err error | |||
| issue.UpdatedBy, err = common.GetUserId(is.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -141,6 +141,13 @@ func (im *issueMatchHandler) CreateIssueMatch(issueMatch *entity.IssueMatch) (*e | |||
| "object": issueMatch, | |||
| }) | |||
|
|
|||
| var err error | |||
| issueMatch.CreatedBy, err = common.GetUserId(im.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -175,7 +182,14 @@ func (im *issueMatchHandler) UpdateIssueMatch(issueMatch *entity.IssueMatch) (*e | |||
| "object": issueMatch, | |||
| }) | |||
|
|
|||
| err := im.database.UpdateIssueMatch(issueMatch) | |||
| var err error | |||
| issueMatch.UpdatedBy, err = common.GetUserId(im.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -113,6 +113,13 @@ func (imc *issueMatchChangeHandler) CreateIssueMatchChange(issueMatchChange *ent | |||
| "object": issueMatchChange, | |||
| }) | |||
|
|
|||
| var err error | |||
| issueMatchChange.CreatedBy, err = common.GetUserId(imc.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -133,7 +140,14 @@ func (imc *issueMatchChangeHandler) UpdateIssueMatchChange(issueMatchChange *ent | |||
| "object": issueMatchChange, | |||
| }) | |||
|
|
|||
| err := imc.database.UpdateIssueMatchChange(issueMatchChange) | |||
| var err error | |||
| issueMatchChange.UpdatedBy, err = common.GetUserId(imc.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -112,6 +112,13 @@ func (ir *issueRepositoryHandler) CreateIssueRepository(issueRepository *entity. | |||
| "filter": f, | |||
| }) | |||
|
|
|||
| var err error | |||
| issueRepository.BaseIssueRepository.CreatedBy, err = common.GetUserId(ir.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -142,7 +149,14 @@ func (ir *issueRepositoryHandler) UpdateIssueRepository(issueRepository *entity. | |||
| "object": issueRepository, | |||
| }) | |||
|
|
|||
| err := ir.database.UpdateIssueRepository(issueRepository) | |||
| var err error | |||
| issueRepository.BaseIssueRepository.UpdatedBy, err = common.GetUserId(ir.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -178,6 +178,13 @@ func (iv *issueVariantHandler) CreateIssueVariant(issueVariant *entity.IssueVari | |||
| "filter": f, | |||
| }) | |||
|
|
|||
| var err error | |||
| issueVariant.CreatedBy, err = common.GetUserId(iv.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -208,7 +215,14 @@ func (iv *issueVariantHandler) UpdateIssueVariant(issueVariant *entity.IssueVari | |||
| "object": issueVariant, | |||
| }) | |||
|
|
|||
| err := iv.database.UpdateIssueVariant(issueVariant) | |||
| var err error | |||
| issueVariant.UpdatedBy, err = common.GetUserId(iv.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -135,6 +135,13 @@ func (s *serviceHandler) CreateService(service *entity.Service) (*entity.Service | |||
| "filter": f, | |||
| }) | |||
|
|
|||
| var err error | |||
| service.BaseService.CreatedBy, err = common.GetUserId(s.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -164,7 +171,14 @@ func (s *serviceHandler) UpdateService(service *entity.Service) (*entity.Service | |||
| "object": service, | |||
| }) | |||
|
|
|||
| err := s.database.UpdateService(service) | |||
| var err error | |||
| service.BaseService.UpdatedBy, err = common.GetUserId(s.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -142,6 +142,13 @@ func (sg *supportGroupHandler) CreateSupportGroup(supportGroup *entity.SupportGr | |||
| Name: []*string{&supportGroup.Name}, | |||
| } | |||
|
|
|||
| var err error | |||
| supportGroup.CreatedBy, err = common.GetUserId(sg.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -173,7 +180,14 @@ func (sg *supportGroupHandler) UpdateSupportGroup(supportGroup *entity.SupportGr | |||
| "object": supportGroup, | |||
| }) | |||
|
|
|||
| err := sg.database.UpdateSupportGroup(supportGroup) | |||
| var err error | |||
| supportGroup.UpdatedBy, err = common.GetUserId(sg.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -112,6 +112,13 @@ func (u *userHandler) CreateUser(user *entity.User) (*entity.User, error) { | |||
| "object": user, | |||
| }) | |||
|
|
|||
| var err error | |||
| user.CreatedBy, err = common.GetUserId(u.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| @@ -141,7 +148,14 @@ func (u *userHandler) UpdateUser(user *entity.User) (*entity.User, error) { | |||
| "object": user, | |||
| }) | |||
|
|
|||
| err := u.database.UpdateUser(user) | |||
| var err error | |||
| user.UpdatedBy, err = common.GetUserId(u.database, "S0000000") | |||
There was a problem hiding this comment.
See my comment for the activity handler
| } | ||
|
|
||
| func QueryCreateUser(port string, user User) *model.User { | ||
| client := graphql.NewClient(fmt.Sprintf("http://localhost:%s/query", port)) |
There was a problem hiding this comment.
Should we use these helper functions in the other tests too?
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| type Issue struct { |
There was a problem hiding this comment.
Why do we need to redefine issue here?
| @@ -178,8 +194,17 @@ func NewSeverity(url string) Severity { | |||
| } | |||
|
|
|||
| type Cvss struct { | |||
| Metadata | |||
| @@ -137,6 +152,7 @@ func MaxPaginated() Paginated { | |||
| } | |||
|
|
|||
| type Severity struct { | |||
| Metadata | |||
|
I think we need to update the |
There was a problem hiding this comment.
From my perspective, I'd like in this PR the metadata removed from the entities where it should not be part of. @MR2011 did a good job spotting them.
For the other open topics I would suggest creating tickets:
-
As well the topic of adding udpated_by for delete operations
-
instead of getting users by fixed "uniuqeUserId" in each handler write a functionality on the app layer to get the authenticated user (this can then for now do this "statical" fetching)
-
Address Todos
-
Add updated_by to inserts
| @@ -137,6 +137,13 @@ func (a *activityHandler) CreateActivity(activity *entity.Activity) (*entity.Act | |||
| "object": activity, | |||
| }) | |||
|
|
|||
| var err error | |||
| activity.CreatedBy, err = common.GetUserId(a.database, "S0000000") | |||
There was a problem hiding this comment.
That is true, but right now, we do not have the user from the context, right?
I would suggest we solve the fetching of the authenticated user from the context in a separate PR.
| @@ -157,7 +164,14 @@ func (a *activityHandler) UpdateActivity(activity *entity.Activity) (*entity.Act | |||
| "object": activity, | |||
| }) | |||
|
|
|||
| err := a.database.UpdateActivity(activity) | |||
| var err error | |||
| activity.UpdatedBy, err = common.GetUserId(a.database, "S0000000") | |||
|
|
||
| func GetUserId(db database.Database, uniqueUserId string) (int64, error) { | ||
| filter := &entity.UserFilter{UniqueUserID: []*string{&uniqueUserId}} | ||
| ids, err := db.GetAllUserIds(filter) |
There was a problem hiding this comment.
Better get the whole User, we probably going to replace the "AllUserIDs" thing with a "AllCursors" thing
There was a problem hiding this comment.
We could also keep the "GetAllUserIds" function though. Have no strong opinion here!
There was a problem hiding this comment.
"S0000000" is current placeholder, when OIDC auth will be ready we will have correct user name and these name will be existing entry in database.
Anyway we have an option to disable authentication so in this case it will be needed to have some "nobody", "anonymous", "guest" or "none" user, whatever we will name it.
Second problem will be authentication via JWT token, I guess we would need some scannerUser, or we would need to validate user name from JWT (or use systemuser for mentioned purpose).
| @@ -137,6 +137,13 @@ func (a *activityHandler) CreateActivity(activity *entity.Activity) (*entity.Act | |||
| "object": activity, | |||
| }) | |||
|
|
|||
| var err error | |||
| activity.CreatedBy, err = common.GetUserId(a.database, "S0000000") | |||
There was a problem hiding this comment.
Maybe instead of hardcoding the User here we can have a "GetAuthenticatedUser()" method in the App Layer which does for now centrally return only the userID for the system user but we can adjust then only this single function to later get the authenticated user from the application context.
| @@ -208,9 +211,11 @@ func (s *SqlDatabase) CreateActivity(activity *entity.Activity) (*entity.Activit | |||
|
|
|||
| query := ` | |||
| INSERT INTO Activity ( | |||
There was a problem hiding this comment.
In theory we can do that via a trigger on the DB layer, yes. But than we need to add the trigger to each DB Table. Its probably more work then just adding it here.
| @@ -4,6 +4,7 @@ | |||
| package entity | |||
|
|
|||
| type SeverityFilter struct { | |||
| Metadata | |||
| } | ||
|
|
||
| type SupportGroupFilter struct { | ||
| Metadata |
| @@ -22,6 +19,7 @@ type SupportGroupFilter struct { | |||
| } | |||
|
|
|||
| type SupportGroupAggregations struct { | |||
| Metadata | |||
| } | ||
|
|
||
| type UserFilter struct { | ||
| Metadata |
| @@ -34,6 +31,7 @@ type UserFilter struct { | |||
| } | |||
|
|
|||
| type UserAggregations struct { | |||
| Metadata | |||
Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct
Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist