API Feedback from Aurel

[tahpot]

Closes #113 #108 #105 #111 #112 #106 #107

[aurelticot]

@tahpot let me know when the PR is ready for review. I see you're adding a few more things

[tahpot]

Note: This refactor has made a lot of breaking changes.

You will need to disconnect any existing connections you have and then re-create them before using this branch.

[aurelticot]

See inline comments

---

Additionally, I was unable to connect to Google while testing with the embedded web dev UI.

Got the following error:

It refers to:

data-connector-server/src/utils.ts

Lines 59 to 64 in 16b742a

	const account = new AutoAccount({
	privateKey: contextSignature,
	network: VERIDA_ENVIRONMENT,
	// @ts-ignore
	didClientConfig: DID_CLIENT_CONFIG
	})

The network and DID client config come from my serverconfig.local.json whic seem correct.

The contextSignature is passed when calling Utils.getNetwork in the `callback controller function.

data-connector-server/src/providers/controller.ts

Lines 88 to 100 in 16b742a

	public static async callback(req: UniqueRequest, res: Response, next: any) {
	logger.trace('callback()')
	const providerName = req.params.provider
	const provider = Providers(providerName)
	try {
	const connectionResponse = await provider.callback(req, res, next)
	const did = req.session.did
	const key = req.session.key
	const redirect = req.session.redirect
	const networkInstance = await Utils.getNetwork(key, req.requestId)

The key comes from the session, this may be the missing element

[tahpot]

See inline comments

Additionally, I was unable to connect to Google while testing with the embedded web dev UI.

Got the following error:

It refers to:

data-connector-server/src/utils.ts

Lines 59 to 64 in 16b742a

	const account = new AutoAccount({
	privateKey: contextSignature,
	network: VERIDA_ENVIRONMENT,
	// @ts-ignore
	didClientConfig: DID_CLIENT_CONFIG
	})

The network and DID client config come from my serverconfig.local.json whic seem correct.

The contextSignature is passed when calling Utils.getNetwork in the `callback controller function.

data-connector-server/src/providers/controller.ts

Lines 88 to 100 in 16b742a

	public static async callback(req: UniqueRequest, res: Response, next: any) {
	logger.trace('callback()')
	const providerName = req.params.provider
	const provider = Providers(providerName)
	try {
	const connectionResponse = await provider.callback(req, res, next)
	const did = req.session.did
	const key = req.session.key
	const redirect = req.session.redirect
	const networkInstance = await Utils.getNetwork(key, req.requestId)

The key comes from the session, this may be the missing element

I had this issue as well. There's a bug in the UI where entering the private key for the first time in the connection screen doesn't work. Enter the private key, then reload the page, then try to connect.

[aurelticot]

I had this issue as well. There's a bug in the UI where entering the private key for the first time in the connection screen doesn't work. Enter the private key, then reload the page, then try to connect.

Ok.
I was about to comment tohat I don't have this issue in the Vault web when using this branch for the server, so yes it comes from the embedded dev web UI.

[aurelticot]

Just did a static review. I'm starting the server and updating the Vault to actually test it.

I resolved the inline comments from the previous review except for a couple for which I added new comments for you

[aurelticot]

Got another issue with the readableId of the profile.

When connecting a new account, once in the callback controller, it calls the specific callback of the provider:

data-connector-server/src/providers/controller.ts

Line 94 in 5f0cf9f

const connectionResponse = await provider.callback(req, res, next)

In the case of Google, it sets the profile with the username and readableId using the email.

data-connector-server/src/providers/google/index.ts

Lines 82 to 87 in 5f0cf9f

	profile: {
	username: data.profile.email,
	readableId: data.profile.email,
	...data.profile,
	}
	};

Back in the callback controller, it eventually saveProvider (should actually be called saveNewConnection to be more relevant)

data-connector-server/src/providers/controller.ts

Line 103 in 5f0cf9f

const connection = await syncManager.saveProvider(providerId, connectionResponse.accessToken, connectionResponse.refreshToken, connectionResponse.profile)

This function redefined the profile and overwrite whatever was in readableId and forget to include username:

data-connector-server/src/sync-manager.ts

Lines 171 to 184 in 5f0cf9f

	const connectionProfile: ConnectionProfile = {
	id: profile.id,
	name: profile.displayName,
	avatar: {
	uri: profile.photos && profile.photos.length ? profile.photos[0].value : undefined
	},
	readableId: `${profile.displayName} (${profile.id})`,
	//uri:
	givenName: profile.name.givenName,
	familyName: profile.name.familyName,
	email: profile.emails && profile.emails.length ? profile.emails[0].value : undefined,
	...(profile.connectionProfile ? profile.connectionProfile : {})
	}

It can be easily fixed with the following (but also requires fixing the types of the profile):

const connectionProfile: ConnectionProfile = {
            id: profile.id,
            name: profile.displayName,
            avatar: {
                uri: profile.photos && profile.photos.length ? profile.photos[0].value : undefined
            },
            readableId: profile.readableId || `${profile.displayName} (${profile.id})`,
            username: profile.username,
            givenName: profile.name.givenName,
            familyName: profile.name.familyName,
            email: profile.emails && profile.emails.length ? profile.emails[0].value : undefined,
            ...(profile.connectionProfile ? profile.connectionProfile : {})
        }

Back to Google: data.profile.email doesn't exist in the Passport profile, it's data.profile.emails, hence the importance of typing passport properly, it would have been caught.

[tahpot]

Back to Google: data.profile.email doesn't exist in the Passport profile, it's data.profile.emails, hence the importance of typing passport properly, it would have been caught.

Should be fixed with: 218a7d8

[tahpot]

Back in the callback controller, it eventually saveProvider (should actually be called saveNewConnection to be more relevant)

Fixed.

It can be easily fixed with the following (but also requires fixing the types of the profile)

Should also be fixed.

See 3007d70

[aurelticot]

All good. Thanks