feat(profile): Add API endpoints for ecosystem_anon_id #5364

jaredhirsch · 2020-05-19T22:31:24Z

Starting off the AET work with an API that will need to agree with changes that ride the trains into desktop. This ecosystem_anon_id field isn't yet persisted in the core profile fields in the auth DB, that's a different bug. Not 100% sure I'm logging correctly. Still need to add tests to api.js.

codecov-commenter · 2020-05-19T22:49:55Z

Codecov Report

Merging #5364 into master will decrease coverage by 13.27%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #5364       +/-   ##
===========================================
- Coverage   93.54%   80.27%   -13.28%     
===========================================
  Files         332       47      -285     
  Lines       16125     1171    -14954     
  Branches      373        0      -373     
===========================================
- Hits        15084      940    -14144     
+ Misses       1041      231      -810

Impacted Files	Coverage Δ
packages/fxa-profile-server/lib/routes/profile.js	`89.18% <0.00%> (ø)`
...bulk-mailer/nodemailer-mocks/write-to-disk-mock.js
...bulk-mailer/nodemailer-mocks/stream-output-mock.js
...xa-admin-panel/src/components/LogoLockup/index.tsx
packages/fxa-auth-server/lib/routes/totp.js
...kages/fxa-auth-server/lib/oauth/routes/key_data.js
...payments-server/src/components/AppLayout/index.tsx
packages/fxa-auth-server/lib/routes/utils/otp.js
packages/fxa-auth-server/lib/routes/idp.js
...server/lib/oauth/routes/authorized-clients/list.js
... and 245 more

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 9b2d9d1...c5ab58b. Read the comment docs.

rfk · 2020-05-19T23:25:17Z

I'll take a more detailed look, but this seems broadly as I expected 👍

