Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Harvester API handlers #154

Conversation

maxlambrecht
Copy link
Contributor

@maxlambrecht maxlambrecht commented May 11, 2023

Pull request check list

  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality

Add Harvester API handlers for:

  • GET /relationships
  • PATCH /relationships
  • POST /bundle
  • POST /bundles/sync

Description of change

Implementation of the GET relationships handler.
Implementatiojn of the POST bundle handler.
Implementatiojn of the POST bundle/sync handler.
Implementation of the PATCH relationships handler.
Redefinition of the ConsentStatus to make it consistent across all the layers (API, business, datastore), as an enum of three possible values: pending, accepted, denied.
Change the data model to reflect the ConsentStatus properly.

Which issue this pull requests fixes

Part of the work of GLCP-39469

Copy link
Collaborator

@Victorblsilveira Victorblsilveira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very straightforward. There are some similarities with my PR. We may need to align some approaches.
See the comments bellow.

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
@maxlambrecht maxlambrecht changed the title Implement Harvester API: handler "get relationships" Implement Harvester API handlers May 12, 2023
@maxlambrecht maxlambrecht force-pushed the feature/harvester-get-relationships branch from 00e4342 to 7bfa3c1 Compare May 15, 2023 17:20
Copy link
Collaborator

@Victorblsilveira Victorblsilveira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good.
Just some minor comments.

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mgbcaio mgbcaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple more comments. lemme know what you think.

pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
pkg/server/endpoints/harvester.go Outdated Show resolved Hide resolved
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
@maxlambrecht maxlambrecht force-pushed the feature/harvester-get-relationships branch from 2df9473 to a004a68 Compare May 16, 2023 14:16
Signed-off-by: Max Lambrecht <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@mgbcaio mgbcaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚲

@maxlambrecht maxlambrecht merged commit 558818e into HewlettPackard:main May 16, 2023
@maxlambrecht maxlambrecht deleted the feature/harvester-get-relationships branch May 16, 2023 14:35
Copy link
Contributor

@wibarre wibarre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Max for all your work. This looks great! A left a couple of comments.

}

// PatchRelationshipsRelationshipID accept/denied relationships requests - (PATCH /relationships/{relationshipID})
func (h *HarvesterAPIHandlers) PatchRelationshipsRelationshipID(ctx echo.Context, relationshipID api.UUID) error {
// PatchRelationship accept/denies relationships requests - (PATCH /relationships/{relationshipID})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit picky
// PatchRelationship accepts/denies relationships requests - (PATCH /relationships/{relationshipID}

}

// update the relationship consent status for the authenticated trust domain
if relationship.TrustDomainAID == authTD.ID.UUID {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check explicitly that the relationship.TrustDomainB == authTD.ID.UUID instead of having the else statement? Could we have a case that relationship.TrustdomainA or B does not match authTD.ID.UUID? Should we return an error if the authenticated trust domain does not match A or B in the relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a validation above preventing that case:

if relationship.TrustDomainAID != authTD.ID.UUID && relationship.TrustDomainBID != authTD.ID.UUID {
		err := fmt.Errorf("relationship doesn't belong to the authenticated trust domain")
		return h.handleErrorAndLog(err, err.Error(), http.StatusUnauthorized)
	}

return h.handleErrorAndLog(err, msg, http.StatusInternalServerError)
}

return echoCtx.JSON(http.StatusOK, newToken)
return chttp.WriteResponse(echoCtx, newToken)
}

// BundleSync synchronize the status of trust bundles between server and harvester - (POST /trust-domain/{trustDomainName}/bundles/sync)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit picky
// BundleSync synchronizes the status of trust bundles between server and harvester - (POST /trust-domain/{trustDomainName}/bundles/sync)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a grammar checker 😀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using spelling check in the Visual Studio.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My IDE has a checker, but IDK why it doesn't detect that error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants