refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463

aldousalvarez · 2024-08-07T05:58:03Z

Commit to be reviewed

refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json

Primary Changes
----------------
1. added OAuth2 security endpoints scopes to openapi.json
2. added a test to make sure if the scopes are indeed getting 
   pulled from the spec file

Fixes #2693

Pull Request Requirements

Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

outSH · 2024-08-07T10:31:11Z

...es/cactus-cmd-api-server/src/test/typescript/unit/get-open-api-spec-v1-oauth2-scopes.test.ts

+}
+
+describe("cmd-api-server:getOpenApiSpecV1Endpoint", () => {
+  const logLevel: LogLevelDesc = "TRACE";


Change log level to info, dont use trace for tests in CI

@outSH Got it, just finished updating it.

outSH

LGTM, Thank you!

petermetz · 2024-08-07T20:37:07Z

...es/cactus-cmd-api-server/src/test/typescript/unit/get-open-api-spec-v1-oauth2-scopes.test.ts

+  it("HTTP - returns the OpenAPI spec .json document of the API server itself", async () => {
+    const res1Promise = apiClient.getOpenApiSpecV1();
+    await expect(res1Promise).resolves.toHaveProperty("data.openapi");
+    const res1 = await res1Promise;
+    expect(res1.status).toEqual(200);
+    expect(res1.data).toBeTruthy();
+
+    console.log("Response data type:", typeof res1.data);
+    console.log("Response data:", res1.data);
+
+    let openApiSpec;
+    try {
+      openApiSpec =
+        typeof res1.data === "string" ? JSON.parse(res1.data) : res1.data;
+    } catch (error) {
+      throw new Error(`Failed to parse OpenAPI spec: ${error.message}`);
+    }
+
+    expect(openApiSpec).toHaveProperty("components");
+    expect(openApiSpec.components).toHaveProperty("securitySchemes");
+
+    const securitySchemes = openApiSpec.components.securitySchemes;
+    expect(securitySchemes).toBeObject();
+
+    const expectedScopes: IExpectedScopes = {
+      "read:health": "Read health information",
+      "read:metrics": "Read metrics information",
+      "read:spec": "Read OpenAPI specification",
+    };
+
+    const securitySchemeNames = Object.keys(securitySchemes);
+
+    securitySchemeNames.forEach((schemeName) => {
+      const scheme = securitySchemes[schemeName];
+      expect(scheme).toHaveProperty("flows");
+      const flows = scheme.flows;
+      expect(flows).toHaveProperty("authorizationCode");
+      const scopes = flows.authorizationCode.scopes as IExpectedScopes;
+
+      Object.keys(expectedScopes).forEach((scope) => {
+        expect(scopes).toHaveProperty(scope);
+        expect(scopes[scope]).toEqual(expectedScopes[scope]);
+      });
+    });
+  });


@aldousalvarez This verifies that the OpenAPI .json file has the security scheme declared. It does not verify that those are being respected by the endpoint implementations themselves. Please update the endpoints touched by this diff to make use of the declared scopes and then write 2 more test cases for each endpoint in addition this one I'm commenting on. One of them should be the positive test case where with a valid JWT the API client can execute the request and with an invalid JWT the request execution fails with an HTTP 401 unauthorized error.
Note: by test cases I just mean another it("does something...", async () => {...}) block not a separate file.

aldousalvarez · 2024-08-21T15:27:58Z

Hello @petermetz, Already done the requested changes, re-requested for your review. Thank you

petermetz

@aldousalvarez Looking much better now but it's still missing the crucial negative test case for missing or incorrect scopes, please see my comment above.

petermetz · 2024-08-22T18:01:08Z

...es/cactus-cmd-api-server/src/test/typescript/unit/get-open-api-spec-v1-oauth2-scopes.test.ts

+    expect(res1.status).toEqual(200);
+    expect(res1.data).toBeTruthy();


@aldousalvarez In this test case it is supposed to fail because you are executing with a JWT that does not have the scope declared (so the JWT has a valid signature but it does not declare the correct role based access control and therefore should not work)

Primary Changes ---------------- 1. added OAuth2 security endpoints scopes to openapi.json 2. added a test to make sure if the scopes are indeed getting pulled from the spec file Fixes hyperledger#2693 Signed-off-by: aldousalvarez <[email protected]>

aldousalvarez requested review from petermetz, takeutak, izuru0, jagpreetsinghsasan, VRamakrishna, sandeepnRES and outSH as code owners August 7, 2024 05:58

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from 5d2cd8e to 3321ed3 Compare August 7, 2024 05:58

outSH requested changes Aug 7, 2024

View reviewed changes

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from 3321ed3 to f44cf0c Compare August 7, 2024 11:51

aldousalvarez requested a review from outSH August 7, 2024 11:52

outSH approved these changes Aug 7, 2024

View reviewed changes

petermetz requested changes Aug 7, 2024

View reviewed changes

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from f44cf0c to 25c1eba Compare August 21, 2024 14:40

aldousalvarez requested a review from petermetz August 21, 2024 15:27

petermetz requested changes Aug 22, 2024

View reviewed changes

aldousalvarez requested a review from RafaelAPB as a code owner September 6, 2024 03:14

aldousalvarez force-pushed the aldousalvarez/issue2693 branch 2 times, most recently from 7aeb0e1 to dade3eb Compare September 6, 2024 03:19

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from dade3eb to 4c58d17 Compare September 6, 2024 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463

refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463

aldousalvarez commented Aug 7, 2024 •

edited

Loading

outSH Aug 7, 2024

aldousalvarez Aug 7, 2024

outSH left a comment

petermetz Aug 7, 2024

aldousalvarez commented Aug 21, 2024 •

edited

Loading

petermetz left a comment

petermetz Aug 22, 2024

		expect(res1.status).toEqual(200);
		expect(res1.data).toBeTruthy();

refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463

Are you sure you want to change the base?

refactor(cmd-api-server): pull OAuth2 endpoint scopes from openapi.json #3463

Conversation

aldousalvarez commented Aug 7, 2024 • edited Loading

Commit to be reviewed

outSH Aug 7, 2024

Choose a reason for hiding this comment

aldousalvarez Aug 7, 2024

Choose a reason for hiding this comment

outSH left a comment

Choose a reason for hiding this comment

petermetz Aug 7, 2024

Choose a reason for hiding this comment

aldousalvarez commented Aug 21, 2024 • edited Loading

petermetz left a comment

Choose a reason for hiding this comment

petermetz Aug 22, 2024

Choose a reason for hiding this comment

aldousalvarez commented Aug 7, 2024 •

edited

Loading

aldousalvarez commented Aug 21, 2024 •

edited

Loading