Could we please also include this field in the response from GET /profile so that it can be fetched along with other data in a single operation? (It may be that we get this for free by virtue of having it in _core_profile, but the docs don't indicate this to be the case).

rfk · 2020-05-19T23:27:17Z

packages/fxa-profile-server/lib/routes/ecosystem_anon_id/post.js

+      ecosystemAnonId: Joi.string().required(),
+    },
+  },
+  handler: async function ecosystemAnonIdPost(req) {


Ideally, clients would be able to If-Match or If-Unmodified-Since on this post, for the case when we're filling in a placeholder anon id and we don't want two clients to race in setting it.

Sounds good. I've added a note to #4325, since I can't test the ETag is checked against the If-Match value until the DB returns an actual response.

rfk · 2020-05-19T23:27:54Z

To clarify, is the idea that we'll land and ship just the API surface in this bug, and plumb in the actual storing in a followup?

jaredhirsch · 2020-05-20T00:40:57Z

To clarify, is the idea that we'll land and ship just the API surface in this bug, and plumb in the actual storing in a followup?

Yes, exactly. I'm hoping to unblock desktop client work by starting with the API, then I'll work downwards.

jaredhirsch · 2020-05-20T00:41:48Z

Thanks for taking a look! I'll fixup the bits you mentioned tomorrow morning and ping you for a followup 👍

jaredhirsch · 2020-05-20T21:46:36Z

Could we please also include this field in the response from GET /profile so that it can be fetched along with other data in a single operation? (It may be that we get this for free by virtue of having it in _core_profile, but the docs don't indicate this to be the case).

I think this should work now, as I've added ecosystemAnonId as an optional field in the response schema for GET /profile. It appears that GET /profile pulls data from lib/profileCache, which in turn aggregates data from several routes: _core_profile, plus profile-specific routes like avatar and displayName. I'll double-check that turns out to be true when I get eco_anon_id fully plumbed through to the auth DB, but I think we're good.

vbudhram

@6a68 Thanks, this LGTM 👍

vbudhram · 2020-05-21T14:40:38Z

packages/fxa-profile-server/docs/API.md

+}
+```
+
+Returns a `204 No Content` if there is no ecosystem_anon_id for the user.


I'm cool with this, I've seen other endpoints return 200 and an empty response.

vbudhram · 2020-05-21T14:40:38Z

packages/fxa-profile-server/lib/routes/profile.js

@@ -23,6 +23,7 @@ module.exports = {
  },
  response: {
    schema: {
+      ecosystemAnonId: Joi.string().optional(),


nit: Update docs

vbudhram · 2020-05-21T14:40:38Z

packages/fxa-profile-server/test/api.js

+    var tok = token();
+
+    describe('GET', function() {
+      xit('should return an ecosystem_anon_id', function() {});


vbudhram · 2020-05-21T14:40:38Z

packages/fxa-profile-server/test/api.js

@@ -765,6 +767,28 @@ describe('api', function() {
          assertSecurityHeaders(res);
        });
    });
+
+    it('should omit `ecosystem_anon_id` if not returned by auth-server', () => {


In the auth-server I've been slowly updating the test code to use async/await since it makes things a little easier to read.

oh, nice! yeah, I was just trying to follow the existing style in the files, but I'll start converting to async/await 👍

vladikoff · 2020-05-21T14:41:23Z

packages/fxa-profile-server/lib/routes/ecosystem_anon_id/post.js

+    const uid = req.auth.credentials.user;
+    logger.info('activityEvent', { event: 'ecosystemAnonId.post', uid: uid });
+
+    return req.server.methods.profileCache.drop(uid).then(() => {


could this just use await ? We've been slowly converting the routes

yeah, definitely

rfk

A belated r+ here, but thanks for pushing this through first!

The ecosystem_anon_id work for AET (Jira epic FXA-880) started from the API used by desktop code, in order to unblock that work, which can be done in parallel. This new identifier is eventually going to be part of the core profile fields stored in the auth server DB, which means there will be changes in mysql, auth-db, auth-server, and profile-server. The initial no-op profile-server API implementation landed as #5364. This change moves one step further down the stack, updating the profile-server API handler from a no-op to an actual call into the _core_profile, and adding a deceptively simple change to the auth server that allows an ecosystemAnonId field to be passed through from auth-db. Future commits will integrate this new field into the auth-db and eventually into mysql.

jaredhirsch requested a review from rfk May 19, 2020

data-sync-user mentioned this pull request May 19, 2020

Update profile server to accept ecosystem_anon_id field #4324

Closed

rfk reviewed May 19, 2020

View changes

data-sync-user mentioned this pull request May 20, 2020

Add ecosystem_anon_id field to core profile data returned by auth server #4325

Closed

jaredhirsch requested a review from rfk May 20, 2020

vbudhram approved these changes May 21, 2020

View changes

vladikoff reviewed May 21, 2020

View changes

jaredhirsch force-pushed the fxa-1234 branch from 615c1b6 to 69b4c6a May 21, 2020

feat(profile): Add API endpoints for ecosystem_anon_id

c5ab58b

jaredhirsch force-pushed the fxa-1234 branch from 69b4c6a to c5ab58b May 21, 2020

jaredhirsch deleted the fxa-1234 branch May 21, 2020

rfk reviewed May 22, 2020

View changes

jaredhirsch mentioned this pull request May 27, 2020

feat(aet): add ecosystem_anon_id field to auth server core profile data #5481

Merged

mozilla / fxa

feat(profile): Add API endpoints for ecosystem_anon_id #5364

feat(profile): Add API endpoints for ecosystem_anon_id #5364

jaredhirsch commented May 19, 2020

codecov-commenter commented May 19, 2020 •

edited

rfk commented May 19, 2020

This comment has been minimized.

This comment has been minimized.

rfk commented May 19, 2020

jaredhirsch commented May 20, 2020

jaredhirsch commented May 20, 2020

jaredhirsch commented May 20, 2020

vbudhram left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rfk left a comment

mozilla / fxa

feat(profile): Add API endpoints for ecosystem_anon_id #5364

feat(profile): Add API endpoints for ecosystem_anon_id #5364

Conversation

jaredhirsch commented May 19, 2020

codecov-commenter commented May 19, 2020 • edited

Codecov Report

rfk commented May 19, 2020

This comment has been minimized.

rfk May 19, 2020 Member

This comment has been minimized.

jaredhirsch May 20, 2020 Author Member

rfk commented May 19, 2020

jaredhirsch commented May 20, 2020

jaredhirsch commented May 20, 2020

jaredhirsch commented May 20, 2020

vbudhram left a comment

This comment has been minimized.

vbudhram May 21, 2020 Contributor

This comment has been minimized.

vbudhram May 21, 2020 Contributor

This comment has been minimized.

vbudhram May 21, 2020 Contributor

This comment has been minimized.

vbudhram May 21, 2020 Contributor

This comment has been minimized.

jaredhirsch May 21, 2020 Author Member

This comment has been minimized.

vladikoff May 21, 2020 Contributor

This comment has been minimized.

jaredhirsch May 21, 2020 Author Member

rfk left a comment

codecov-commenter commented May 19, 2020 •

edited

rfk May 19, 2020
Member

jaredhirsch May 20, 2020
Author Member

vbudhram May 21, 2020
Contributor

vbudhram May 21, 2020
Contributor

vbudhram May 21, 2020
Contributor

vbudhram May 21, 2020
Contributor

jaredhirsch May 21, 2020
Author Member

vladikoff May 21, 2020
Contributor

jaredhirsch May 21, 2020
Author Member