Skip to content

Commit 0e19a2b

Browse files
AlexTugarevAlexTugarev
committed
wip: use redacted results in service
1 parent 42b51f3 commit 0e19a2b

File tree

4 files changed

+81
-40
lines changed

4 files changed

+81
-40
lines changed

‎components/server/src/auth/auth-provider-service.spec.db.ts

Lines changed: 43 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -37,24 +37,29 @@ describe("AuthProviderService", async () => {
3737
clientId: "123",
3838
clientSecret: "secret-123",
3939
};
40-
const expectedProviderPartial = () =>
41-
<Partial<AuthProviderParams>>{
42-
builtin: false,
40+
const expectedEntry = () =>
41+
<Partial<AuthProviderEntry>>{
4342
host: "github.com",
44-
disallowLogin: false,
4543
oauth: {
4644
authorizationUrl: "https://github.com/login/oauth/authorize",
4745
callBackUrl: "https://gitpod.io/auth/callback",
4846
clientId: "123",
49-
clientSecret: "secret-123",
47+
clientSecret: "redacted",
5048
tokenUrl: "https://github.com/login/oauth/access_token",
5149
},
5250
organizationId: undefined,
5351
type: "GitHub",
54-
verified: false,
5552
status: "pending",
5653
ownerId: owner.id,
5754
};
55+
const expectedParams = () =>
56+
<Partial<AuthProviderParams>>{
57+
builtin: false,
58+
disallowLogin: false,
59+
verified: false,
60+
...expectedEntry(),
61+
oauth: { ...expectedEntry().oauth, clientSecret: "secret-123" },
62+
};
5863

5964
const newOrgEntry = () =>
6065
<AuthProviderEntry.NewOrgEntry>{
@@ -65,24 +70,29 @@ describe("AuthProviderService", async () => {
6570
clientSecret: "secret-123",
6671
organizationId: org.id,
6772
};
68-
const expectedOrgProviderPartial = () =>
69-
<Partial<AuthProviderParams>>{
70-
builtin: false,
73+
const expectedOrgEntry = () =>
74+
<Partial<AuthProviderEntry>>{
7175
host: "github.com",
72-
disallowLogin: true,
7376
oauth: {
7477
authorizationUrl: "https://github.com/login/oauth/authorize",
7578
callBackUrl: "https://gitpod.io/auth/callback",
7679
clientId: "123",
77-
clientSecret: "secret-123",
80+
clientSecret: "redacted",
7881
tokenUrl: "https://github.com/login/oauth/access_token",
7982
},
8083
organizationId: org.id,
8184
type: "GitHub",
82-
verified: false,
8385
status: "pending",
8486
ownerId: owner.id,
8587
};
88+
const expectedOrgParams = () =>
89+
<Partial<AuthProviderParams>>{
90+
builtin: false,
91+
disallowLogin: true,
92+
verified: false,
93+
...expectedOrgEntry(),
94+
oauth: { ...expectedOrgEntry().oauth, clientSecret: "secret-123" },
95+
};
8696

8797
beforeEach(async () => {
8898
container = createTestContainer();
@@ -109,14 +119,14 @@ describe("AuthProviderService", async () => {
109119

110120
describe("createAuthProviderOfUser", async () => {
111121
it("should create user-level provider", async () => {
112-
const providersAtStart = await service.getAllAuthProviders();
122+
const providersAtStart = await service.getAllAuthProviderParams();
113123
expect(providersAtStart).to.be.empty;
114124

115125
await service.createAuthProviderOfUser(owner.id, newEntry());
116126

117-
const providers = await service.getAllAuthProviders();
127+
const providers = await service.getAllAuthProviderParams();
118128
expect(providers).to.have.lengthOf(1);
119-
expect(providers[0]).to.deep.include(expectedProviderPartial());
129+
expect(providers[0]).to.deep.include(expectedParams());
120130
});
121131

122132
it("should fail in case of conflict with built-in provider", async () => {
@@ -127,7 +137,7 @@ describe("AuthProviderService", async () => {
127137
host: "github.com",
128138
} as any);
129139

130-
const providersAtStart = await service.getAllAuthProviders();
140+
const providersAtStart = await service.getAllAuthProviderParams();
131141
expect(providersAtStart).to.be.empty;
132142

133143
await expectError(ErrorCodes.CONFLICT, service.createAuthProviderOfUser(owner.id, newEntry()));
@@ -142,7 +152,7 @@ describe("AuthProviderService", async () => {
142152
);
143153
});
144154
it("should fail if trying to register same host", async () => {
145-
const providersAtStart = await service.getAllAuthProviders();
155+
const providersAtStart = await service.getAllAuthProviderParams();
146156
expect(providersAtStart).to.be.empty;
147157

148158
await service.createAuthProviderOfUser(owner.id, newEntry());
@@ -153,14 +163,14 @@ describe("AuthProviderService", async () => {
153163

154164
describe("createOrgAuthProvider", async () => {
155165
it("should create org-level provider", async () => {
156-
const providersAtStart = await service.getAllAuthProviders();
166+
const providersAtStart = await service.getAllAuthProviderParams();
157167
expect(providersAtStart).to.be.empty;
158168

159169
await service.createOrgAuthProvider(owner.id, newOrgEntry());
160170

161-
const providers = await service.getAllAuthProviders();
171+
const providers = await service.getAllAuthProviderParams();
162172
expect(providers).to.have.lengthOf(1);
163-
expect(providers[0]).to.deep.include(expectedOrgProviderPartial());
173+
expect(providers[0]).to.deep.include(expectedOrgParams());
164174
});
165175
it("should fail if host is not reachable", async () => {
166176
await expectError(
@@ -172,12 +182,24 @@ describe("AuthProviderService", async () => {
172182
);
173183
});
174184
it("should fail if trying to register same host", async () => {
175-
const providersAtStart = await service.getAllAuthProviders();
185+
const providersAtStart = await service.getAllAuthProviderParams();
176186
expect(providersAtStart).to.be.empty;
177187

178188
await service.createOrgAuthProvider(owner.id, newOrgEntry());
179189

180190
await expectError(ErrorCodes.CONFLICT, service.createAuthProviderOfUser(owner.id, newOrgEntry()));
181191
});
182192
});
193+
describe("getAuthProvider", async () => {
194+
it("should find org-level provider", async () => {
195+
const providersAtStart = await service.getAllAuthProviderParams();
196+
expect(providersAtStart).to.be.empty;
197+
198+
const created = await service.createOrgAuthProvider(owner.id, newOrgEntry());
199+
200+
const retrieved = await service.getAuthProvider(owner.id, created.id);
201+
console.log(JSON.stringify(retrieved));
202+
expect(retrieved).to.deep.include(expectedOrgEntry());
203+
});
204+
});
183205
});

‎components/server/src/auth/auth-provider-service.ts

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,12 @@ export class AuthProviderService {
3333
) {}
3434

3535
/**
36-
* Returns all auth providers.
36+
* Returns all **unredacted** auth provider params to be used in the internal
37+
* authenticator parts.
38+
*
39+
* Known internal client `HostContextProviderImpl`
3740
*/
38-
async getAllAuthProviders(exceptOAuthRevisions: string[] = []): Promise<AuthProviderParams[]> {
41+
async getAllAuthProviderParams(exceptOAuthRevisions: string[] = []): Promise<AuthProviderParams[]> {
3942
const all = await this.authProviderDB.findAll(exceptOAuthRevisions);
4043
return all.map((provider) => this.toAuthProviderParams(provider));
4144
}
@@ -68,7 +71,7 @@ export class AuthProviderService {
6871
async getAuthProviderDescriptionsUnauthenticated(): Promise<AuthProviderInfo[]> {
6972
const { builtinAuthProvidersConfigured } = this.config;
7073

71-
const authProviders = [...(await this.getAllAuthProviders()), ...this.config.authProviderConfigs];
74+
const authProviders = [...(await this.getAllAuthProviderParams()), ...this.config.authProviderConfigs];
7275

7376
const toPublic = (ap: AuthProviderParams) =>
7477
<AuthProviderInfo>{
@@ -89,7 +92,7 @@ export class AuthProviderService {
8992
async getAuthProviderDescriptions(user: User): Promise<AuthProviderInfo[]> {
9093
const { builtinAuthProvidersConfigured } = this.config;
9194

92-
const authProviders = [...(await this.getAllAuthProviders()), ...this.config.authProviderConfigs];
95+
const authProviders = [...(await this.getAllAuthProviderParams()), ...this.config.authProviderConfigs];
9396

9497
// explicitly copy to avoid bleeding sensitive details
9598
const toInfo = (ap: AuthProviderParams) =>
@@ -180,13 +183,13 @@ export class AuthProviderService {
180183
await this.auth.checkPermissionOnUser(userId, "read_info", userId);
181184

182185
const result = await this.authProviderDB.findByUserId(userId);
183-
return result;
186+
return result.map((ap) => AuthProviderEntry.redact(ap));
184187
}
185188

186189
async getAuthProvidersOfOrg(userId: string, organizationId: string): Promise<AuthProviderEntry[]> {
187190
await this.auth.checkPermissionOnOrganization(userId, "read_git_provider", organizationId);
188191
const result = await this.authProviderDB.findByOrgId(organizationId);
189-
return result;
192+
return result.map((ap) => AuthProviderEntry.redact(ap));
190193
}
191194

192195
async deleteAuthProviderOfUser(userId: string, authProviderId: string): Promise<void> {
@@ -230,7 +233,7 @@ export class AuthProviderService {
230233
await this.auth.checkPermissionOnUser(userId, "read_info", userId);
231234
}
232235

233-
return result;
236+
return AuthProviderEntry.redact(result);
234237
}
235238

236239
async createAuthProviderOfUser(userId: string, entry: AuthProviderEntry.NewEntry): Promise<AuthProviderEntry> {
@@ -257,7 +260,8 @@ export class AuthProviderService {
257260
}
258261

259262
const authProvider = this.initializeNewProvider(entry);
260-
return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
263+
const result = await this.authProviderDB.storeAuthProvider(authProvider, true);
264+
return AuthProviderEntry.redact(result);
261265
}
262266

263267
private isBuiltInProvider(host: string) {
@@ -291,7 +295,8 @@ export class AuthProviderService {
291295
oauth,
292296
status: "pending",
293297
};
294-
return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
298+
const result = await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
299+
return AuthProviderEntry.redact(result);
295300
}
296301

297302
async createOrgAuthProvider(userId: string, newEntry: AuthProviderEntry.NewOrgEntry): Promise<AuthProviderEntry> {
@@ -319,7 +324,8 @@ export class AuthProviderService {
319324
}
320325

321326
const authProvider = this.initializeNewProvider(newEntry);
322-
return await this.authProviderDB.storeAuthProvider(authProvider, true);
327+
const result = await this.authProviderDB.storeAuthProvider(authProvider, true);
328+
return AuthProviderEntry.redact(result);
323329
}
324330

325331
async updateOrgAuthProvider(userId: string, entry: AuthProviderEntry.UpdateOrgEntry): Promise<AuthProviderEntry> {
@@ -351,7 +357,8 @@ export class AuthProviderService {
351357
status: "pending",
352358
};
353359

354-
return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
360+
const result = await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true);
361+
return AuthProviderEntry.redact(result);
355362
}
356363

357364
private initializeNewProvider(newEntry: AuthProviderEntry.NewEntry): AuthProviderEntry {

‎components/server/src/auth/auth-provider.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,26 +5,38 @@
55
*/
66

77
import express from "express";
8-
import { AuthProviderInfo, User, OAuth2Config, AuthProviderEntry } from "@gitpod/gitpod-protocol";
8+
import { AuthProviderInfo, User, AuthProviderEntry } from "@gitpod/gitpod-protocol";
99

1010
import { UserEnvVarValue } from "@gitpod/gitpod-protocol";
1111

1212
export const AuthProviderParams = Symbol("AuthProviderParams");
1313
export interface AuthProviderParams extends AuthProviderEntry {
14-
readonly builtin: boolean; // true, if `ownerId` == ""
15-
readonly verified: boolean; // true, if `status` == "verified"
16-
17-
readonly oauth: OAuth2Config;
14+
/**
15+
* computed value: `true`, if `ownerId` == ""
16+
*/
17+
readonly builtin: boolean;
18+
/**
19+
* computed value: `true`, if `status` == "verified"
20+
*/
21+
readonly verified: boolean;
1822

19-
// properties to control behavior
23+
/**
24+
* @deprecated unused
25+
*/
2026
readonly hiddenOnDashboard?: boolean;
2127

2228
/**
23-
* @deprecated: looks like this is unused after all
29+
* @deprecated unused
2430
*/
2531
readonly disallowLogin?: boolean;
2632

33+
/**
34+
* @deprecated unused
35+
*/
2736
readonly description: string;
37+
/**
38+
* @deprecated unused
39+
*/
2840
readonly icon: string;
2941
}
3042
export function parseAuthProviderParamsFromEnv(json: object): AuthProviderParams[] {

‎components/server/src/auth/host-context-provider-impl.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ export class HostContextProviderImpl implements HostContextProvider {
7575
const knownOAuthRevisions = Array.from(this.dynamicHosts.entries())
7676
.map(([_, hostContext]) => hostContext.authProvider.params.oauthRevision)
7777
.filter((rev) => !!rev) as string[];
78-
const newAndUpdatedAuthProviders = await this.authProviderService.getAllAuthProviders(knownOAuthRevisions);
78+
const newAndUpdatedAuthProviders = await this.authProviderService.getAllAuthProviderParams(knownOAuthRevisions);
7979
ctx.span?.setTag("updateDynamicHosts.newAndUpdatedAuthProviders", newAndUpdatedAuthProviders.length);
8080

8181
for (const config of newAndUpdatedAuthProviders) {

0 commit comments

Comments
 (0)