-
Notifications
You must be signed in to change notification settings - Fork 208
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
chore: Remove getIdentityByDID route #1558
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||
// Copyright © 2022 Kaleido, Inc. | ||||||
// | ||||||
// SPDX-License-Identifier: Apache-2.0 | ||||||
// | ||||||
|
@@ -20,17 +20,19 @@ | |||||
"net/http" | ||||||
"strings" | ||||||
|
||||||
"github.com/google/uuid" | ||||||
"github.com/hyperledger/firefly-common/pkg/ffapi" | ||||||
"github.com/hyperledger/firefly-common/pkg/i18n" | ||||||
"github.com/hyperledger/firefly/internal/coremsgs" | ||||||
"github.com/hyperledger/firefly/pkg/core" | ||||||
) | ||||||
|
||||||
var getIdentityByID = &ffapi.Route{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
Name: "getIdentityByID", | ||||||
Path: "identities/{iid}", | ||||||
Path: "identities/{id:.+}", | ||||||
EnriqueL8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
Method: http.MethodGet, | ||||||
PathParams: []*ffapi.PathParam{ | ||||||
{Name: "iid", Example: "id", Description: coremsgs.APIParamsIdentityID}, | ||||||
{Name: "id", Example: "id", Description: coremsgs.APIParamsIdentityIDs}, | ||||||
}, | ||||||
QueryParams: []*ffapi.QueryParam{ | ||||||
{Name: "fetchverifiers", Example: "true", Description: coremsgs.APIParamsFetchVerifiers, IsBool: true}, | ||||||
|
@@ -41,10 +43,30 @@ | |||||
JSONOutputCodes: []int{http.StatusOK}, | ||||||
Extensions: &coreExtensions{ | ||||||
CoreJSONHandler: func(r *ffapi.APIRequest, cr *coreRequest) (output interface{}, err error) { | ||||||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||||||
return cr.or.NetworkMap().GetIdentityByIDWithVerifiers(cr.ctx, r.PP["iid"]) | ||||||
switch id := r.PP["id"]; { | ||||||
case isUUID(id): | ||||||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||||||
return cr.or.NetworkMap().GetIdentityByIDWithVerifiers(cr.ctx, id) | ||||||
} | ||||||
return cr.or.NetworkMap().GetIdentityByID(cr.ctx, id) | ||||||
case isDID(id): | ||||||
if strings.EqualFold(r.QP["fetchverifiers"], "true") { | ||||||
return cr.or.NetworkMap().GetIdentityByDIDWithVerifiers(cr.ctx, id) | ||||||
} | ||||||
return cr.or.NetworkMap().GetIdentityByDID(cr.ctx, id) | ||||||
} | ||||||
return cr.or.NetworkMap().GetIdentityByID(cr.ctx, r.PP["iid"]) | ||||||
return nil, i18n.NewError(cr.ctx, i18n.MsgUnknownIdentityType) | ||||||
}, | ||||||
}, | ||||||
} | ||||||
|
||||||
// isUUID checks if the given string is a UUID | ||||||
func isUUID(s string) bool { | ||||||
_, err := uuid.Parse(s) | ||||||
return err == nil | ||||||
} | ||||||
|
||||||
// isDID checks if the given string is a potential DID | ||||||
func isDID(s string) bool { | ||||||
return strings.HasPrefix(s, "did:") | ||||||
} | ||||||
Comment on lines
+70
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the same level of check and change this to a regex expression - I think we should have one for DIDs already in the codebase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have the DID validation happening later in the code, I preferred to determine if this a DID string or not. Because if this is a DID but a not valid one, I want the user to receive the proper error code (vs in this case the error code will be "unknown format"). wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah because you are not catching the error in the switch statement, you could catch the error there if you wish to? It just feels since you have moved the validation here make sense to construct the correct error instead of relying on the function underneath to throw the right thing - but will leave up to you |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,7 +288,7 @@ func (as *apiServer) nsOpenAPIHandlerFactory(req *http.Request, publicURL string | |
} | ||
} | ||
|
||
func (as *apiServer) namespacedSwaggerHandler(hf *ffapi.HandlerFactory, r *mux.Router, publicURL, relativePath string, format ffapi.OpenAPIFormat) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably better to fix the bug and not remove this |
||
func (as *apiServer) namespacedSwaggerHandler(hf *ffapi.HandlerFactory, r *mux.Router, publicURL, relativePath string) { | ||
r.HandleFunc(`/api/v1/namespaces/{ns}`+relativePath, hf.APIWrapper(func(res http.ResponseWriter, req *http.Request) (status int, err error) { | ||
return as.nsOpenAPIHandlerFactory(req, publicURL).OpenAPIHandler("", ffapi.OpenAPIFormatJSON, nsRoutes)(res, req) | ||
})) | ||
|
@@ -300,7 +300,7 @@ func (as *apiServer) namespacedSwaggerUI(hf *ffapi.HandlerFactory, r *mux.Router | |
})) | ||
} | ||
|
||
func (as *apiServer) namespacedContractSwaggerGenerator(hf *ffapi.HandlerFactory, r *mux.Router, mgr namespace.Manager, publicURL, relativePath string, format ffapi.OpenAPIFormat) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here keep this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This just seems to be a bug that the publicURL is maybe not used? |
||
func (as *apiServer) namespacedContractSwaggerGenerator(hf *ffapi.HandlerFactory, r *mux.Router, mgr namespace.Manager, relativePath string, format ffapi.OpenAPIFormat) { | ||
r.HandleFunc(`/api/v1/namespaces/{ns}/apis/{apiName}`+relativePath, hf.APIWrapper(func(res http.ResponseWriter, req *http.Request) (status int, err error) { | ||
vars := mux.Vars(req) | ||
or, err := mgr.Orchestrator(req.Context(), vars["ns"], false) | ||
|
@@ -372,16 +372,16 @@ func (as *apiServer) createMuxRouter(ctx context.Context, mgr namespace.Manager) | |
r.HandleFunc(`/api/openapi.yaml`, hf.APIWrapper(oaf.OpenAPIHandler(`/api/v1`, ffapi.OpenAPIFormatYAML, routes))) | ||
r.HandleFunc(`/api`, hf.APIWrapper(oaf.SwaggerUIHandler(`/api/openapi.yaml`))) | ||
// Namespace relative APIs | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/swagger.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/openapi.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/swagger.yaml`, ffapi.OpenAPIFormatYAML) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/openapi.yaml`, ffapi.OpenAPIFormatYAML) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/swagger.json`) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/openapi.json`) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/swagger.yaml`) | ||
as.namespacedSwaggerHandler(hf, r, as.apiPublicURL, `/api/openapi.yaml`) | ||
as.namespacedSwaggerUI(hf, r, as.apiPublicURL, `/api`) | ||
// Dynamic swagger for namespaced contract APIs | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/swagger.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/openapi.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/swagger.yaml`, ffapi.OpenAPIFormatYAML) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, as.apiPublicURL, `/api/openapi.yaml`, ffapi.OpenAPIFormatYAML) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to keep this change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's just the linter is saying that the arguments are not being used in the function - ideally we fix the bug there and make it work properly, maybe not as part of this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will revert my changes and open an issue so we can address this later |
||
as.namespacedContractSwaggerGenerator(hf, r, mgr, `/api/swagger.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, `/api/openapi.json`, ffapi.OpenAPIFormatJSON) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, `/api/swagger.yaml`, ffapi.OpenAPIFormatYAML) | ||
as.namespacedContractSwaggerGenerator(hf, r, mgr, `/api/openapi.yaml`, ffapi.OpenAPIFormatYAML) | ||
as.namespacedContractSwaggerUI(hf, r, as.apiPublicURL, `/api`) | ||
|
||
r.HandleFunc(`/favicon{any:.*}.png`, favIcons) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -48,6 +48,7 @@ var ( | |||||
APIParamsGroupHash = ffm("api.params.groupID", "The hash of the group") | ||||||
APIParamsFetchVerifiers = ffm("api.params.fetchVerifiers", "When set, the API will return the verifier for this identity") | ||||||
APIParamsIdentityID = ffm("api.params.identityID", "The identity ID, which is a UUID generated by FireFly") | ||||||
APIParamsIdentityIDs = ffm("api.params.identityIDs", "The identity ID, which is a UUID generated by FireFly or the identity DID") | ||||||
APIParamsMessageID = ffm("api.params.messageID", "The message ID") | ||||||
APIParamsDID = ffm("api.params.DID", "The identity DID") | ||||||
APIParamsNodeNameOrID = ffm("api.params.nodeNameOrID", "The name or ID of the node") | ||||||
|
@@ -107,7 +108,7 @@ var ( | |||||
APIEndpointsGetGroupByHash = ffm("api.endpoints.getGroupByHash", "Gets a group by its ID (hash)") | ||||||
APIEndpointsGetGroups = ffm("api.endpoints.getGroups", "Gets a list of groups") | ||||||
APIEndpointsGetIdentities = ffm("api.endpoints.getIdentities", "Gets a list of all identities that have been registered in the namespace") | ||||||
APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID") | ||||||
APIEndpointsGetIdentityByID = ffm("api.endpoints.getIdentityByID", "Gets an identity by its ID (UUID/DID)") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
APIEndpointsGetIdentityDID = ffm("api.endpoints.getIdentityDID", "Gets the DID for an identity based on its ID") | ||||||
APIEndpointsGetIdentityVerifiers = ffm("api.endpoints.getIdentityVerifiers", "Gets the verifiers for an identity") | ||||||
APIEndpointsGetMsgByID = ffm("api.endpoints.getMsgByID", "Gets a message by its ID") | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the google UUID library directly, use the one in firefly-common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we are not validating the input, rather we are just filtering the input, so I didn't think we need to call the UUID validator with the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just more of a consistency and dependency thing to use the firefly-common library for this