Skip to content

[server] update OIDC users on sign-in (email) – WEB-370 #17633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 22, 2023
Merged

[server] update OIDC users on sign-in (email) – WEB-370 #17633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3458db
[server] update OIDC users on sign-in (email)
AlexTugarev May 16, 2023
97e881f
adding simple tests for update of email attribute
AlexTugarev May 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 61 additions & 3 deletions components/server/src/iam/iam-session-app.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import * as request from "supertest";
import * as chai from "chai";
import { OIDCCreateSessionPayload } from "./iam-oidc-create-session-payload";
import { BUILTIN_INSTLLATION_ADMIN_USER_ID, TeamDB } from "@gitpod/gitpod-db/lib";
import { TeamMemberInfo } from "@gitpod/gitpod-protocol";
import { TeamMemberInfo, User } from "@gitpod/gitpod-protocol";
const expect = chai.expect;

@suite(timeout(10000))
Expand All @@ -33,20 +33,25 @@ class TestIamSessionApp {
protected knownSubjectID = "111";
protected knownEmail = "[email protected]";

protected knownUser: Partial<User> = {
id: "id-known-user",
identities: [],
};

protected userServiceMock: Partial<UserService> = {
createUser: (params) => {
return { id: "id-new-user" } as any;
},

findUserForLogin: async (params) => {
if (params.candidate?.authId === this.knownSubjectID) {
return { id: "id-known-user" } as any;
return this.knownUser as any;
}
return undefined;
},
findOrgOwnedUser: async (params) => {
if (params.email === this.knownEmail) {
return { id: "id-known-user" } as any;
return this.knownUser as any;
}
return undefined;
},
Expand Down Expand Up @@ -88,6 +93,7 @@ class TestIamSessionApp {

public before() {
this.teamDbMock.memberships.clear();
this.knownUser.identities = [];

const container = new Container();
container.load(
Expand Down Expand Up @@ -231,6 +237,58 @@ class TestIamSessionApp {
expect(this.teamDbMock.memberships.has(BUILTIN_INSTLLATION_ADMIN_USER_ID)).to.be.false;
expect(this.teamDbMock.memberships.has("id-new-user")).to.be.true;
}

@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));

Comment on lines +241 to +263
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, if we make it a db-test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this way it makes assumptions about the internal paths of the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(result.statusCode, JSON.stringify(result.body)).to.equal(200);
expect(newEmail, "update was not called").not.to.be.undefined;
expect(newEmail).to.equal(payload.claims.email);
Comment on lines +265 to +266
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first check seems redundant, given we also do the second comparison, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First is checking if the update is actually called, second is checking if the new value is as expected.

}

@test public async testSessionRequest_no_update_if_same_email() {
this.knownUser.identities = [
{
authId: this.payload.claims.sub,
authProviderId: this.payload.claims.aud,
authName: "Test User",
primaryEmail: this.payload.claims.email,
},
];

let updateUserIdentityCalled = false;
this.userServiceMock.updateUserIdentity = async () => {
updateUserIdentityCalled = true;
};

const result = await request(this.app.create())
.post("/session")
.set("Content-Type", "application/json")
.send(JSON.stringify(this.payload));

expect(result.statusCode, JSON.stringify(result.body)).to.equal(200);
expect(updateUserIdentityCalled).to.be.false;
}
}

module.exports = new TestIamSessionApp();
18 changes: 18 additions & 0 deletions components/server/src/iam/iam-session-app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ export class IamSessionApp {
const payload = OIDCCreateSessionPayload.validate(req.body);

const existingUser = await this.findExistingOIDCUser(payload);
if (existingUser) {
await this.updateOIDCUserOnSign(existingUser, payload);
}

const user = existingUser || (await this.createNewOIDCUser(payload));

await new Promise<void>((resolve, reject) => {
Expand Down Expand Up @@ -138,6 +142,20 @@ export class IamSessionApp {
return existingUser;
}

/**
* Updates `User.identities[current IdP].primaryEmail`
*/
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);
}
}
Comment on lines +148 to +157
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


protected async createNewOIDCUser(payload: OIDCCreateSessionPayload): Promise<User> {
const { claims, organizationId } = payload;

Expand Down
6 changes: 3 additions & 3 deletions components/server/src/user/user-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,20 +302,20 @@ export class UserService {
user.name = user.name || authUser.name || authUser.primaryEmail;
user.avatarUrl = user.avatarUrl || authUser.avatarUrl;
await this.onAfterUserLoad(user);
await this.updateUserIdentity(user, candidate, token);
await this.updateUserIdentity(user, candidate);
await this.userDb.storeSingleToken(candidate, token);
}

async onAfterUserLoad(user: User): Promise<User> {
return user;
}

async updateUserIdentity(user: User, candidate: Identity, token: Token) {
async updateUserIdentity(user: User, candidate: Identity) {
// ensure single identity per auth provider instance
user.identities = user.identities.filter((i) => i.authProviderId !== candidate.authProviderId);
user.identities.push(candidate);

await this.userDb.storeUser(user);
await this.userDb.storeSingleToken(candidate, token);
}

async deauthorize(user: User, authProviderId: string) {
Expand Down