feat: FI-49 Add Mixpanel basic auth #14

elien2016 · 2024-01-11T01:55:58Z

Describe your changes

added basic auth and Mixpanel integration
added some types and narrowed param or return types at some places
added try catch in ExternalAuthIntegrationService

(haven't encrypted yet, will do later)
(will add/update docs and tests after a preliminary review)

Issue ticket number and link

FI-49
https://catting.atlassian.net/browse/FI-49?atlOrigin=eyJpIjoiNDk0ZDhiOGQ2ZGQyNDc2MjlhZDA1MWQwMGQ3YTUxYmYiLCJwIjoiaiJ9

Checklist before requesting a review

I have performed a self-review of my code
If it is a core feature, I have added thorough tests.
Do we need to implement analytics?
Will this be part of a product update? If yes, please write one phrase about this update.

also parsed github oauth2 tokenResponse into proper json

codesandbox · 2024-01-11T01:56:00Z

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

faze059 · 2024-01-11T03:34:16Z

@core/cms/src/external-auth-integration/auth-providers/basic-auth/@types/index.ts

Need to change comments

The purpose of this draft PR is to check if the general idea of the implementation of basic auth is correct; all the comments and docs were pasted over from existing files and are all wrong and should be ignored. I'll add docs and tests and linting and formatting after getting confirmation that the general idea is right...

faze059 · 2024-01-11T03:38:30Z

@core/cms/src/external-auth-integration/auth-providers/basic-auth/@types/index.ts

+export type BasicAuthString = `basic-${string}`;
+
+/**
+ * Represents the client credential for Basic HTTP Authentication.


I found that in OAuth2, ICredentials may not a good name, it should be like IClientCredentials or sth

is IClientCredentials meant to be shared with other types of auth, like Basic Auth?

faze059 · 2024-01-11T03:40:12Z

@core/cms/src/external-auth-integration/auth-providers/basic-auth/@types/index.ts

+ * @property {number} [tokenRefreshBuffer] - Time in seconds before expiration to refresh the token.
+ * @property {{ [key: string]: string }} [callbackUriParams] - Optional additional parameters for the callback URI.
+ */
+export interface BasicAuthProvider {


IBasicAuthConfiguration

faze059 · 2024-01-11T03:42:43Z

@core/cms/src/external-auth-integration/auth-providers/basic-auth/@types/index.ts

+ * @property {string} callbackUri - The URI for the OAuth2 callback after authentication.
+ * @property {{ [key: string]: string }} [callbackUriParams] - Optional additional parameters for the callback URI.
+ */
+export interface BasicAuthConfig {


I found that name in OAuth may be confused, actually BasicAuthConfig cannot store pw and username

The pw and username are the service account username and secret; I thought that's the credential we're supposed to put in .env (like the clientId and client secret of OAuth)? The authenticate() method basically just verifies it, and the verify() method retrieves an expiration date (generally service accounts don't expire so it would be null most of the time) and some other info. We should discuss it tmr, I'm a bit confused lol

we will store PW and username in db, actually in OAuth 2, we still store credentials in db, however the interface name ICredential in OAuth is confused, I have made a new PR to change it

faze059 · 2024-01-11T03:44:08Z

...ms/src/external-auth-integration/auth-providers/basic-auth/interface/basic-auth.interface.ts

Change comment

...ms/src/external-auth-integration/auth-providers/basic-auth/mixpanel/v1/mixpanel.v1.config.ts

faze059 · 2024-01-11T03:47:28Z

...ms/src/external-auth-integration/auth-providers/basic-auth/mixpanel/v1/mixpanel.v1.config.ts

+        authenticateUrl: "https://mixpanel.com/api/app/me",
+        verifyUrl: "https://mixpanel.com/api/app/organizations/{organizationId}/service-accounts",
+      },
+      credential: { username: process.env.MIXPANEL_V1_BASIC_USERNAME, password: process.env.MIXPANEL_V1_BASIC_PASSWORD },


rm credential: { username: process.env.MIXPANEL_V1_BASIC_USERNAME, password: process.env.MIXPANEL_V1_BASIC_PASSWORD }, as previous described

faze059 · 2024-01-11T03:50:51Z

...s/src/external-auth-integration/auth-providers/basic-auth/mixpanel/v1/mixpanel.v1.service.ts

+   * @param _accessToken
+   * @returns {Promise<any>} The response from the token verification endpoint.
+   */
+  async verify(): Promise<BasicAuthVerificationResponse> {


Looks good, i am not sure why verify is calling authenticate again?

it calls authenticate() for now to get organizationId (required parameter in verifyUrl); I was thinking maybe storing organizationId to Postgres after calling authenticate() for the first time; I wasn't sure if we've finished database integration... lmk what you think

yes, we have db but it is not flexible. I am going to change recently

faze059 · 2024-01-11T03:51:29Z

@core/cms/src/external-auth-integration/auth-providers/oauth2/@types/index.ts

Looks good!

@core/cms/src/external-auth-integration/auth-providers/oauth2/github/v1/github.v1.service.ts

faze059 · 2024-01-11T03:57:08Z

@ybw0014 What do you think?

ybw0014

resolve merge conflicts

ybw0014

The username and password should be provided by the user. They should be passed to authenticate(), and we really need to rewrite this method to include DTO, that carries required info.
For oauth providers, the dto only need to have the userId.
For basic auth providers, the dto also need to have the username and password.
And for the future access key, the dto need to have the access key.

ybw0014 · 2024-01-24T04:35:12Z

@core/cms/src/app.controller.ts

@@ -8,18 +8,24 @@ import { CreateCredentialDto } from "./database-integration/dto/create-credentia
 import { ValidateCredentialPipe } from "./common/pipes/validate-credential.pipe";
 import { ExternalAuthIntegrationService } from "./external-auth-integration/external-auth-integration.service";



Suggested change

ybw0014 · 2024-01-24T04:38:23Z

@core/cms/src/dto/basic-auth-verify.dto.ts

+import { BasicAuthString } from "~/external-auth-integration/auth-providers/basic-auth/@types";
+
+export class BasicAuthVerifyDto {
+  providerKey: BasicAuthString;
+}


can just extend BasicAuthDto to reduce duplicated code

ybw0014 · 2024-01-24T04:40:53Z

...ms/src/external-auth-integration/auth-providers/basic-auth/interface/basic-auth.interface.ts

+/**
+ * This interface represents an OAuth 2.0 provider.
+ */
+export interface IBasicAuth {
+  /**
+   * Returns the authentication URL for the OAuth provider.
+   */
+  authenticate(): Promise<any>;
+
+  /**
+   * Verify the access token.
+   * However, the OAuth 2.0 spec (RFC 6749) doesn't clearly define the interaction between a Resource Server (RS) and Authorization Server (AS) for access token (AT) validation.
+   * @param accessToken The access token.
+   */
+  verify?(): Promise<any>;
+}


change the docs

elien2016 added 5 commits January 9, 2024 23:20

fix: 🐛 fix github oauth2 issues after code update

33d3da2

also parsed github oauth2 tokenResponse into proper json

feat: 🎸 add basic auth

95d10eb

feat: 🎸 rename AuthScheme to AuthService

e3f2fa0

feat: 🎸 add Mixpanel basic auth

c31bfd5

feat: 🎸 add try catch in ExternalAuthIntegrationService

c293336

elien2016 added the enhancement New feature or request label Jan 11, 2024

faze059 requested review from faze059, ybw0014 and zien2017 January 11, 2024 02:25

faze059 added 2 commits January 10, 2024 21:36

fix: rm unused import

1feaae4

fix: add comments of factory

4666f07

faze059 requested changes Jan 11, 2024

View reviewed changes

ybw0014 requested changes Jan 11, 2024

View reviewed changes

ybw0014 requested changes Jan 20, 2024

View reviewed changes

elien2016 added 3 commits January 23, 2024 18:04

Merge branch 'CMS' into FI-49-Add-mixpanel-Basic-Auth

1be948b

feat: 🎸 add dtos for basic auth

f4ac452

feat: 🎸 update allowedCredentials.json

c2c8fd1

ybw0014 requested changes Jan 24, 2024

View reviewed changes

elien2016 closed this Jan 24, 2024

feat: 🎸 parameterize user info for basic auth

79474ac

faze059 reopened this Jan 26, 2024

faze059 requested a review from Zhiqi-Yu January 26, 2024 21:25

		@@ -8,18 +8,24 @@ import { CreateCredentialDto } from "./database-integration/dto/create-credentia
		import { ValidateCredentialPipe } from "./common/pipes/validate-credential.pipe";
		import { ExternalAuthIntegrationService } from "./external-auth-integration/external-auth-integration.service";

feat: FI-49 Add Mixpanel basic auth #14

Are you sure you want to change the base?

feat: FI-49 Add Mixpanel basic auth #14

Uh oh!

Conversation

elien2016 commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue ticket number and link

Checklist before requesting a review

Uh oh!

codesandbox bot commented Jan 11, 2024

Review or Edit in CodeSandbox

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

faze059 commented Jan 11, 2024

Uh oh!

ybw0014 left a comment

Choose a reason for hiding this comment

Uh oh!

ybw0014 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elien2016 commented Jan 11, 2024 •

edited

Loading