Authentication module #49

javiesses · 2020-09-17T02:57:24Z

Reducer
- Reducer tests
Operations
- Operations tests POC (are commented to prevent breaking CircleCI tests, they point to a local server)
- Operations unit tests with mocked server

The base of the PR is #46 to make it easier to compare the changes, will be changed once that PR is merged

ilanolkies · 2020-09-18T04:05:03Z

packages/rif-id-core/src/operations/authentication.ts

+
+export const serviceLoginFactory = (agent: Agent) => (serviceUrl: string, serviceDid: string, did: string, cb?: Callback<string>) => (dispatch: Dispatch) => callbackify(
+  async () => {
+    const makeLoginCredentialPayload = (challenge: string) => {


We should create this closure outside serviceLoginFactory closure

I left them inside the serviceAuthenticationFactory because they depends on some variables that are received by the parent function. Do you think I should move them outside and send the needed params? I'm talking about did and serviceDid.

We can curry them

ilanolkies · 2020-09-18T04:05:26Z

packages/rif-id-core/src/operations/authentication.ts

+      }
+    }
+
+    const verifyServiceDid = (message: Message): Credential => {


ilanolkies · 2020-09-18T04:08:15Z

packages/rif-id-core/src/operations/authentication.ts

+    return axios.post(`${serviceUrl}/request-auth`, { did })
+      .then(res => res.status === 200 && res.data)
+      .then(data => data.challenge)
+      .then(makeLoginCredentialPayload)


Do you think we should use credential model here or just a signed jwt? I think using just a jwt to respond here makes it simpler and it has the same effect, signing the challenge.

I partially agree on that. It's true that it will become simpler to create just a signed JWT instead of a VC, but take in account that daf-w3c already has an action handler to build those VCs, and if we want to sign a JWT we should build an action handler to do that within daf-did-jwt because it is not created yet, or use directly did-jwt package with no agent involved, we just need to obtain the signer from it.
I think is cleaner to create a daf-did-jwt action handler inside rif-id-daf package

In another hand, if we change the type of the JWT we should change the auth package as well, it is expecting the challenge inside a W3C VC
https://github.com/rsksmart/rif-identity-services/blob/cc4b183b9dc3d60e958be1da87afeafb87aba9ad/rif-id-jwt-auth/src/index.js#L76-L78

we should build an action handler to do that within daf-did-jwt

that's a bummer...

I think is cleaner to create a daf-did-jwt action handler inside rif-id-daf package

I would rise an issue in uPort project to see if they are interested in merging this, what do you think?

we should change the auth package as well

IMHO it worths it

Anyway, before taking any desicion let's think a little bit better on this interface. I guess a good extension in the future will include some credentials in this step

User requests access

Services sends a token and a selective disclosure request

User fills request with declarative details and/or presentations + challange

Service grants access and gives credential to user, probably including metadata taken from the request response.

Based on what we already agreed, let's do the following:

User requests access

Service sends a challenge

Users signs a VP with the received challenge in the VP metadata. The user should include the needed VC inside that VP, if the service does not require any VC, it should send an empty array

Service validates the challenge and performs the needed business validations with the received VCs

If all the validations pass, the service creates a VC and sends it JWT representation to the user

User uses the received VC to authenticate each request to the service

Remarks:

Why the user needs to include VCs in the authentication process? Because the service may need to verify some business logic before granting access, for instance, that the user is older than 18 years old

Why the user will authenticate with the received VC and not with a ad hoc VP for each request? Because there is not added value by doing that and it needs an extra signing step with no sense

Created a new issue in rif-identity-services

ilanolkies · 2020-09-18T04:13:30Z

packages/rif-id-core/src/operations/authentication.ts

+import { ActionSignW3cVc } from 'daf-w3c'
+import { addServiceToken } from '../reducers/authentication'
+
+export const serviceLoginFactory = (agent: Agent) => (serviceUrl: string, serviceDid: string, did: string, cb?: Callback<string>) => (dispatch: Dispatch) => callbackify(


I would use the name “authenticate” instead of “login”, it describes better what this is doing. What do you think about this?

Yep! Sounds better

packages/rif-id-core/src/reducers/authentication.ts

packages/rif-id-core/test/operations/authentication.test.ts

ilanolkies · 2020-09-18T04:20:38Z

packages/rif-id-core/test/operations/authentication.test.ts

+
+    await serviceLogin(serviceUrl, notServiceDid, identity.did)(store.dispatch).catch(e => {
+      expect(e.message).toEqual('The issuer of the auth credential is not the expected did')
+    })


Let’s test the state after the error is thrown. It should be same than before throwing. Adding issue to add it in this one and in the other operations tests

packages/rif-id-core/src/operations/authentication.ts

…llback after succesful login

javiesses requested review from jessgusclark and ilanolkies September 17, 2020 02:57

ilanolkies added this to the v0.1 milestone Sep 17, 2020

ilanolkies added the enhancement New feature or request label Sep 17, 2020

javiesses marked this pull request as ready for review September 17, 2020 19:00

javiesses changed the base branch from handle-credentials to develop September 17, 2020 19:02

javiesses force-pushed the auth-module branch from 69f7c05 to 535f07b Compare September 17, 2020 19:07

ilanolkies suggested changes Sep 18, 2020

View reviewed changes

javiesses force-pushed the auth-module branch from 535f07b to a47d1b1 Compare September 18, 2020 19:54

javiesses mentioned this pull request Sep 18, 2020

Extract pattern used in most of reducer actions #52

Open

ilanolkies mentioned this pull request Sep 21, 2020

rif-id-core: test state after the error is thrown #54

Open

javiesses added 9 commits September 21, 2020 18:21

initialize, add and delete credentials

0e6147c

Authentication reducer

17105da

serviceLogin operation

dc4f3e6

npm audit fix

3e24143

Changed auth operations to prevent it to fail

8cc08ef

Added tests and validation to auth operations

a89ac0b

Improved tests, readability and return the token to be used by the ca…

3674a2f

…llback after succesful login

Rebase

f6ec5e7

Moved internal closures out of the function with needed params curried

6c097d6

javiesses force-pushed the auth-module branch 2 times, most recently from 123f73f to 6c097d6 Compare September 23, 2020 14:23

Update rif-id-daf version

c14179a

ilanolkies approved these changes Sep 24, 2020

View reviewed changes

ilanolkies merged commit 216f1cc into develop Sep 24, 2020

delete-merged-branch bot deleted the auth-module branch September 24, 2020 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication module #49

Authentication module #49

javiesses commented Sep 17, 2020 •

edited

Loading

ilanolkies Sep 18, 2020

javiesses Sep 18, 2020

ilanolkies Sep 21, 2020

ilanolkies Sep 18, 2020

ilanolkies Sep 18, 2020

javiesses Sep 18, 2020

javiesses Sep 18, 2020

ilanolkies Sep 21, 2020

javiesses Sep 21, 2020

javiesses Sep 23, 2020

ilanolkies Sep 18, 2020

javiesses Sep 18, 2020

ilanolkies Sep 18, 2020

Authentication module #49

Authentication module #49

Conversation

javiesses commented Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiesses commented Sep 17, 2020 •

edited

Loading