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

[PLA-1645] infuse token #143

Merged
merged 10 commits into from
Sep 26, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Aug 1, 2024

PR Type

Enhancement, Bug fix


Description

  • Enhanced sendReadmoreEvent in ReadMoreButton.vue to open URLs in a new tab if the readmore prop contains an HTTP link.
  • Simplified fetchUri function in Collections.vue and Tokens.vue by directly using the response object and removing unnecessary JSON parsing.
  • Updated token name cell styling in Tokens.vue.
  • Added new form fields and logic for ENJ infusion in CreateToken.vue, including state variables for infusion options.
  • Added auth parameter to API service methods in index.ts, updating headers logic to conditionally include authorization or CSRF token.

Changes walkthrough 📝

Relevant files
Enhancement
ReadMoreButton.vue
Enhance `sendReadmoreEvent` to handle URLs                             

resources/js/components/ReadMoreButton.vue

  • Added logic to open URLs in a new tab if the readmore prop contains an
    HTTP link.
  • Retained existing event emission for non-URL readmore props.
  • +5/-1     
    Collections.vue
    Simplify `fetchUri` function in Collections page                 

    resources/js/components/pages/Collections.vue

  • Simplified fetchUri function to directly use the response object.
  • Removed unnecessary JSON parsing.
  • Removed unused error parameter in catch block.
  • +3/-4     
    Tokens.vue
    Simplify `fetchUri` and update token name styling               

    resources/js/components/pages/Tokens.vue

  • Simplified fetchUri function to directly use the response object.
  • Removed unnecessary JSON parsing.
  • Removed unused error parameter in catch block.
  • Updated token name cell styling.
  • +3/-4     
    CreateToken.vue
    Add ENJ infusion options to CreateToken form                         

    resources/js/components/pages/create/CreateToken.vue

  • Added new form fields and logic for ENJ infusion.
  • Introduced state variables for infusion options.
  • Updated form validation and submission logic.
  • +45/-2   
    index.ts
    Add authentication parameter to API service methods           

    resources/js/api/index.ts

  • Added auth parameter to API service methods.
  • Updated headers logic to conditionally include authorization or CSRF
    token.
  • Set default auth to false for fetchUrl method.
  • +9/-4     

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

    @zlayine zlayine self-assigned this Aug 1, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Aug 1, 2024
    Copy link

    github-actions bot commented Aug 1, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Yes, the changes in resources/js/api/index.ts introduce a conditional logic for authentication headers that could lead to security vulnerabilities if the auth flag is not managed securely across different environments or configurations.

    ⚡ Key issues to review

    Possible Bug
    The new implementation in sendReadmoreEvent does not check for the validity of the URL before opening it. This could lead to potential security risks if the URL is malformed or is a JavaScript execution scheme.

    Security Concern
    The conditional inclusion of either an Authorization header or CSRF token based on the auth flag and isMultiTenant status could lead to inconsistent security practices. This might expose the application to security vulnerabilities if not handled correctly.

    Copy link

    github-actions bot commented Aug 1, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Improve security by adding rel="noopener noreferrer" to window.open

    Consider adding rel="noopener noreferrer" to the window.open method to improve
    security by preventing the new page from being able to access the window.opener
    property and ensuring it runs in a separate process.

    resources/js/components/ReadMoreButton.vue [21]

    -window.open(props.readmore, '_blank');
    +window.open(props.readmore, '_blank', 'noopener,noreferrer');
     
    Suggestion importance[1-10]: 10

    Why: This suggestion enhances security by preventing the new page from accessing the window.opener property and ensuring it runs in a separate process, which is crucial for preventing potential security vulnerabilities.

    10
    Possible bug
    Correct the typo in the variable name to ensure consistency

    Ensure that the variable name totalInfuseAmount is consistent in its spelling to
    avoid potential bugs due to typos.

    resources/js/components/pages/create/CreateToken.vue [287]

    -name="totalIbfuseAmount"
    +name="totalInfuseAmount"
     
    Suggestion importance[1-10]: 9

    Why: Correcting the typo in the variable name is crucial to avoid potential bugs and ensure that the code functions as intended.

    9
    Enhancement
    Improve error handling in the catch block

    It's recommended to handle exceptions more specifically rather than returning a
    generic '-' character. Consider logging the error or providing a more descriptive
    message.

    resources/js/components/pages/Collections.vue [339]

    -} catch {
    -    return '-';
    +} catch (error) {
    +    console.error("Failed to fetch URI:", error);
    +    return 'Fetch error';
     }
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves error handling by logging the error and providing a more descriptive message, which can help in debugging and understanding the cause of the failure.

    8
    Maintainability
    Simplify the conditional assignment of headers in the fetch method

    The auth condition inside the fetch method could be simplified by using a ternary
    operator to assign the header based on isMultiTenant status, reducing the need for
    an if-else structure.

    resources/js/api/index.ts [50-53]

     if (auth) {
    -    if (!useAppStore().isMultiTenant) {
    -        headers.Authorization = useAppStore().config.authorization_token;
    -    } else {
    -        headers['X-CSRF-TOKEN'] = csrf;
    -    }
    +    const headerKey = useAppStore().isMultiTenant ? 'X-CSRF-TOKEN' : 'Authorization';
    +    headers[headerKey] = useAppStore().isMultiTenant ? csrf : useAppStore().config.authorization_token;
     }
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code maintainability by simplifying the conditional logic using a ternary operator, making the code more concise and easier to read.

    7

    @zlayine zlayine force-pushed the feature/pla-1645/infuse-token branch 2 times, most recently from 6650ba1 to 4b94fa4 Compare September 25, 2024 16:28
    @zlayine zlayine force-pushed the feature/pla-1645/infuse-token branch from 4b94fa4 to 8dd78b2 Compare September 25, 2024 16:42
    @zlayine zlayine changed the base branch from master to feature/pla-2008/api-2.0 September 25, 2024 16:43
    @zlayine zlayine marked this pull request as ready for review September 25, 2024 16:43
    @zlayine zlayine merged commit 9ed12eb into feature/pla-2008/api-2.0 Sep 26, 2024
    4 checks passed
    @zlayine zlayine deleted the feature/pla-1645/infuse-token branch September 26, 2024 08:16
    zlayine added a commit that referenced this pull request Oct 4, 2024
    zlayine added a commit that referenced this pull request Oct 11, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants