Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

Support new implementation for requesting claims in selective disclosure #99

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ugoamanoh
Copy link
Contributor

@ugoamanoh ugoamanoh commented May 21, 2019

What's here

Added a new claims param to the payload when making selective disclosure requests. This keeps us up to date with the latest changes in the specs Selective Disclosure Request
Created a helper class to make it easier for developers to create claims while making requests

Testing

Run the gradle command ./gradlew test cC --no-parallel

@ugoamanoh ugoamanoh changed the title [wip] Support new implementation for requesting claims in selective disclosure Support new implementation for requesting claims in selective disclosure May 23, 2019
Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

I made a few suggestions.

In a way building request using maps directly looks cleaner.
I would love it if we could come up with a DSL with the same look and feel but with proper type checks.

This PR works from a type safety perspective but I think we can make it a little more usable.

I'm thinking of something along these lines:

val claimsReq = ClaimsRequest()
.requestCredential("email") {
    issuedBy("did:web:uport.claims")
    issuedBy("did:web:sobol.io", "https://sobol.io/verify"))
    essential false
    reason "To contact you with regard to your account"
  }
.requestUserInfo("name") {
    essential true
    reason "To show your name when you are logged in"
  }

Then, to keep type safety all the way through, the SelectiveDisclosureParams can accept ClaimsRequest object instead of generic Map<String, Any> and perform the conversion to a payload map in buildPayloadForShareReq

What do you think?

* added to the selective disclosure request
*
*/
class ClaimsRequestParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this requires a call to build() to be used in a request, it makes sense to call it ClaimsRequestBuilder or something along those lines.
This class does not encapsulate the params themselves.

@@ -121,6 +128,7 @@ internal fun buildPayloadForShareReq(params: SelectiveDisclosureRequestParams):
val payload = params.extras.orEmpty().toMutableMap()

payload["callback"] = params.callbackUrl
payload["claims"] = params.claims.orEmpty().toMutableMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

since the claims field does not get modified later on, there is probably no need to make it mutable here.

Suggested change
payload["claims"] = params.claims.orEmpty().toMutableMap()
payload["claims"] = params.claims.orEmpty()

* [specs](https://github.com/uport-project/specs/blob/develop/messages/sharereq.md#claims-spec)
*
*/
val claims: Map<String, Any?>? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

The claims param makes the other ones obsolete.
Should we print out a warning when claims param is used along with requested and verified ?

)
.addIssuer("did:web:idverifier.claims", "https://idverifier.example")
)
.addUserInfo("name", UserInfoParams("Show your name to other users", true))
Copy link
Contributor

Choose a reason for hiding this comment

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

since UserInfoParams can only be used in this context, it makes sense to have the parameters be directly passed to the .addUserInfo() method.
This call would then look like this:

Suggested change
.addUserInfo("name", UserInfoParams("Show your name to other users", true))
.addUserInfo("name", "Show your name to other users", true)

It is a bit more concise and can provide the same parameter type safety

@@ -36,11 +31,70 @@ class uPortLoginActivity : AppCompatActivity() {
// create a DID
val issuerDID = "did:ethr:${signer.getAddress()}"

/*@Suppress("StringLiteralDuplication")
val claim = mapOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

in a way, this way of building the request looks cleaner.
I would love it if we could come up with a DSL with the same look and feel but with proper type checks

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

Successfully merging this pull request may close these issues.

2 participants