This repository has been archived by the owner on Apr 5, 2023. It is now read-only.

feat: Validate and check Waci profile creation that each scope has output descriptors configured. #586

Merged

talwinder50 merged 1 commit into trustbloc:main from talwinder50:issue-581

Jan 27, 2022

Member

talwinder50 commented Jan 27, 2022

closes #581

Signed-off-by: talwinder50 [email protected]

cla-bot bot added the cla-signed label

talwinder50 force-pushed the issue-581 branch from 73027df to e4c8883 Compare

January 27, 2022 13:36

talwinder50 changed the title ~~feat: Validate and check Waci profile creation that each scope has output descriptors configured.~~ WIP feat: Validate and check Waci profile creation that each scope has output descriptors configured.

talwinder50 force-pushed the issue-581 branch from e4c8883 to cd8c6ac Compare

January 27, 2022 19:51

talwinder50 changed the title ~~WIP feat: Validate and check Waci profile creation that each scope has output descriptors configured.~~ feat: Validate and check Waci profile creation that each scope has output descriptors configured.

talwinder50 force-pushed the issue-581 branch 2 times, most recently from 0b05a86 to b9e3b49 Compare

January 27, 2022 19:56

talwinder50 requested review from Moopli, rolsonquadras and DRK3

January 27, 2022 19:57

DRK3 reviewed

View reviewed changes

pkg/restapi/issuer/operation/models.go Outdated

