Feature: Canner PAT authenticator #181

onlyjackfrost · 2023-06-05T03:40:42Z

Description

In this pull request, I implemented an extension "extension-authenticator-canner" that enabled Vulcan API to authenticate users with Canner Enterprise and retrieve the user information from Canner Enterprise. We can access these user attributes after authentication.

Please see the README to know how to setup the extension.

Issue ticket number

#177

Note

We changed the auth configure checking logic to if "auth.enabled" is set to true but didn't configure any authenticator, we will not throw a configured error when serving but throw "all types of authenticator failed" when sending Data API(with or without "Authorization" header or any replacement field/header).

Test result:

labs/playground
only pat

only basic auth

auth is enabled but not configured

vercel · 2023-06-05T03:40:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
vulcan-sql-document	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 12, 2023 10:19am

kokokuo

Beside some suggestions, others LGTM 👍

kokokuo · 2023-06-05T06:26:32Z

packages/integration-testing/src/examples/example5-get-user-profile-CannerPAT.spec.ts

+    const stub = sinon.default.stub(
+      CannerPATAuthenticator.prototype,
+      <any>'fetchCannerUser'


Suggest adding the comment to describe what reason make here should use the prototype to stub the method because it's an unregular way to stub the method of a class.

Btw, here you could also have another way to prevent using the prototype to stub private method, the solution ( Depends on you, two way both work, just need to add the comment if you use the unregular solution):

Could create a folder e.g: canner/

Move the CannerPATAuthenticator class to the folder canner/

Make fetchCannerUser private method to a function expression object, and put it in a helpers.ts file, move the helpers.ts under the canner/, it means the helpers.ts only used in the related file under the canner/.

import the fetchCannerUser function from helpers and call it in the CannerPATAuthenticator, then you could stub the function in the test cases, and prevent stubbing the private method.

PS: if your private method could be a common method and make multiple places to use, then you could create a folder utils/ under the libs/ and make the private common method a function and put it in a file under the utils/, and write the test case for the function under the test

You could refer to the flattenElements.ts and https://github.com/Canner/vulcan-sql/blob/2b5e282aa047cfaff1c80471e2e4bca119bde769/packages/core/src/lib/utils/normalizedStringValue.ts under the core package, we also write the tests case for them under the test/utils :)

Thanks for the solution!
I'll add some comments for now, and I think we can encapsulate how to make requests to Canner and move it to utils

Do you forget to add the // TODO: refactor to avoid stubbing private method ?

kokokuo · 2023-06-05T06:50:44Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+    // const { host, port } = this.options;
+    // if (this.options && (!host || !port))
+    //   throw new ConfigurationError('please provide canner "host" and "port".');


Do you forget to remove the code ?

kokokuo · 2023-06-05T07:09:28Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+    const res = await this.fetchCannerUser(token);
+    if (res.status == 401) throw new UserError('invalid token');
+    if (!res.data.data?.userMe) {
+      throw new InternalError('Can not retrieve user info from canner server');


The Axios will throw the error if the HTTP code is not 200, so if you could like to check the 401.

You may need to handle the code in the fetchCannerUser method through try-catch and check the status to throw UserError and check the userMe together, of couse, you could catch the error in fetchCannerUser and transform the response and return data to make your validate to check 40 and userMe.

Do you miss the part ? Or did I miss some situation from canner enterprise?

I saw you update the fetchCannerUser method, but since you throw the error from my suggestion, so you wouldn't trigger the conditions if(res.status == 401) throw new UserError('invalid token');

Btw: In JS or TS, the equal should be use ===, not ==, it will have different result.

Otherwise, just curious what situation do you call the userMe and got the status = 200, but userMe is undefined or {}?

In graphQL, when you got 200 , because you have typed the GraphQL response named userMe and it's a field, it must get the result in general.

If you do not type the field, the graphQL syntax error will throw an error like the below:

I'll remove this part case I've handled the error axios throw that the response status is not 2xx in fetchCannerUser.

kokokuo · 2023-06-05T07:10:59Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+  private async fetchCannerUser(token: string) {
+    const graphqlUrl = this.getCannerUrl('/web/graphql');
+    try {
+      return await axios.default.post(


Why do you need to call post by axios.default.post? because calling axios.post directly is the more common way.

I use import * as axios from 'axios', I'll change the import and use axios.post directly

kokokuo · 2023-06-05T07:21:00Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+      throw new InternalError(
+        `Failed to fetch user info from canner server: ${error}`
+      );


Just curious, would you like to print the error's message or the whole content of the error object?

You use the template string way to print the message with error object, and Axios will throw the AxiosError object, so the template string way have printed the [object Object] possibly, not sure if have you tested the error throw result ?

I'll modify this part, and handle the error the same as #172 does

kokokuo · 2023-06-05T07:24:30Z

packages/integration-testing/src/examples/example2-auth-user.spec.ts

+// clear stub after each test
+afterEach(() => {
+  sinon.default.restore();
+});
+


Seems in the file, you didn't use the sinon to stub ? so actually you don't add the restore ?

I'll remove it from the test file.

kokokuo · 2023-06-05T08:48:59Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+import { CannerPATAuthenticator } from '@vulcan-sql/serve/auth';
+import { AuthResult, AuthStatus, KoaContext } from '@vulcan-sql/serve/models';
+
+const wrappedAuthCredential = async (


Wrap the function to reuse it great!

However, according to your test cases for asserting the error, you should prevent including other non-target test logistics, like here you create the authenticator and called the activate method, it is not the target act code section even though these create heavier and activate behavior seems not possibly cause others errors.

kokokuo · 2023-06-05T08:52:19Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+    sinon.default
+      .stub(authenticator, <any>'fetchCannerUser')
+      .resolves(resolveValue);


Same suggestion as above using prototype way.

kokokuo · 2023-06-05T08:53:01Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+  const authenticator = new CannerPATAuthenticator({ options }, '');
+  await authenticator.activate();
+  return await authenticator.getTokenInfo(ctx);


Same suggestion as above creating the authenticator and calling activate method with the target method at the same place.

I've removed this function because it is not used after refactoring.

kokokuo · 2023-06-05T08:55:03Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+    ssl: false,
+  },
+};
+const invalidToken = Buffer.from('clientId:clientSecret').toString('base64');


great way to create the invalid token and also make members know the token content pattern.

- add Canner PAT authenticator - add unit tests - add integration tests

…axios error handling

codecov-commenter · 2023-06-07T09:03:59Z

Codecov Report

Patch coverage: 82.22% and project coverage change: -0.07 ⚠️

Comparison is base (59b152c) 91.06% compared to head (646b3d8) 90.99%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #181      +/-   ##
===========================================
- Coverage    91.06%   90.99%   -0.07%     
===========================================
  Files          328      331       +3     
  Lines         5393     5430      +37     
  Branches       716      725       +9     
===========================================
+ Hits          4911     4941      +30     
- Misses         343      345       +2     
- Partials       139      144       +5

Flag	Coverage Δ
extension-authenticator-canner	`79.48% <79.48%> (?)`
extension-driver-canner	`84.65% <ø> (ø)`
integration-testing	`90.27% <ø> (ø)`
serve	`87.06% <100.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/lib/middleware/auth/authCredentialsMiddleware.ts	`91.66% <ø> (+3.66%)`	⬆️
...kages/serve/src/models/extensions/authenticator.ts	`93.33% <ø> (ø)`
...-authenticator-canner/src/lib/authenticator/pat.ts	`77.14% <77.14%> (ø)`
...uthenticator-canner/src/lib/authenticator/index.ts	`100.00% <100.00%> (ø)`
...s/extension-authenticator-canner/src/lib/config.ts	`100.00% <100.00%> (ø)`
...kages/serve/src/lib/auth/httpBasicAuthenticator.ts	`87.23% <100.00%> (ø)`
...es/serve/src/lib/auth/passwordFileAuthenticator.ts	`81.81% <100.00%> (ø)`
...es/serve/src/lib/middleware/auth/authMiddleware.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kokokuo

Besides some questions and suggestions, others LGTM 👍

Btw, we should move our canner PAT authenticator to a new extension package like canner driver, because canner integration is an optional (enhancement) feature for VulcanSQL.

Keep going 👍

kokokuo · 2023-06-07T14:21:50Z

packages/integration-testing/src/examples/example5-get-user-profile-CannerPAT.spec.ts

+    const stub = sinon.default.stub(
+      CannerPATAuthenticator.prototype,
+      <any>'fetchCannerUser'


Do you forget to add the // TODO: refactor to avoid stubbing private method ?

kokokuo · 2023-06-07T14:22:46Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+export interface CannerPATOptions {
+  host: string;
+  port: number;
+  // default is false


Great comment to make other members know the default value 👍

kokokuo · 2023-06-07T14:25:55Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+    }
+    const cannerUser = res.data.data?.userMe;
+
+    // eslint-disable-next-line @typescript-eslint/no-unused-vars


Why add the ESlint comment? Because seems you could use the username directly, see below:

const { username, ...restAttrs } = cannerUser; return { status: AuthStatus.SUCCESS, type: this.getExtensionId()!, // method name user: { name: username, attr: restAttrs, }, } as AuthResult;

I'll remove the Eslint comment here

kokokuo · 2023-06-07T14:35:46Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+  private getCannerUrl = (path = '/') => {
+    const { host, port, ssl = false } = this.options;
+    if (process.env['IS_ON_KUBERNETES'])
+      return `http://${process.env['WEB_SERVICE_HOST']}${path}`; // for internal usage, we don't need to specify port
+    else {
+      const protocol = ssl ? 'https' : 'http';
+      return `${protocol}://${host}:${port}${path}`;
+    }


Why use the function expression in the getCannerUrl?
If we use the class, we should follow the regular format to write the private method:

private getCannerUrl(path: string = '/') { .... }

Otherwise, we should prevent to use of the function expression for the public method or private method for good habit, or you may face the issues possibly in your career:

Inheritance issue

Multiple decorators issue

Performance issue

Please read our 7. Not use function expression to define methods in a class at Backend Convention document, we also provide the reference link it describe more clearly.

Thanks for the advice! I'll take a look at the document.

kokokuo · 2023-06-07T14:41:15Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+    const res = await this.fetchCannerUser(token);
+    if (res.status == 401) throw new UserError('invalid token');
+    if (!res.data.data?.userMe) {
+      throw new InternalError('Can not retrieve user info from canner server');


Do you miss the part ? Or did I miss some situation from canner enterprise?

I saw you update the fetchCannerUser method, but since you throw the error from my suggestion, so you wouldn't trigger the conditions if(res.status == 401) throw new UserError('invalid token');

Btw: In JS or TS, the equal should be use ===, not ==, it will have different result.

Otherwise, just curious what situation do you call the userMe and got the status = 200, but userMe is undefined or {}?

In graphQL, when you got 200 , because you have typed the GraphQL response named userMe and it's a field, it must get the result in general.

If you do not type the field, the graphQL syntax error will throw an error like the below:

kokokuo · 2023-06-07T14:45:03Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+): Promise<CannerPATAuthenticator> => {
+  const authenticator = new CannerPATAuthenticator({ options }, '');
+  if (stubValue) {
+    // TODO: refactor to avoid stubbing private method


Thanks for adding the TODO for refactoring in the future and the comment 👍

kokokuo · 2023-06-07T15:11:24Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+  mockedAxios.post.mockRejectedValue({
+    request: 'mock request',
+    response: {
+      data: { error: 'mock response error' },
+      status: 500,
+      statusText: 'string',
+      headers: {},
+      config: {},
+    } as any,
+    isAxiosError: true,
+    toJSON: () => ({}),
+    message: 'An error occurred!',
+    name: 'Error',
+  });


Why do you need to mock the axios.post reject value ? You could mock the rejected value for fetchCannerUser directly ?

After discussion, in actually, @onlyjackfrost would like to test the fetchCannerUser's catch and transform part, but currently fetchCannerUser is a private method that is called by the private validate method, so that why he needs to mock the axio.post, so our conclusion is to update the description fo the test case to make others know the intention.

kokokuo · 2023-06-07T15:23:43Z

packages/serve/test/auth/cannerPATAuthenticator.spec.ts

+  const mockResolveValue = {
+    status: 401,
+    data: { error: 'invalid token from canner' },
+  };
+  const authenticator = await getStubAuthenticator(
+    mockOptions,
+    mockResolveValue
+  );


Just curious why do you pass the mockResolveValue be a resolved value for stub fetchCannerUser?

Seems the fetchCannerUser won't return the data whose status is 401, because when Axios get the 401, like the above question in the validate I asked. Or did I miss some situations from Canner Enterprise?

I'll remove the test cases that are testing the remote response whose status is not 200, and keep the test case that is testing the error handling of axios.

kokokuo

I missed some questions, so I append two in here

kokokuo · 2023-06-08T01:58:06Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+          operationName: 'UserMe',
+          variables: {},
+          query:
+            'query UserMe{userMe {accountRole attributes createdAt email groups {id name} lastName firstName username',


The grammar seems incorrect, in GraphQL, you should have the eclose }, like below:

query UserMe{userMe {accountRole attributes createdAt email groups {id name} lastName firstName username}}

You could paste your code in the graphQL playground after you launch your web locally.

kokokuo · 2023-06-08T01:59:54Z

packages/serve/src/lib/auth/cannerPATAuthenticator.ts

+          operationName: 'UserMe',
+          variables: {},
+          query:
+            'query UserMe{userMe {accountRole attributes createdAt email groups {id name} lastName firstName username',


Why do get the attributes, because the attributes usually are empty?

So even the in the Canner Enterprise, our frontend seems not to get the field to display or use, cc @fredalai, @shimin, @JSYOU

These attributes were picked after discussing with William.

Thanks for checking the attribute field and that us know it !👍

…sed code and refine test cases

… module to extension

kokokuo

Besides 3 suggestions, others LGTM 👍

Btw, here are 3 PR descriptions for you :)

Suggest to add the introduction of how to use Canner PAT extension and its configration in PR descriotion, because it is a new feature, like other PR: Feature: Support Catalog User Interface #160, Feature: Dynamic data masking - custom string #140, Feature: Passing route information to template engine #142, Feature: Syntax sugars for parameter validators #108
Your current NOTE seems to explain the reason we have the options issue but does not mention you solve it, so according to your PR description and title, not know you solve the issue.
Suggest you could add another screenshot is that not type options, or other users look your PR may consider we must add options

auth:
  enabled: true
  # looks like the options also could not add it, right ?
  options:
``

kokokuo · 2023-06-12T09:19:46Z

packages/extension-authenticator-canner/README.md

+## Testing
+
+```bash
+nx test extension-authenticator-canner
+```
+
+This library was generated with [Nx](https://nx.dev).


Actually, you don't need to add the testing section, because the README will wrap to NPM and publish for user reading, so user will install the NPM and won't need the information like other extension packages.

For the testing information, we will provide it in the development documentation in the future.

kokokuo · 2023-06-12T09:29:30Z

labs/playground1/examples/use-canner-pat-authenticator/vulcan.yaml

If we create a folder and only provide the vulcan.yaml sample and not contain other SQL, profiles, or API schema files, then your README of the extension-authenticator-canner has done it, so maybe you don't need to add it again ？

kokokuo · 2023-06-12T09:34:05Z

packages/extension-authenticator-canner/src/test/cannerPATAuthenticator.spec.ts

+});
+
+// Token info
+it('Should throw when use getTokenInfo with cannerPATAuthenticator', async () => {


Do you miss the error word => Should throw error... ?

kokokuo

Thanks for fixing it, Well done, LGTM 👍

onlyjackfrost changed the title ~~Feature: Canner PAT authenticator~~ [DRAFT] Feature: Canner PAT authenticator Jun 5, 2023

kokokuo reviewed Jun 5, 2023

View reviewed changes

onlyjackfrost changed the title ~~[DRAFT] Feature: Canner PAT authenticator~~ Feature: Canner PAT authenticator Jun 6, 2023

feat: Canner PAT authenticator

4bd1b29

- add Canner PAT authenticator - add unit tests - add integration tests

onlyjackfrost force-pushed the feature/authenticator branch from 7420552 to 73cee37 Compare June 7, 2023 08:26

vercel bot deployed to Preview June 7, 2023 08:26 View deployment

chore: remove unused code, handle axios error, add test case to test …

824d133

…axios error handling

onlyjackfrost force-pushed the feature/authenticator branch from 73cee37 to 824d133 Compare June 7, 2023 08:56

vercel bot deployed to Preview June 7, 2023 08:57 View deployment

kokokuo reviewed Jun 7, 2023

View reviewed changes

kokokuo reviewed Jun 8, 2023

View reviewed changes

onlyjackfrost added 3 commits June 8, 2023 12:44

fix(feature/authenticator): fix incorrect query structure, remove unu…

08aa72d

…sed code and refine test cases

feat(feature/authenticator): move canner pat authenticator from serve…

efdacb0

… module to extension

chore(feature/authenticator): add playground vulcan.yaml example

94a636e

vercel bot deployed to Preview June 12, 2023 07:01 View deployment

fix(feat/authenticator): remove test cases cause logic change

3d3ab85

vercel bot deployed to Preview June 12, 2023 08:37 View deployment

onlyjackfrost requested review from kokokuo and fredalai June 12, 2023 08:47

kokokuo reviewed Jun 12, 2023

View reviewed changes

chore(feature/authenticator): fix typo

646b3d8

vercel bot deployed to Preview June 12, 2023 10:19 View deployment

kokokuo approved these changes Jun 13, 2023

View reviewed changes

kokokuo merged commit 8b1b1d7 into develop Jun 13, 2023

hanshino deleted the feature/authenticator branch January 31, 2024 07:38

Feature: Canner PAT authenticator #181

Feature: Canner PAT authenticator #181

Conversation

onlyjackfrost commented Jun 5, 2023 • edited by kokokuo Loading

Description

Issue ticket number

Note

Test result:

vercel bot commented Jun 5, 2023 • edited Loading

kokokuo left a comment

Choose a reason for hiding this comment

kokokuo Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 7, 2023 • edited Loading

Codecov Report

kokokuo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

onlyjackfrost Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokokuo left a comment

Choose a reason for hiding this comment

onlyjackfrost commented Jun 5, 2023 •

edited by kokokuo

Loading

vercel bot commented Jun 5, 2023 •

edited

Loading

kokokuo Jun 5, 2023 •

edited

Loading

kokokuo Jun 5, 2023 •

edited

Loading

kokokuo Jun 7, 2023 •

edited

Loading

codecov-commenter commented Jun 7, 2023 •

edited

Loading

kokokuo left a comment •

edited

Loading

kokokuo Jun 7, 2023 •

edited

Loading

kokokuo Jun 7, 2023 •

edited

Loading

kokokuo Jun 7, 2023 •

edited

Loading

onlyjackfrost Jun 8, 2023 •

edited

Loading

kokokuo left a comment •

edited

Loading

kokokuo Jun 12, 2023 •

edited

Loading