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)

petermetz

@aldousalvarez Please fix the rest of the failing tests of the cmd-api-server package and also the custom-checks check which also is failing.

petermetz · 2024-10-18T02:17:23Z

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

+    const resPromise = apiClient.getHealthCheckV1({
+      headers: { Authorization: invalidBearerToken },
+    });
+    await expect(resPromise).rejects.toThrow(


@aldousalvarez Almost there! Please make sure to explicitly assert that the status property of the is 401 or 403 depending on which one is applicable (403 if the token is valid BUT missing the correct scope, 401 otherwise)

petermetz · 2024-10-18T02:17:31Z

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

+    });
+    await expect(resPromise).rejects.toThrow(
+      "Request failed with status code 401",
+    );


@aldousalvarez Almost there! Please make sure to explicitly assert that the status property of the is 401 or 403 depending on which one is applicable (403 if the token is valid BUT missing the correct scope, 401 otherwise)

aldousalvarez · 2024-10-21T11:35:21Z

Hello @petermetz I added another test case where it would be 403, and then I also added that it logs which status code it gets,. Should I also add 403 in the responses on the openapi.json as well? Thanks

petermetz · 2024-10-31T18:08:43Z

Hello @petermetz I added another test case where it would be 403, and then I also added that it logs which status code it gets,. Should I also add 403 in the responses on the openapi.json as well? Thanks

@aldousalvarez Sorry for the slow response! Thank you for the changes and to your question: Yes, please declare the 403 in the openapi.tpl.json spec as well (and make sure to run codegen so that it updates openapi.json also)

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-cacti#2693 Signed-off-by: aldousalvarez <[email protected]> 1. Please also refactor the third endpoint (prometheus metrics) accordingly 2. Also please extend the test case with each tokens having specific scopes and then assert that the tokesn with the correct scopes work and the ones that don't have the correct scopes do not even when they are otherwise valid tokens. Signed-off-by: Peter Somogyvari <[email protected]>

aldousalvarez · 2024-11-06T04:23:26Z

Hello @petermetz re-requested for review. already done with the requested changes and updated some of the tests with the required scope after running yarn run codegen so the tests would pass. Thank you

petermetz · 2024-11-07T17:56:35Z

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

+    try {
+      await resPromise;
+    } catch (error) {
+      const statusCode = error.response?.status;
+      console.log(`Request failed with status code: ${statusCode}`);
+    }
+  });


@adrianbatuto Please clean up this other code statements, the line above this already verifies that the request will fail.

This applies to the entire PR - please go through all the assertions in all the test cases that you've modified to make the requested changes.

petermetz · 2024-11-07T17:57:08Z

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

+    const resPromise = apiClient.getHealthCheckV1({
+      headers: { Authorization: invalidBearerToken },
+    });
+    await expect(resPromise).rejects.toThrow();


@adrianbatuto Please verify not just that it gets rejected but also that the status code is as expected. You can look at other test cases to see how we do negative test cases with HTTP status code assertions (let me know if you need more detailed pointers)

This applies to the entire PR - please go through all the assertions in all the test cases that you've modified to make the requested changes.

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 3 times, most recently from dade3eb to 4c58d17 Compare September 6, 2024 03:40

petermetz force-pushed the aldousalvarez/issue2693 branch from 4c58d17 to 20d64d4 Compare October 4, 2024 02:39

aldousalvarez force-pushed the aldousalvarez/issue2693 branch 2 times, most recently from af634c3 to 21fdde9 Compare October 9, 2024 12:48

aldousalvarez requested a review from petermetz October 9, 2024 12:49

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from 21fdde9 to a637f50 Compare October 10, 2024 07:54

petermetz requested changes Oct 18, 2024

View reviewed changes

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from a637f50 to 916d802 Compare October 21, 2024 11:31

aldousalvarez force-pushed the aldousalvarez/issue2693 branch 10 times, most recently from bf9ae91 to 30dbad7 Compare November 5, 2024 12:50

aldousalvarez force-pushed the aldousalvarez/issue2693 branch from 30dbad7 to 614db4e Compare November 6, 2024 03:08

aldousalvarez requested a review from petermetz November 6, 2024 04:04

petermetz requested changes Nov 7, 2024

View reviewed changes

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

petermetz left a comment

petermetz Oct 18, 2024

petermetz Oct 18, 2024

aldousalvarez commented Oct 21, 2024

petermetz commented Oct 31, 2024

aldousalvarez commented Nov 6, 2024

petermetz Nov 7, 2024

petermetz Nov 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outSH left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

petermetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldousalvarez commented Oct 21, 2024

petermetz commented Oct 31, 2024

aldousalvarez commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldousalvarez commented Aug 7, 2024 •

edited

Loading

aldousalvarez commented Aug 21, 2024 •

edited

Loading