Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: adding body as a possible option for getting token #117

Merged

Conversation

aversini
Copy link
Collaborator

@aversini aversini commented Jul 19, 2024

User description

BREAKING CHANGE: the params are now an object


PR Type

Enhancement, Tests


Description

  • Enhanced getToken function to support token extraction from the request body.
  • Added BodyLike type definition to represent the request body structure.
  • Updated existing tests to use the new object parameter format for getToken.
  • Added new tests to verify token extraction from the body and prioritization over cookies and headers.

Changes walkthrough 📝

Relevant files
Tests
getToken.test.ts
Add and update tests for getToken function enhancements   

packages/auth-common/src/components/tests/getToken.test.ts

  • Added tests for extracting token from body.
  • Updated existing tests to use new object parameter format.
  • Added test to prioritize token from body over cookie or authorization
    header.
  • +30/-7   
    Enhancement
    getToken.ts
    Enhance getToken function to support token extraction from body

    packages/auth-common/src/components/getToken.ts

  • Added BodyLike type definition.
  • Enhanced getToken function to accept body parameter.
  • Implemented token extraction from body.
  • Updated function documentation to reflect new behavior.
  • +22/-3   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    BREAKING CHANGE: the params are now an object
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Type Consistency
    The getFromBody function should ensure the type consistency of the return value. It currently returns undefined if the access_token is not a string, which might lead to type inconsistencies in getToken where the return type is expected to be a string. Consider returning an empty string if access_token is not valid.

    Error Handling
    There is no error handling for malformed inputs in the getFromBody and getToken functions. Consider adding checks or try-catch blocks to handle potential errors gracefully, especially when dealing with external inputs.

    Copy link

    qodo-merge-pro bot commented Jul 19, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    ✅ Add a test case to ensure getToken handles undefined headers and body correctly

    Add a test case to verify that getToken returns an empty string when both headers
    and body are undefined, ensuring the function handles null inputs gracefully.

    packages/auth-common/src/components/tests/getToken.test.ts [6-8]

     it("should return empty string if no token is found", () => {
       const headers: HeadersLike = {};
       expect(getToken({ headers, clientId })).toBe("");
     });
    +it("should return empty string if headers and body are undefined", () => {
    +  expect(getToken({ clientId })).toBe("");
    +});
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    Why: Adding this test case ensures that the function handles null inputs gracefully, which is crucial for robust testing and reliability.

    9
    ✅ Ensure getToken function always returns a string by simplifying the return statement

    Modify the getToken function to handle cases where fromBody, fromCookie, and
    fromHeader might all be undefined, to ensure the function always returns a string.

    packages/auth-common/src/components/getToken.ts [63-67]

    -if (!fromCookie && !fromHeader && !fromBody) {
    -  return "";
    -}
    -return (fromBody || fromCookie || fromHeader) as string;
    +return fromBody || fromCookie || fromHeader || "";
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Simplifying the return statement ensures that the function always returns a string, which improves code readability and maintainability.

    7
    Possible bug
    Add a null check for the body parameter to prevent potential runtime errors

    Consider adding a null check for body in the getFromBody function to handle cases
    where body might be undefined, which will prevent runtime errors.

    packages/auth-common/src/components/getToken.ts [34-38]

     const getFromBody = (body?: BodyLike) => {
    +  if (!body) return;
       const accessToken = body?.access_token;
       if (typeof accessToken === "string") {
         return accessToken;
       }
     };
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a null check for the body parameter is a good practice to prevent potential runtime errors when body is undefined. This enhances the robustness of the function.

    8
    Maintainability
    Use a common token variable in test cases to reduce repetition and enhance maintainability

    Refactor the test cases to use a common token string variable to avoid repetition
    and make the tests easier to maintain.

    packages/auth-common/src/components/tests/getToken.test.ts [20-33]

    +const commonToken = "commonToken";
     it("should extract token from authorization header", () => {
    -  const token = "someToken";
       const headers: HeadersLike = {
    -    authorization: `Bearer ${token}`,
    +    authorization: `Bearer ${commonToken}`,
       };
    -  expect(getToken({ headers, clientId })).toBe(token);
    +  expect(getToken({ headers, clientId })).toBe(commonToken);
     });
     it("should extract token from cookie", () => {
    -  const token = "anotherToken";
       const headers: HeadersLike = {
    -    cookie: `auth.${clientId}=${token};`,
    +    cookie: `auth.${clientId}=${commonToken};`,
       };
    -  expect(getToken({ headers, clientId })).toBe(token);
    +  expect(getToken({ headers, clientId })).toBe(commonToken);
     });
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a common token variable reduces repetition and makes the tests easier to maintain, although the improvement is minor.

    6

    Copy link

    Bundle Size

    Status File Size (Gzip) Limits
    index.js 17.74 KB 18 kb

    Overall bundle size: 17.74 KB
    Overall status: ✅

    @aversini aversini merged commit 768a977 into main Jul 19, 2024
    4 checks passed
    @aversini aversini deleted the feat-adding-body-as-a-possible-option-for-getting-token branch July 19, 2024 13:07
    @aversini aversini mentioned this pull request Jul 19, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant