diff --git a/components/server/src/auth/auth-provider-service.spec.db.ts b/components/server/src/auth/auth-provider-service.spec.db.ts index 14781e7d27a7c0..4a13f5fbb755f7 100644 --- a/components/server/src/auth/auth-provider-service.spec.db.ts +++ b/components/server/src/auth/auth-provider-service.spec.db.ts @@ -37,24 +37,29 @@ describe("AuthProviderService", async () => { clientId: "123", clientSecret: "secret-123", }; - const expectedProviderPartial = () => - >{ - builtin: false, + const expectedEntry = () => + >{ host: "github.com", - disallowLogin: false, oauth: { authorizationUrl: "https://github.com/login/oauth/authorize", callBackUrl: "https://gitpod.io/auth/callback", clientId: "123", - clientSecret: "secret-123", + clientSecret: "redacted", tokenUrl: "https://github.com/login/oauth/access_token", }, organizationId: undefined, type: "GitHub", - verified: false, status: "pending", ownerId: owner.id, }; + const expectedParams = () => + >{ + builtin: false, + disallowLogin: false, + verified: false, + ...expectedEntry(), + oauth: { ...expectedEntry().oauth, clientSecret: "secret-123" }, + }; const newOrgEntry = () => { @@ -65,24 +70,29 @@ describe("AuthProviderService", async () => { clientSecret: "secret-123", organizationId: org.id, }; - const expectedOrgProviderPartial = () => - >{ - builtin: false, + const expectedOrgEntry = () => + >{ host: "github.com", - disallowLogin: true, oauth: { authorizationUrl: "https://github.com/login/oauth/authorize", callBackUrl: "https://gitpod.io/auth/callback", clientId: "123", - clientSecret: "secret-123", + clientSecret: "redacted", tokenUrl: "https://github.com/login/oauth/access_token", }, organizationId: org.id, type: "GitHub", - verified: false, status: "pending", ownerId: owner.id, }; + const expectedOrgParams = () => + >{ + builtin: false, + disallowLogin: true, + verified: false, + ...expectedOrgEntry(), + oauth: { ...expectedOrgEntry().oauth, clientSecret: "secret-123" }, + }; beforeEach(async () => { container = createTestContainer(); @@ -109,14 +119,14 @@ describe("AuthProviderService", async () => { describe("createAuthProviderOfUser", async () => { it("should create user-level provider", async () => { - const providersAtStart = await service.getAllAuthProviders(); + const providersAtStart = await service.getAllAuthProviderParams(); expect(providersAtStart).to.be.empty; await service.createAuthProviderOfUser(owner.id, newEntry()); - const providers = await service.getAllAuthProviders(); + const providers = await service.getAllAuthProviderParams(); expect(providers).to.have.lengthOf(1); - expect(providers[0]).to.deep.include(expectedProviderPartial()); + expect(providers[0]).to.deep.include(expectedParams()); }); it("should fail in case of conflict with built-in provider", async () => { @@ -127,7 +137,7 @@ describe("AuthProviderService", async () => { host: "github.com", } as any); - const providersAtStart = await service.getAllAuthProviders(); + const providersAtStart = await service.getAllAuthProviderParams(); expect(providersAtStart).to.be.empty; await expectError(ErrorCodes.CONFLICT, service.createAuthProviderOfUser(owner.id, newEntry())); @@ -142,7 +152,7 @@ describe("AuthProviderService", async () => { ); }); it("should fail if trying to register same host", async () => { - const providersAtStart = await service.getAllAuthProviders(); + const providersAtStart = await service.getAllAuthProviderParams(); expect(providersAtStart).to.be.empty; await service.createAuthProviderOfUser(owner.id, newEntry()); @@ -153,14 +163,14 @@ describe("AuthProviderService", async () => { describe("createOrgAuthProvider", async () => { it("should create org-level provider", async () => { - const providersAtStart = await service.getAllAuthProviders(); + const providersAtStart = await service.getAllAuthProviderParams(); expect(providersAtStart).to.be.empty; await service.createOrgAuthProvider(owner.id, newOrgEntry()); - const providers = await service.getAllAuthProviders(); + const providers = await service.getAllAuthProviderParams(); expect(providers).to.have.lengthOf(1); - expect(providers[0]).to.deep.include(expectedOrgProviderPartial()); + expect(providers[0]).to.deep.include(expectedOrgParams()); }); it("should fail if host is not reachable", async () => { await expectError( @@ -172,7 +182,7 @@ describe("AuthProviderService", async () => { ); }); it("should fail if trying to register same host", async () => { - const providersAtStart = await service.getAllAuthProviders(); + const providersAtStart = await service.getAllAuthProviderParams(); expect(providersAtStart).to.be.empty; await service.createOrgAuthProvider(owner.id, newOrgEntry()); @@ -180,4 +190,16 @@ describe("AuthProviderService", async () => { await expectError(ErrorCodes.CONFLICT, service.createAuthProviderOfUser(owner.id, newOrgEntry())); }); }); + describe("getAuthProvider", async () => { + it("should find org-level provider", async () => { + const providersAtStart = await service.getAllAuthProviderParams(); + expect(providersAtStart).to.be.empty; + + const created = await service.createOrgAuthProvider(owner.id, newOrgEntry()); + + const retrieved = await service.getAuthProvider(owner.id, created.id); + console.log(JSON.stringify(retrieved)); + expect(retrieved).to.deep.include(expectedOrgEntry()); + }); + }); }); diff --git a/components/server/src/auth/auth-provider-service.ts b/components/server/src/auth/auth-provider-service.ts index 86606d7a0b8e18..ffcc52e2840b40 100644 --- a/components/server/src/auth/auth-provider-service.ts +++ b/components/server/src/auth/auth-provider-service.ts @@ -33,9 +33,12 @@ export class AuthProviderService { ) {} /** - * Returns all auth providers. + * Returns all **unredacted** auth provider params to be used in the internal + * authenticator parts. + * + * Known internal client `HostContextProviderImpl` */ - async getAllAuthProviders(exceptOAuthRevisions: string[] = []): Promise { + async getAllAuthProviderParams(exceptOAuthRevisions: string[] = []): Promise { const all = await this.authProviderDB.findAll(exceptOAuthRevisions); return all.map((provider) => this.toAuthProviderParams(provider)); } @@ -68,7 +71,7 @@ export class AuthProviderService { async getAuthProviderDescriptionsUnauthenticated(): Promise { const { builtinAuthProvidersConfigured } = this.config; - const authProviders = [...(await this.getAllAuthProviders()), ...this.config.authProviderConfigs]; + const authProviders = [...(await this.getAllAuthProviderParams()), ...this.config.authProviderConfigs]; const toPublic = (ap: AuthProviderParams) => { @@ -89,7 +92,7 @@ export class AuthProviderService { async getAuthProviderDescriptions(user: User): Promise { const { builtinAuthProvidersConfigured } = this.config; - const authProviders = [...(await this.getAllAuthProviders()), ...this.config.authProviderConfigs]; + const authProviders = [...(await this.getAllAuthProviderParams()), ...this.config.authProviderConfigs]; // explicitly copy to avoid bleeding sensitive details const toInfo = (ap: AuthProviderParams) => @@ -180,13 +183,13 @@ export class AuthProviderService { await this.auth.checkPermissionOnUser(userId, "read_info", userId); const result = await this.authProviderDB.findByUserId(userId); - return result; + return result.map((ap) => AuthProviderEntry.redact(ap)); } async getAuthProvidersOfOrg(userId: string, organizationId: string): Promise { await this.auth.checkPermissionOnOrganization(userId, "read_git_provider", organizationId); const result = await this.authProviderDB.findByOrgId(organizationId); - return result; + return result.map((ap) => AuthProviderEntry.redact(ap)); } async deleteAuthProviderOfUser(userId: string, authProviderId: string): Promise { @@ -230,7 +233,7 @@ export class AuthProviderService { await this.auth.checkPermissionOnUser(userId, "read_info", userId); } - return result; + return AuthProviderEntry.redact(result); } async createAuthProviderOfUser(userId: string, entry: AuthProviderEntry.NewEntry): Promise { @@ -257,7 +260,8 @@ export class AuthProviderService { } const authProvider = this.initializeNewProvider(entry); - return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true); + const result = await this.authProviderDB.storeAuthProvider(authProvider, true); + return AuthProviderEntry.redact(result); } private isBuiltInProvider(host: string) { @@ -291,7 +295,8 @@ export class AuthProviderService { oauth, status: "pending", }; - return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true); + const result = await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true); + return AuthProviderEntry.redact(result); } async createOrgAuthProvider(userId: string, newEntry: AuthProviderEntry.NewOrgEntry): Promise { @@ -319,7 +324,8 @@ export class AuthProviderService { } const authProvider = this.initializeNewProvider(newEntry); - return await this.authProviderDB.storeAuthProvider(authProvider, true); + const result = await this.authProviderDB.storeAuthProvider(authProvider, true); + return AuthProviderEntry.redact(result); } async updateOrgAuthProvider(userId: string, entry: AuthProviderEntry.UpdateOrgEntry): Promise { @@ -351,7 +357,8 @@ export class AuthProviderService { status: "pending", }; - return await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true); + const result = await this.authProviderDB.storeAuthProvider(authProvider as AuthProviderEntry, true); + return AuthProviderEntry.redact(result); } private initializeNewProvider(newEntry: AuthProviderEntry.NewEntry): AuthProviderEntry { diff --git a/components/server/src/auth/auth-provider.ts b/components/server/src/auth/auth-provider.ts index 8ab3999ca11787..b91246eab57c7f 100644 --- a/components/server/src/auth/auth-provider.ts +++ b/components/server/src/auth/auth-provider.ts @@ -5,26 +5,38 @@ */ import express from "express"; -import { AuthProviderInfo, User, OAuth2Config, AuthProviderEntry } from "@gitpod/gitpod-protocol"; +import { AuthProviderInfo, User, AuthProviderEntry } from "@gitpod/gitpod-protocol"; import { UserEnvVarValue } from "@gitpod/gitpod-protocol"; export const AuthProviderParams = Symbol("AuthProviderParams"); export interface AuthProviderParams extends AuthProviderEntry { - readonly builtin: boolean; // true, if `ownerId` == "" - readonly verified: boolean; // true, if `status` == "verified" - - readonly oauth: OAuth2Config; + /** + * computed value: `true`, if `ownerId` == "" + */ + readonly builtin: boolean; + /** + * computed value: `true`, if `status` == "verified" + */ + readonly verified: boolean; - // properties to control behavior + /** + * @deprecated unused + */ readonly hiddenOnDashboard?: boolean; /** - * @deprecated: looks like this is unused after all + * @deprecated unused */ readonly disallowLogin?: boolean; + /** + * @deprecated unused + */ readonly description: string; + /** + * @deprecated unused + */ readonly icon: string; } export function parseAuthProviderParamsFromEnv(json: object): AuthProviderParams[] { diff --git a/components/server/src/auth/host-context-provider-impl.ts b/components/server/src/auth/host-context-provider-impl.ts index 5ac1ed5707ed6a..e29caa1dfac447 100644 --- a/components/server/src/auth/host-context-provider-impl.ts +++ b/components/server/src/auth/host-context-provider-impl.ts @@ -75,7 +75,7 @@ export class HostContextProviderImpl implements HostContextProvider { const knownOAuthRevisions = Array.from(this.dynamicHosts.entries()) .map(([_, hostContext]) => hostContext.authProvider.params.oauthRevision) .filter((rev) => !!rev) as string[]; - const newAndUpdatedAuthProviders = await this.authProviderService.getAllAuthProviders(knownOAuthRevisions); + const newAndUpdatedAuthProviders = await this.authProviderService.getAllAuthProviderParams(knownOAuthRevisions); ctx.span?.setTag("updateDynamicHosts.newAndUpdatedAuthProviders", newAndUpdatedAuthProviders.length); for (const config of newAndUpdatedAuthProviders) {