@@ @@ -46,6 +48,7 @@ type WalletConnect struct { @@
               // txnData contains session data.
               type txnData struct {
+              	// Todo rename this to profile ID

Collaborator

DRK3 Jan 27, 2022

Should this todo still be here?

Member Author

talwinder50 Jan 27, 2022

yes the issuerID is now misleading it should be profileID added specifically on the value to be renamed. Didnt mention issue number as it can be done part of any follow up prs.

DRK3 reviewed

View reviewed changes

pkg/restapi/issuer/operation/operations.go Outdated

+              		for _, pCredScope := range profileData.CredentialScopes {
+              			if _, ok := o.cmOutputDescriptor[pCredScope]; !ok {
+              				commhttp.WriteErrorResponseWithLog(rw, http.StatusInternalServerError,
+              					fmt.Sprintf("failed to get output descriptors configured for waci "+

Collaborator

DRK3 Jan 27, 2022

Two things:

This string has no formatting directives, so you can simply use the string without calling fmt.Sprintf
I think this error message isn't specific enough - why couldn't the output descriptors get configured? What exactly went wrong?

Member Author

talwinder50 Jan 27, 2022

It has formatting directives now

DRK3 reviewed

View reviewed changes

pkg/restapi/issuer/operation/operations_test.go Outdated

@@ @@ -646,6 +686,31 @@ func TestCreateProfile(t *testing.T) { @@
               		require.Contains(t, resErr.ErrMessage, "create oidc client")
               	})
+              	t.Run("create profile - failed to output descriptors configured for waci profiles", func(t *testing.T) {

Collaborator

DRK3 Jan 27, 2022

You probably mean "failed to configure output descriptors for WACI profiles" instead

codecov bot commented Jan 27, 2022 •

edited

Loading

Codecov Report

Merging #586 (c01fddb) into main (30ecbb5) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   87.70%   87.86%   +0.16%     
==========================================
  Files          28       28              
  Lines        4106     4119      +13     
==========================================
+ Hits         3601     3619      +18     
+ Misses        299      294       -5     
  Partials      206      206

Impacted Files	Coverage Δ
pkg/profile/issuer/profile.go	`90.00% <100.00%> (+0.52%)`	⬆️
pkg/restapi/issuer/operation/operations.go	`89.98% <100.00%> (+0.53%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30ecbb5...c01fddb. Read the comment docs.

DRK3 reviewed

View reviewed changes

test/bdd/pkg/issuer/issuer_steps.go Outdated

-              	e.states[issuerID] = state
+              	e.states[profileID] = state
+              	resp, err := bddutil.HTTPDo(http.MethodGet, //nolint: bodyclose

Collaborator

DRK3 Jan 27, 2022

Is this a false positive for the linter? If so, you can add a small comment next to the nolint to let other developers know. (I've also encountered this false positive, there's an open issue for it)

//nolint: bodyclose // False positive

DRK3 reviewed

View reviewed changes

test/bdd/pkg/issuer/issuer_steps.go Outdated

+              	// validating only status code as the vue page needs javascript support
+              	if resp.StatusCode != http.StatusOK {
+              		// nolint:wrapcheck // ignore

Collaborator

DRK3 Jan 27, 2022

You should replace // ignore with a specific reason of why you need the nolint

DRK3 reviewed

View reviewed changes

test/bdd/pkg/issuer/issuer_steps.go


		defer bddutil.CloseResponseBody(resp.Body)

		// validating only status code as the vue page needs javascript support

Collaborator

DRK3 Jan 27, 2022

Not sure what vue pages/Javascript have to do with this. Can you explain?

(It might just be me who isn't familiar with how this works, so let me know)

Member Author

talwinder50 Jan 27, 2022

This is the existing code i only changed variable name and this endpoint gets automatically redirects to vue page thats why its written here that we are only testing status code

DRK3 reviewed

View reviewed changes

test/bdd/pkg/issuer/issuer_steps.go Outdated

-              func (e *Steps) createProfileWithWACI(id, name, issuerURL, supportedVCContexts, linkedWallet, oidcProvider string) error { //nolint:lll
-              	err := e.createProfile(id, name, issuerURL, supportedVCContexts, "false", "false", oidcProvider, linkedWallet, true)
+              // id, name, issuerURL, supportedVCContexts, credScope, issuerID, linkedWallet, oidcProvider string
+              func (e *Steps) createProfileWithWACI(id, name, issuerURL, supportedVCContexts, credScopes, issuerID, linkedWallet, oidcProvider string) error { //nolint:lll

Collaborator

DRK3 Jan 27, 2022

With how many arguments are in this function, it makes sense to create a custom struct type to hold them all instead. This will also help to resolve the lll linter complaint.

Member Author

talwinder50 Jan 27, 2022

Can be done as part of #593

talwinder50 mentioned this pull request

Refactor bdd test input parameters to struct #593

Closed

Moopli reviewed

View reviewed changes

test/bdd/pkg/issuer/issuer_steps.go Outdated

-              func (e *Steps) createProfileWithWACI(id, name, issuerURL, supportedVCContexts, linkedWallet, oidcProvider string) error { //nolint:lll
-              	err := e.createProfile(id, name, issuerURL, supportedVCContexts, "false", "false", oidcProvider, linkedWallet, true)
+              // id, name, issuerURL, supportedVCContexts, credScope, issuerID, linkedWallet, oidcProvider string

Contributor

Moopli Jan 27, 2022

leftover commented out param list from when you were editing this?

Moopli approved these changes

View reviewed changes

talwinder50 force-pushed the issue-581 branch 2 times, most recently from cfd16a4 to fe0ac37 Compare

January 27, 2022 20:28

rolsonquadras reviewed

View reviewed changes

pkg/profile/issuer/profile.go Outdated

@@ @@ -125,6 +125,12 @@ func validateProfileRequest(pr *ProfileData) error { @@
               		return fmt.Errorf("supported vc contexts mandatory")
               	}
+              	if pr.SupportsWACI {
+              		if pr.IssuerID == "" {
+              			return fmt.Errorf("issuer id mandatory for waci profiles")

Member

rolsonquadras Jan 27, 2022

can we combine this into one line (if pr.SupportsWACI && pr.IssuerID == "") ?

pkg/restapi/issuer/operation/models.go Outdated

@@ @@ -16,6 +16,8 @@ import ( @@
               )
               // ProfileDataRequest req for profile creation.
+              // Issuer ID identifies who is the issuer of the credential manifests being issued.
+              // CMStyle represents an entity styles object as defined in credential manifest spec.

Member

rolsonquadras Jan 27, 2022

Can we add these field level specific comments at field level than at struct level ?

pkg/restapi/issuer/operation/operations.go Outdated

+              	if profileData.SupportsWACI {
+              		for _, pCredScope := range profileData.CredentialScopes {
+              			if _, ok := o.cmOutputDescriptor[pCredScope]; !ok {
+              				commhttp.WriteErrorResponseWithLog(rw, http.StatusInternalServerError,

Member

rolsonquadras Jan 27, 2022

This should be BadRequest error code than ServerError.

pkg/restapi/issuer/operation/operations.go Outdated

+              		for _, pCredScope := range profileData.CredentialScopes {
+              			if _, ok := o.cmOutputDescriptor[pCredScope]; !ok {
+              				commhttp.WriteErrorResponseWithLog(rw, http.StatusInternalServerError,
+              					fmt.Sprint("failed to find the allowed cred scope in credential manifest output descriptor "+

Member

rolsonquadras Jan 27, 2022

Its better to pass the missing scope in the response message as to be more explicit about failed scope to the caller.

Collaborator

DRK3 Jan 27, 2022

One other thing I'd like to suggest...

Is this scenario something that can happen due to an incorrect configuration by the user? If so - you can add something here to make it more explicit how it happens and give them a hint as to how to fix it. (Something like are the scopes configured?)

However, if that configuration is already validated and this error is only here because of the ok check (and so this scenario is essentially impossible) then you can ignore what I wrote above.

Collaborator

DRK3 Jan 27, 2022

By impossible, I mean that there's no way a user can end up in this situation without modifying the code somehow.

talwinder50 force-pushed the issue-581 branch 2 times, most recently from 15775b0 to 0f0529f Compare

January 27, 2022 20:50

talwinder50 requested review from rolsonquadras and DRK3

January 27, 2022 20:59


          feat: Validate and check Waci profile creation that each scope has ou…

c01fddb

…tput descriptors configured.

closes trustbloc#581

Signed-off-by: talwinder50 <[email protected]>

talwinder50 force-pushed the issue-581 branch from 0f0529f to c01fddb Compare

January 27, 2022 21:13

DRK3 approved these changes

View reviewed changes

talwinder50 merged commit de984ef into trustbloc:main

sudeshrshetty reviewed

View reviewed changes

pkg/profile/issuer/profile.go

@@ @@ -125,6 +125,10 @@ func validateProfileRequest(pr *ProfileData) error { @@
               		return fmt.Errorf("supported vc contexts mandatory")
               	}
+              	if pr.SupportsWACI && pr.IssuerID == "" {
+              		return fmt.Errorf("issuer id mandatory for waci profiles")

Member

sudeshrshetty Jan 31, 2022

No, it is not mandatory, you can assign UUID if not set. Refer my comments from original PR.

pkg/restapi/issuer/operation/models.go

@@ @@ -46,6 +48,7 @@ type WalletConnect struct { @@
               // txnData contains session data.
               type txnData struct {
+              	// Todo #580 rename IssuerID to ProfileID

Member

sudeshrshetty Jan 31, 2022

Why renaming issuerID to profileID???
#580 doesn't mention any of this.

Member Author

talwinder50 Jan 31, 2022

#580 (comment) @sudeshrshetty This was my reason,

talwinder50 mentioned this pull request

fix: Update profile issuer id to be not mandatory #594

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels