[#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations #92

AleDore · 2020-10-12T16:32:14Z

This PR is intended to manage ADB2C custom attribute Token Name, in order to allow existing CreateUser and the brand new UpdateUser to manage this custom attribute.

pagopa-github-bot · 2020-10-12T16:33:23Z

Affected stories

🌟 #175227073: Come ADM, vorrei poter settare l' attributo Token Name all' interno degli attributi custom di B2C programmaticamente.

Generated by 🚫 dangerJS

francescopersico · 2020-10-13T07:06:22Z

When you update a user, setting a different token_name, what happens to already created service? The token_name in the services remains the old one. Can it be a problem?

gunzip · 2020-10-13T08:16:11Z

The ratio here is that attributes on user account are default values for newly create services, so I think it's not a problem if the old services maintain the old token name (it must be documented)

AleDore · 2020-10-13T09:02:27Z

When you update a user, setting a different token_name, what happens to already created service? The token_name in the services remains the old one. Can it be a problem?

Since this operation will be performed only by administrators, I think it will be used only for newly accounts or on existing ones, that will be enabled after the test's phase. It shouldn't be a problem if old services maintain the old token name.

balanza · 2020-10-13T08:31:33Z

UpdateUser/handler.ts

+        internalErrorHandler("Could not get the ADB2C client", error)
+      )
+      .chain(graphRbacManagementClient =>
+        getUserFromList(graphRbacManagementClient, userPayload.email)


Should we decode userPayload? It's an input, it can be anything

Yep, userPayload is validated here

sorry didn't see that

AleDore

Can we merge this? @francescopersico @gunzip ?

AleDore · 2020-10-13T13:14:15Z

I' ve checked that on updateUser operation, we expect first_name and last_name in the userPayload, but I think that the only information we have about the user to update is email isn't it? cc: @gunzip @francescopersico

gunzip · 2020-10-13T14:11:30Z

I think updateUser should work in the same way as the other update methods (ie. UpdateService), retrieving the old user properties and merging the overridden values, what about it?

AleDore · 2020-10-13T15:49:35Z

I think updateUser should work in the same way as the other update methods (ie. UpdateService), retrieving the old user properties and merging the overridden values, what about it?

Nope because graph API expect a payload containing only the attributes to be updated (so other attributes are not overwritten if not present ), but the question is: Do we always have informations about first_name and last_name while performing UpdateUser operation ? I think that the only information we got in that case is the email associated with the B2C account, so first_name and last_name should be optional in the userPayload used for Update. What do you think?

gunzip · 2020-10-13T20:50:16Z

that's right, maybe you can use t.partial(User)for the decoder

AleDore · 2020-10-14T11:23:51Z

that's right, maybe you can use t.partial(User)for the decoder

Maybe we can define a payload for UpdateUser that has all possible attributes optional.

gunzip · 2020-10-14T12:56:31Z

UpdateUser/handler.ts

+        first_name: userPayload.first_name
+          ? userPayload.first_name
+          : user.givenName,
+        id: updateUserResponse.objectId,
+        last_name: userPayload.last_name ? userPayload.last_name : user.surname,


why this mapping here? can't we just return the exact values ?
first_name -> first_name, last_name -> last_name

UserCreated has first_name and last_name as required fields. Since userPayload has the same optional attributes, we can map over user attributes as a fallback option.

we can make it shorter like

first_name: userPayload.first_name || user.givenName

less cognitive load imho

why do we need the fallback? moreover, it prevent the caller to check if the values are undefined

why do we need the fallback? moreover, it prevent the caller to check if the values are undefined

Because if the caller doesn't provide first_name or last_name (namely it doesn't want to update first_name or last_name), we must provide the same attributes on UserCreated response because they are required. I think that B2C user always has this attributes ('cause createUser expects them in the request payload as required fields)

fallback is not a good idea (it's an arbitrary mapping just to fill value to satisfy the decoder). if we must return these values then they must be retrieved from the original user record otherwise we should relax the constraints on these fields

Ok let's create a UserUpdated instead of UserCreated definition that allows direct mapping from userPayload

balanza · 2020-10-14T13:28:31Z

UpdateUser/handler.ts

+        first_name: userPayload.first_name
+          ? userPayload.first_name
+          : user.givenName,
+        id: updateUserResponse.objectId,
+        last_name: userPayload.last_name ? userPayload.last_name : user.surname,


we can make it shorter like

first_name: userPayload.first_name || user.givenName

less cognitive load imho

balanza · 2020-10-14T13:29:38Z

UpdateUser/handler.ts

+        id: updateUserResponse.objectId,
+        last_name: userPayload.last_name ? userPayload.last_name : user.surname,
+        token_name: userPayload.token_name
+      } as UserCreated)


Instead of this manual cast, should we use a codec?

balanza · 2020-10-14T15:03:59Z

UpdateUser/handler.ts

+  ).chain(updateUserResponse =>
+    UserCreated.decode({
+      email,
+      first_name: userPayload.first_name || user.givenName,
+      id: updateUserResponse.objectId,
+      last_name: userPayload.last_name || user.surname,
+      token_name: userPayload.token_name
+    }).fold(errs => fromLeft(toError(errs)), usr => taskEither.of(usr))


what about

Suggested change

).chain(updateUserResponse =>

UserCreated.decode({

email,

first_name: userPayload.first_name || user.givenName,

id: updateUserResponse.objectId,

last_name: userPayload.last_name || user.surname,

token_name: userPayload.token_name

}).fold(errs => fromLeft(toError(errs)), usr => taskEither.of(usr))

).chain(updateUserResponse =>

fromEither(UserCreated.decode({

email,

first_name: userPayload.first_name || user.givenName,

id: updateUserResponse.objectId,

last_name: userPayload.last_name || user.surname,

token_name: userPayload.token_name

})).mapLeft(toError)

balanza · 2020-10-14T15:07:47Z

UpdateUser/handler.ts

+import { fromLeft } from "fp-ts/lib/TaskEither";
+import { taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";


If you accept that suggestion you'll need this, too

Suggested change

import { fromLeft } from "fp-ts/lib/TaskEither";

import { taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

import { fromEither, taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

My bad, you' re right 😄

codecov-io · 2020-10-14T15:15:46Z

Codecov Report

Merging #92 into master will decrease coverage by 0.97%.
The diff coverage is 69.64%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   83.94%   82.96%   -0.98%     
==========================================
  Files          44       48       +4     
  Lines        1489     1603     +114     
  Branches      124      128       +4     
==========================================
+ Hits         1250     1330      +80     
- Misses        234      268      +34     
  Partials        5        5

Impacted Files	Coverage Δ
utils/healthcheck.ts	`41.02% <41.02%> (ø)`
CreateUser/handler.ts	`87.17% <75.00%> (+0.33%)`	⬆️
utils/config.ts	`75.00% <75.00%> (ø)`
Info/handler.ts	`83.33% <85.71%> (ø)`
UpdateUser/handler.ts	`89.13% <89.13%> (ø)`

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 33442c2...c56eee6. Read the comment docs.

balanza · 2020-10-14T15:44:33Z

UpdateUser/handler.ts

+import { fromEither } from "fp-ts/lib/TaskEither";
+import { TaskEither, tryCatch } from "fp-ts/lib/TaskEither";


no need to import twice

Suggested change

import { fromEither } from "fp-ts/lib/TaskEither";

import { TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

import { fromEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

It' s so strange that lint doesn't report a non grouped import 😄

AleDore · 2020-10-15T10:02:30Z

Can we merge this? @balanza @BurnedMarshal

BurnedMarshal · 2020-10-15T10:56:27Z

UpdateUser/__tests__/handler.ts

+mockGetToken.mockImplementation(() => {
+  return Promise.resolve(undefined);
+});
+const mockUsersCreate = jest.fn();


Could be this mockUsersUpdate?

Fixed in 2948303

BurnedMarshal · 2020-10-15T11:04:21Z

UpdateUser/__tests__/handler.ts

+      fakeRequestPayload
+    );
+
+    expect(response.kind).toEqual("IResponseErrorInternal");


Can we relate the behaviour of this test with the test conditions?
For example

Suggested change

expect(response.kind).toEqual("IResponseErrorInternal");

expect(mockUsersCreate).toBeCalledTimes(1);

expect(response).toEqual({

apply: expect.any(Function),

detail: expectedError,

kind: "IResponseErrorInternal"

});

Fixed in 2948303

BurnedMarshal

lgtm

AleDore added 2 commits October 12, 2020 18:28

[#175227073] Add UpdateUser &refactoring on Create

a82f342

[#175227073] remove config

f5dc43a

AleDore requested review from balanza, BurnedMarshal, francescopersico and gunzip as code owners October 12, 2020 16:32

AleDore changed the title ~~[#175227073] Allow updating ADB2C User and introduce Token Name management~~ [#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations Oct 12, 2020

balanza reviewed Oct 13, 2020

View reviewed changes

AleDore commented Oct 13, 2020

View reviewed changes

[#175227073] Update User Refactoring

fcaa6c4

gunzip reviewed Oct 14, 2020

View reviewed changes

balanza reviewed Oct 14, 2020

View reviewed changes

[#175227073] Refactor over review

924224c

balanza reviewed Oct 14, 2020

View reviewed changes

[#175227073] Minor refactor due to review

c56eee6

AleDore added 2 commits October 14, 2020 17:38

[#175227073] insert UserUpdated definition

a174b87

[#175227073] fix operation return type

1ec5272

AleDore requested a review from gunzip October 14, 2020 15:41

balanza reviewed Oct 14, 2020

View reviewed changes

[#175227073] Fix Non grouped import

8407bcb

AleDore requested a review from balanza October 14, 2020 16:04

balanza approved these changes Oct 15, 2020

View reviewed changes

BurnedMarshal reviewed Oct 15, 2020

View reviewed changes

[#175227073] Refactor on test conditions

2948303

AleDore requested a review from BurnedMarshal October 15, 2020 15:25

BurnedMarshal approved these changes Oct 15, 2020

View reviewed changes

AleDore merged commit 6b7b7dc into master Oct 15, 2020

AleDore deleted the 175227073_update_b2c_user_and_token_name_mgmt branch October 15, 2020 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations #92

[#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations #92

AleDore commented Oct 12, 2020

pagopa-github-bot commented Oct 12, 2020 •

edited

Loading

francescopersico commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 13, 2020

balanza Oct 13, 2020

AleDore Oct 13, 2020

balanza Oct 13, 2020

AleDore left a comment

AleDore commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 14, 2020

gunzip Oct 14, 2020

AleDore Oct 14, 2020

balanza Oct 14, 2020

gunzip Oct 14, 2020 •

edited

Loading

AleDore Oct 14, 2020

gunzip Oct 14, 2020

AleDore Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020

balanza Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020

AleDore Oct 14, 2020

codecov-io commented Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020

AleDore Oct 14, 2020

AleDore commented Oct 15, 2020

BurnedMarshal Oct 15, 2020

AleDore Oct 15, 2020

BurnedMarshal Oct 15, 2020

AleDore Oct 15, 2020

BurnedMarshal left a comment

		import { fromLeft } from "fp-ts/lib/TaskEither";
		import { taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

	import { fromLeft } from "fp-ts/lib/TaskEither";
	import { taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";
	import { fromEither, taskEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

		import { fromEither } from "fp-ts/lib/TaskEither";
		import { TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

	import { fromEither } from "fp-ts/lib/TaskEither";
	import { TaskEither, tryCatch } from "fp-ts/lib/TaskEither";
	import { fromEither, TaskEither, tryCatch } from "fp-ts/lib/TaskEither";

-    expect(response.kind).toEqual("IResponseErrorInternal");
+    expect(mockUsersCreate).toBeCalledTimes(1);
+    expect(response).toEqual({
+      apply: expect.any(Function),
+      detail: expectedError,
+      kind: "IResponseErrorInternal"
+    });

[#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations #92

[#175227073] Allow updating ADB2C User and introduce Token Name management on Create/Update operations #92

Conversation

AleDore commented Oct 12, 2020

pagopa-github-bot commented Oct 12, 2020 • edited Loading

Affected stories

francescopersico commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleDore left a comment

Choose a reason for hiding this comment

AleDore commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 13, 2020

gunzip commented Oct 13, 2020

AleDore commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunzip Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleDore Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balanza Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

balanza Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleDore commented Oct 15, 2020

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

BurnedMarshal left a comment

Choose a reason for hiding this comment

pagopa-github-bot commented Oct 12, 2020 •

edited

Loading

gunzip Oct 14, 2020 •

edited

Loading

AleDore Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020 •

edited

Loading

balanza Oct 14, 2020 •

edited

Loading

codecov-io commented Oct 14, 2020 •

edited

Loading