feat(authN): Add to all DB entries 'Modified_by'... (#81) #230

michalkrzyz · 2024-09-17T12:27:28Z

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

Related Issue # (issue)
Closes # (issue)
Fixes # (issue)

Remove if not applicable

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help
Separate ticket for tests # (issue/pr)

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?

📜 README.md
🤝 Documentation pages updated
🙅 no documentation needed
(if applicable) generated OpenAPI docs for CRD changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

internal/entity/common.go

internal/e2e/metadata_test.go

internal/api/graphql/graph/schema/activity.graphqls

internal/database/mariadb/init/schema.sql

internal/app/activity/activity_handler.go

internal/api/graphql/graph/model/models.go

internal/app/activity/activity_handler.go

MR2011 · 2024-10-08T07:22:22Z

internal/api/graphql/graph/queryCollection/componentInstance/directRelations.graphql

Most queries don't include the createdBy and updatedBy fields. Do we want to add them?

I have added them only in the files used directly in testing of 'createdBy'/'updatedBy'

internal/api/graphql/graph/schema/issue_variant.graphqls

internal/app/component/component_handler.go

internal/app/component_instance/component_instance_handler.go

internal/app/component_version/component_version_handler.go

MR2011 · 2024-10-08T07:25:15Z

internal/app/evidence/evidence_handler.go

@@ -104,6 +104,7 @@ func (e *evidenceHandler) ListEvidences(filter *entity.EvidenceFilter, options *
 }

 func (e *evidenceHandler) CreateEvidence(evidence *entity.Evidence) (*entity.Evidence, error) {
+	evidence.CreatedBy = "Creator"


Another placeholder

internal/app/evidence/evidence_handler.go

internal/app/issue/issue_handler.go

MR2011 · 2024-10-08T07:26:49Z

internal/database/mariadb/activity.go

@@ -208,9 +211,11 @@ func (s *SqlDatabase) CreateActivity(activity *entity.Activity) (*entity.Activit

 	query := `
 		INSERT INTO Activity (


aren't we missing the updated_by field? This is also in the other db queries the case

Do we need to insert it during creation of an object? is it possible that updatedBy will be given when the object is created in database?

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.

internal/entity/activity.go

internal/entity/component.go

internal/e2e/common/issue.go

internal/entity/common.go

MR2011 · 2024-11-07T13:06:19Z

I think we need to update the updated_by field also in all delete operations

drochow

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

internal/app/activity/activity_handler.go

drochow · 2024-11-07T13:15:31Z

internal/app/common/user_id.go

+
+func GetUserId(db database.Database, uniqueUserId string) (int64, error) {
+	filter := &entity.UserFilter{UniqueUserID: []*string{&uniqueUserId}}
+	ids, err := db.GetAllUserIds(filter)


Better get the whole User, we probably going to replace the "AllUserIDs" thing with a "AllCursors" thing

We could also keep the "GetAllUserIds" function though. Have no strong opinion here!

"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).

internal/app/activity/activity_handler.go

drochow · 2024-11-07T13:28:40Z

internal/database/mariadb/activity.go

@@ -208,9 +211,11 @@ func (s *SqlDatabase) CreateActivity(activity *entity.Activity) (*entity.Activit

 	query := `
 		INSERT INTO Activity (


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.

internal/entity/severity.go

internal/entity/support_group.go

internal/entity/user.go

drochow

LGTM; as discussed remaining comments can be addressed in separate issues.

Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct Add metadata for user Add tests Add metadata for activity, IssueVariant and IssueRepository Add metadata for component Add metadata for componentInstance, componentVersion 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 Refactor and review fixes Test foreign key Add Foreign Key changed issueMetadata and serviceMetadata to objectMetadata

* feat(authN): Add to all DB entries 'Modified_by'... (#81) Move 'CreatedAt', 'DeletedAt' and 'UpdatedAt' to common entity.Info struct Add 'CreatedBy' and 'UpdatedBy' to common entity.Info struct Add metadata for user Add tests Add metadata for activity, IssueVariant and IssueRepository Add metadata for component Add metadata for componentInstance, componentVersion 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 Refactor and review fixes Test foreign key Add Foreign Key changed issueMetadata and serviceMetadata to objectMetadata * chore: bumping for re-trigger * Automatic application of license header --------- Co-authored-by: David Rochow <[email protected]> Co-authored-by: License Bot <[email protected]>

michalkrzyz force-pushed the mikrzyz/issue-81 branch from e084f8b to cb2f9c1 Compare September 17, 2024 14:51

drochow reviewed Sep 18, 2024

View reviewed changes

internal/entity/common.go Outdated Show resolved Hide resolved

michalkrzyz force-pushed the mikrzyz/issue-81 branch 7 times, most recently from 94c222f to 2cdfe8c Compare September 26, 2024 17:39

MR2011 reviewed Sep 30, 2024

View reviewed changes

internal/e2e/metadata_test.go Outdated Show resolved Hide resolved

MR2011 reviewed Sep 30, 2024

View reviewed changes

internal/e2e/metadata_test.go Outdated Show resolved Hide resolved

michalkrzyz force-pushed the mikrzyz/issue-81 branch from 2cdfe8c to b8d5d0a Compare October 1, 2024 16:09

drochow requested changes Oct 8, 2024

View reviewed changes

dorneanu reviewed Oct 8, 2024

View reviewed changes

internal/app/activity/activity_handler.go Outdated Show resolved Hide resolved