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: passing User Agent for backend to parse and save #164

Merged

Conversation

aversini
Copy link
Collaborator

@aversini aversini commented Aug 25, 2024

PR Type

enhancement


Description

  • Added ua (User Agent) parameter to various functions and GraphQL mutations to enable backend parsing and saving of User Agent information.
  • Updated type definitions to include the new ua parameter.
  • Simplified parameter handling in GraphQL calls by directly using params.

Changes walkthrough 📝

Relevant files
Enhancement
constants.ts
Add User Agent parameter to GraphQL mutation                         

packages/auth-provider/src/common/constants.ts

  • Added ua (User Agent) parameter to VERIFY_AUTHENTICATION GraphQL
    mutation.
  • Updated mutation call to include ua.
  • +4/-2     
    services.ts
    Simplify GraphQL call parameter handling                                 

    packages/auth-provider/src/common/services.ts

  • Simplified variable assignment by removing requestData.
  • Directly used params in GraphQL call.
  • +1/-2     
    types.d.ts
    Update type definition to include User Agent                         

    packages/auth-provider/src/common/types.d.ts

  • Added ua (User Agent) to AuthenticateUserProps type definition.
  • +1/-0     
    utilities.ts
    Include User Agent in loginUser function                                 

    packages/auth-provider/src/common/utilities.ts

  • Added ua parameter to loginUser function.
  • Passed ua in the function call.
  • +2/-0     
    AuthProvider.tsx
    Pass User Agent from AuthProvider component                           

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx

  • Passed navigator.userAgent as ua in AuthProvider.
  • Updated function calls to include ua.
  • +2/-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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The User Agent string is being passed to the backend. While not inherently sensitive, it could potentially be used for fingerprinting or tracking. Ensure that the backend handles and stores this information securely, and consider if all parts of the User Agent string are necessary for your use case.

    ⚡ Key issues to review

    Potential Inconsistency
    The sessionExpiration parameter is added to the GraphQL mutation, but it's not clear if it's being used consistently across all relevant functions.

    Code Simplification
    The removal of const requestData = params; simplifies the code, but ensure this change doesn't affect any other parts of the codebase that might have been using requestData.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a utility function to get the user agent string

    Instead of directly using navigator.userAgent, consider creating a utility function
    to get the user agent string. This would make it easier to mock for testing and
    potentially add additional processing or normalization if needed in the future.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [237]

    -ua: navigator.userAgent,
    +ua: getUserAgent(),
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Creating a utility function for obtaining the user agent string is a good practice, enhancing testability and maintainability. This is a beneficial improvement, especially for testing purposes.

    8
    Enhancement
    Use object spread syntax for merging parameters in the request body

    Consider using object spread syntax to merge params with the existing object,
    instead of adding a new property. This approach is more concise and less prone to
    errors if the structure of the object changes in the future.

    packages/auth-provider/src/common/services.ts [69-72]

     body: JSON.stringify({
       query: type.schema,
    -  variables: params,
    +  ...params,
     }),
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use object spread syntax is valid as it can make the code more concise and adaptable to future changes in the object structure. However, it is not crucial for functionality, hence a moderate score.

    7
    Maintainability
    Use a multi-line template literal for improved GraphQL query readability

    Consider using a template literal for the GraphQL query string. This would improve
    readability and make it easier to maintain, especially for longer queries.

    packages/auth-provider/src/common/constants.ts [85-104]

    -VERIFY_AUTHENTICATION: `mutation VerifyPasskeyAuthentication(
    -  $clientId: String!,
    -  $id: String!,
    -  $authentication: AuthenticationOptionsInput!,
    -  $nonce: String!,
    -  $domain: String,
    -  $sessionExpiration: String,
    -  $ua: String) {
    -verifyPasskeyAuthentication(
    -  clientId: $clientId,
    -  id: $id,
    -  authentication: $authentication,
    -  nonce: $nonce,
    -  domain: $domain,
    -  sessionExpiration: $sessionExpiration,
    -  ua: $ua) {
    -    status,
    -    idToken,
    -    accessToken,
    -    refreshToken,
    +VERIFY_AUTHENTICATION: `
    +  mutation VerifyPasskeyAuthentication(
    +    $clientId: String!,
    +    $id: String!,
    +    $authentication: AuthenticationOptionsInput!,
    +    $nonce: String!,
    +    $domain: String,
    +    $sessionExpiration: String,
    +    $ua: String
    +  ) {
    +    verifyPasskeyAuthentication(
    +      clientId: $clientId,
    +      id: $id,
    +      authentication: $authentication,
    +      nonce: $nonce,
    +      domain: $domain,
    +      sessionExpiration: $sessionExpiration,
    +      ua: $ua
    +    ) {
    +      status
    +      idToken
    +      accessToken
    +      refreshToken
    +    }
    +  }
    +`
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: While using a multi-line template literal can improve readability, the current single-line format is still functional. The suggestion is more about style and readability, which are less critical than functional improvements.

    6

    Copy link

    Bundle Size

    Status File Size (Gzip) Limits
    index.js 14.29 KB (+35 B +0.24%) 15 kb

    Overall bundle size: 14.29 KB (+35 B +0.24%)
    Overall status: ✅

    @aversini aversini merged commit 405fdaa into main Aug 25, 2024
    4 checks passed
    @aversini aversini deleted the feat-passing-User-Agent-for-backend-to-parse-and-save branch August 25, 2024 17:28
    @aversini aversini mentioned this pull request Aug 25, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant