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-2007] self hosted auth #160

Merged
merged 4 commits into from
Sep 23, 2024
Merged

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Sep 20, 2024

PR Type

Bug fix, Enhancement


Description

  • Fixed the retrieval of the authorization token in the ApiService by directly accessing authorization_token from useAppStore.
  • Refactored the app store to remove redundant authorization_token from the config object and removed the setAuthorizationToken method.
  • Updated the AppState interface to remove authorization_token from the config.
  • Added a new method in PlatformUiServiceProvider to clear platform UI assets, ensuring old assets are removed before publishing new ones.

Changes walkthrough 📝

Relevant files
Bug fix
index.ts
Fix authorization token retrieval in API service                 

resources/js/api/index.ts

  • Fixed authorization token retrieval from useAppStore.
+1/-1     
Enhancement
index.ts
Refactor authorization token handling in app store             

resources/js/store/index.ts

  • Removed redundant authorization_token from config.
  • Updated setupAccount to await init.
  • Removed setAuthorizationToken method.
  • +2/-12   
    types.interface.ts
    Update AppState interface for authorization token               

    resources/js/types/types.interface.ts

    • Removed authorization_token from config interface.
    +0/-1     
    PlatformUiServiceProvider.php
    Add asset clearing functionality in service provider         

    src/PlatformUiServiceProvider.php

    • Added method to clear platform UI assets.
    +12/-0   

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

    Copy link

    PR Reviewer Guide 🔍

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

    Possible Bug
    The code does not handle the case where authorization_token might be undefined or null. This could potentially lead to setting an undefined or null Authorization header, which might cause unexpected behavior or errors in API requests.

    Redundant Check
    The check for !this.authorization_token seems redundant because the authorization_token is set in the same method (setupAccount) before this check. This could lead to confusion or unnecessary code complexity.

    File Deletion Handling
    The method clearPlatformUIAssets deletes the directory without any checks or logs if the deletion was successful or necessary (e.g., if the directory was not empty). Adding error handling and logging would make this operation safer and more transparent.

    Copy link

    github-actions bot commented Sep 20, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a check to ensure the authorization token is not empty before setting the Authorization header

    Ensure that the Authorization header is set correctly by checking if
    authorization_token is not empty before setting it. This prevents sending invalid or
    empty tokens which can lead to unauthorized requests.

    resources/js/api/index.ts [49]

    -headers.Authorization = useAppStore().authorization_token;
    +if (useAppStore().authorization_token) {
    +    headers.Authorization = useAppStore().authorization_token;
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring that an empty authorization token is not set, which could prevent unauthorized requests. It is a crucial improvement for security and reliability.

    9
    Security
    Ensure authorization_token is not empty before assignment to prevent security risks

    Add a check to ensure that authorization_token is not empty before setting it in the
    setupAccount method to avoid potential security issues with empty tokens.

    resources/js/store/index.ts [117]

    -this.authorization_token = authorization_token;
    +if (authorization_token) {
    +    this.authorization_token = authorization_token;
    +}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances security by preventing the assignment of an empty authorization token, which could lead to security vulnerabilities. It is an important improvement for maintaining secure token handling.

    8
    Reliability
    Implement error handling for directory deletion to enhance reliability

    Add error handling for the File::deleteDirectory method to manage potential failures
    during the deletion process, which can be crucial for maintaining a stable
    application state.

    src/PlatformUiServiceProvider.php [53]

    -File::deleteDirectory($path);
    +if (!File::deleteDirectory($path)) {
    +    Log::error("Failed to delete directory at: " . $path);
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the directory deletion process improves the application's reliability by ensuring that failures are logged and can be addressed, which is crucial for maintaining a stable application state.

    8
    Enhancement
    Improve error handling by replacing a direct return of false with throwing an error for missing authorization tokens

    Replace the direct return of false with throwing an error or a more descriptive
    failure handling mechanism to improve debugging and error tracking.

    resources/js/store/index.ts [61-62]

     if (!this.isMultiTenant && !this.authorization_token) {
    -    return false;
    +    throw new Error("Authorization token is missing in non-multi-tenant mode.");
     }
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves error tracking and debugging by providing a more descriptive failure mechanism, which can be beneficial for developers to understand the cause of the issue more clearly. However, it is not as critical as the security-related suggestions.

    7

    v16Studios
    v16Studios previously approved these changes Sep 20, 2024
    @zlayine zlayine merged commit 1aeb2d9 into master Sep 23, 2024
    3 checks passed
    @zlayine zlayine deleted the bugfix/pla-2007/self-hosted-auth branch September 23, 2024 12:23
    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