[server] update OIDC users on sign-in (email) – WEB-370 #17633
Conversation
| expect(newEmail, "update was not called").not.to.be.undefined; | ||
| expect(newEmail).to.equal(payload.claims.email); |
There was a problem hiding this comment.
The first check seems redundant, given we also do the second comparison, no?
There was a problem hiding this comment.
First is checking if the update is actually called, second is checking if the new value is as expected.
| @test public async testSessionRequest_updates_existing_user() { | ||
| const payload: OIDCCreateSessionPayload = { ...this.payload }; | ||
| payload.claims.sub = this.knownSubjectID; // `userServiceMock.findUserForLogin` will match this value | ||
|
|
||
| this.knownUser.identities = [ | ||
| { | ||
| authId: payload.claims.sub, | ||
| authProviderId: payload.claims.aud, | ||
| authName: "Test User", | ||
| primaryEmail: "[email protected]", | ||
| }, | ||
| ]; | ||
|
|
||
| let newEmail: string | undefined; | ||
| this.userServiceMock.updateUserIdentity = async (user, updatedIdentity) => { | ||
| newEmail = updatedIdentity.primaryEmail; | ||
| }; | ||
|
|
||
| const result = await request(this.app.create()) | ||
| .post("/session") | ||
| .set("Content-Type", "application/json") | ||
| .send(JSON.stringify(payload)); | ||
|
|
There was a problem hiding this comment.
It feels this whole test is quite brittle. We're mostly testing that once you hit /session with some payload, the handler invokes the userServiceMock.updateUserIdentity method which is in fact a stub. So here, we're just checking that it invokes our stub, rather than it actually performs the behaviour we want.
Is it possible to instead use the real service and verify the record got updated in the DB?
There was a problem hiding this comment.
It is, if we make it a db-test.
There was a problem hiding this comment.
Yes, this way it makes assumptions about the internal paths of the code.
There was a problem hiding this comment.
Up to you. I'd personally favour it as a db test as it gives us stronger guarantees but if you think it's pointless then I can live with that.
| protected async updateOIDCUserOnSign(user: User, payload: OIDCCreateSessionPayload) { | ||
| const recent = this.mapOIDCProfileToIdentity(payload); | ||
| const existing = user.identities.find((identity) => identity.authId === recent.authId); | ||
|
|
||
| // Update email | ||
| if (existing && !!recent.primaryEmail && existing.primaryEmail !== recent.primaryEmail) { | ||
| existing.primaryEmail = recent.primaryEmail; | ||
| await this.userService.updateUserIdentity(user, existing); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this whole method be moved into the userService such that it can be more easily re-used in other places? This would be particularly useful as we try to align with the service oriented architecture.
There was a problem hiding this comment.
Thought about that, but opted to not push the mapping in there.
See, userService.updateUserIdentity does the actual update. On top, here is the mapping of the profiles, which very well could be pulled out to be reusable.
|
/unhold |
roboquat
added
deployed: webapp
Updates email address of SSO users on each sign-in.
Related Issue(s)
Fixes WEB-370
How to test
Documentation
Preview status
gitpod:summary
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh/hold