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: NetSuite Internal APIs #658

Merged
merged 7 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added apps/internal/__init__.py
Empty file.
30 changes: 30 additions & 0 deletions apps/internal/actions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from typing import Dict

from apps.netsuite.connector import NetSuiteConnector
from apps.workspaces.models import Workspace, NetSuiteCredentials


def get_netsuite_connection(query_params: Dict):
org_id = query_params.get('org_id')

workspace = Workspace.objects.get(fyle_org_id=org_id)
workspace_id = workspace.id
ns_credentials = NetSuiteCredentials.objects.get(workspace_id=workspace.id)

return NetSuiteConnector(netsuite_credentials=ns_credentials, workspace_id=workspace_id)
Comment on lines +7 to +14
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add input validation and error handling

The function has several critical issues that need to be addressed:

  1. Missing validation for required org_id parameter
  2. No error handling for database queries
  3. Direct use of org_id without validation could pose security risks

Consider implementing these improvements:

 def get_netsuite_connection(query_params: Dict):
-    org_id = query_params.get('org_id')
+    org_id = query_params.get('org_id')
+    if not org_id:
+        raise ValueError('org_id is required in query parameters')
+
+    try:
+        workspace = Workspace.objects.get(fyle_org_id=org_id)
+        ns_credentials = NetSuiteCredentials.objects.get(workspace_id=workspace.id)
+    except (Workspace.DoesNotExist, NetSuiteCredentials.DoesNotExist) as e:
+        raise ValueError(f'Invalid org_id or missing credentials: {str(e)}')
 
-    workspace = Workspace.objects.get(fyle_org_id=org_id)
     workspace_id = workspace.id
-    ns_credentials = NetSuiteCredentials.objects.get(workspace_id=workspace.id)
 
     return NetSuiteConnector(netsuite_credentials=ns_credentials, workspace_id=workspace_id)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def get_netsuite_connection(query_params: Dict):
org_id = query_params.get('org_id')
workspace = Workspace.objects.get(fyle_org_id=org_id)
workspace_id = workspace.id
ns_credentials = NetSuiteCredentials.objects.get(workspace_id=workspace.id)
return NetSuiteConnector(netsuite_credentials=ns_credentials, workspace_id=workspace_id)
def get_netsuite_connection(query_params: Dict):
org_id = query_params.get('org_id')
if not org_id:
raise ValueError('org_id is required in query parameters')
try:
workspace = Workspace.objects.get(fyle_org_id=org_id)
ns_credentials = NetSuiteCredentials.objects.get(workspace_id=workspace.id)
except (Workspace.DoesNotExist, NetSuiteCredentials.DoesNotExist) as e:
raise ValueError(f'Invalid org_id or missing credentials: {str(e)}')
workspace_id = workspace.id
return NetSuiteConnector(netsuite_credentials=ns_credentials, workspace_id=workspace_id)



def get_accounting_fields(query_params: Dict):
ns_connection = get_netsuite_connection(query_params)
resource_type = query_params.get('resource_type')
internal_id = query_params.get('internal_id')

return ns_connection.get_accounting_fields(resource_type, internal_id)
Comment on lines +17 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add parameter validation and error handling

The function needs validation for required parameters and proper error handling.

Implement these improvements:

 def get_accounting_fields(query_params: Dict):
+    required_params = ['resource_type', 'internal_id']
+    for param in required_params:
+        if not query_params.get(param):
+            raise ValueError(f'{param} is required in query parameters')
+
     ns_connection = get_netsuite_connection(query_params)
     resource_type = query_params.get('resource_type')
     internal_id = query_params.get('internal_id')
 
-    return ns_connection.get_accounting_fields(resource_type, internal_id)
+    try:
+        return ns_connection.get_accounting_fields(resource_type, internal_id)
+    except Exception as e:
+        raise ValueError(f'Failed to get accounting fields: {str(e)}')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def get_accounting_fields(query_params: Dict):
ns_connection = get_netsuite_connection(query_params)
resource_type = query_params.get('resource_type')
internal_id = query_params.get('internal_id')
return ns_connection.get_accounting_fields(resource_type, internal_id)
def get_accounting_fields(query_params: Dict):
required_params = ['resource_type', 'internal_id']
for param in required_params:
if not query_params.get(param):
raise ValueError(f'{param} is required in query parameters')
ns_connection = get_netsuite_connection(query_params)
resource_type = query_params.get('resource_type')
internal_id = query_params.get('internal_id')
try:
return ns_connection.get_accounting_fields(resource_type, internal_id)
except Exception as e:
raise ValueError(f'Failed to get accounting fields: {str(e)}')



def get_exported_entry(query_params: Dict):
ns_connection = get_netsuite_connection(query_params)
resource_type = query_params.get('resource_type')
internal_id = query_params.get('internal_id')

return ns_connection.get_exported_entry(resource_type, internal_id)
Comment on lines +25 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor to reduce code duplication and add validation

This function is nearly identical to get_accounting_fields. Consider refactoring to reduce duplication.

Here's a suggested implementation:

+def _validate_and_get_params(query_params: Dict, operation: str):
+    required_params = ['resource_type', 'internal_id']
+    for param in required_params:
+        if not query_params.get(param):
+            raise ValueError(f'{param} is required in query parameters')
+
+    ns_connection = get_netsuite_connection(query_params)
+    resource_type = query_params.get('resource_type')
+    internal_id = query_params.get('internal_id')
+    return ns_connection, resource_type, internal_id
+
 def get_exported_entry(query_params: Dict):
-    ns_connection = get_netsuite_connection(query_params)
-    resource_type = query_params.get('resource_type')
-    internal_id = query_params.get('internal_id')
+    ns_connection, resource_type, internal_id = _validate_and_get_params(
+        query_params, 'exported_entry'
+    )
 
-    return ns_connection.get_exported_entry(resource_type, internal_id)
+    try:
+        return ns_connection.get_exported_entry(resource_type, internal_id)
+    except Exception as e:
+        raise ValueError(f'Failed to get exported entry: {str(e)}')

Also update get_accounting_fields to use the new helper function:

 def get_accounting_fields(query_params: Dict):
-    ns_connection = get_netsuite_connection(query_params)
-    resource_type = query_params.get('resource_type')
-    internal_id = query_params.get('internal_id')
+    ns_connection, resource_type, internal_id = _validate_and_get_params(
+        query_params, 'accounting_fields'
+    )
 
-    return ns_connection.get_accounting_fields(resource_type, internal_id)
+    try:
+        return ns_connection.get_accounting_fields(resource_type, internal_id)
+    except Exception as e:
+        raise ValueError(f'Failed to get accounting fields: {str(e)}')

Committable suggestion skipped: line range outside the PR's diff.

6 changes: 6 additions & 0 deletions apps/internal/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from django.apps import AppConfig


class InternalConfig(AppConfig):
default_auto_field = 'django.db.models.BigAutoField'
name = 'apps.internal'
Empty file.
10 changes: 10 additions & 0 deletions apps/internal/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import itertools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed


Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo, drop that unused import like it's hot!

Listen up! That itertools import ain't being used nowhere in this joint. Let's keep our code clean and lean, know what I'm sayin'?

Here's the fix, straight from the streets of good coding practices:

-import itertools
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import itertools
🧰 Tools
🪛 Ruff

1-1: itertools imported but unused

Remove unused import: itertools

(F401)

from django.urls import path

from .views import AccountingFieldsView, ExportedEntryView

urlpatterns = [
path('accounting_fields/', AccountingFieldsView.as_view(), name='accounting-fields'),
path('exported_entry/', ExportedEntryView.as_view(), name='exported-entry'),
]
68 changes: 68 additions & 0 deletions apps/internal/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import logging
import traceback
from rest_framework import generics
from rest_framework.response import Response
from rest_framework import status

from apps.workspaces.permissions import IsAuthenticatedForInternalAPI

from fyle_netsuite_api.utils import assert_valid

from .actions import get_accounting_fields, get_exported_entry

logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)


class AccountingFieldsView(generics.GenericAPIView):
authentication_classes = []
permission_classes = [IsAuthenticatedForInternalAPI]
Comment on lines +18 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix contradictory authentication configuration

The empty authentication_classes = [] with IsAuthenticatedForInternalAPI permission is contradictory. Without an authentication class, there's no way to authenticate the request, making the permission check ineffective.

class AccountingFieldsView(generics.GenericAPIView):
-    authentication_classes = []
+    authentication_classes = [TokenAuthentication]  # or appropriate auth class
     permission_classes = [IsAuthenticatedForInternalAPI]

Committable suggestion skipped: line range outside the PR's diff.


def get(self, request, *args, **kwargs):
try:
params = request.query_params

assert_valid(params.get('org_id') is not None, 'Org ID is required')
assert_valid(params.get('resource_type') is not None, 'Resource Type is required')

if params.get('resource_type') in ('custom_segments', 'custom_lists', 'custom_record_types'):
assert_valid(params.get('internal_id') is not None, 'Internal ID is required')

response = get_accounting_fields(request.query_params)
return Response(
data={'data': response},
status=status.HTTP_200_OK
)

except Exception:
logger.info(f"Error in AccountingFieldsView: {traceback.format_exc()}")
return Response(
data={'error': traceback.format_exc()},
status=status.HTTP_400_BAD_REQUEST
)


class ExportedEntryView(generics.GenericAPIView):
authentication_classes = []
permission_classes = [IsAuthenticatedForInternalAPI]

def get(self, request, *args, **kwargs):
try:
params = request.query_params
assert_valid(params.get('org_id') is not None, 'Org ID is required')
assert_valid(params.get('resource_type') is not None, 'Resource Type is required')
assert_valid(params.get('internal_id') is not None, 'Internal ID is required')

Comment on lines +51 to +55
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add resource type validation and refactor validation logic

  1. The resource_type parameter should be validated against allowed values
  2. The validation logic is duplicated between views and could be refactored into a reusable function
+VALID_RESOURCE_TYPES = {'custom_segments', 'custom_lists', 'custom_record_types', ...}  # add all valid types
+
+def validate_params(params, require_internal_id=True):
+    assert_valid(params.get('org_id') is not None, 'Org ID is required')
+    assert_valid(params.get('resource_type') is not None, 'Resource Type is required')
+    assert_valid(
+        params.get('resource_type') in VALID_RESOURCE_TYPES,
+        f'Invalid resource type. Must be one of: {", ".join(VALID_RESOURCE_TYPES)}'
+    )
+    if require_internal_id:
+        assert_valid(params.get('internal_id') is not None, 'Internal ID is required')
+
 def get(self, request, *args, **kwargs):
     try:
         params = request.query_params
-        assert_valid(params.get('org_id') is not None, 'Org ID is required')
-        assert_valid(params.get('resource_type') is not None, 'Resource Type is required')
-        assert_valid(params.get('internal_id') is not None, 'Internal ID is required')
+        validate_params(params)

Committable suggestion skipped: line range outside the PR's diff.

response = get_exported_entry(request.query_params)

return Response(
data={'data': response},
status=status.HTTP_200_OK
)

except Exception:
logger.info(f"Error in AccountingFieldsView: {traceback.format_exc()}")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect logger message

The error message incorrectly references "AccountingFieldsView" instead of "ExportedEntryView".

-            logger.info(f"Error in AccountingFieldsView: {traceback.format_exc()}")
+            logger.info(f"Error in ExportedEntryView: {traceback.format_exc()}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logger.info(f"Error in AccountingFieldsView: {traceback.format_exc()}")
logger.info(f"Error in ExportedEntryView: {traceback.format_exc()}")

return Response(
data={'error': traceback.format_exc()},
status=status.HTTP_400_BAD_REQUEST
)
37 changes: 36 additions & 1 deletion apps/netsuite/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,42 @@ def sync_customers(self):
attributes, 'PROJECT', self.workspace_id, True)

return []


def get_accounting_fields(self, resource_type: str, internal_id: str):
"""
Retrieve accounting fields for a specific resource type and internal ID.

Args:
resource_type (str): The type of resource to fetch.
internal_id (str): The internal ID of the resource.

Returns:
list or dict: Parsed JSON representation of the resource data.
"""
module = getattr(self.connection, resource_type)
method_map = {
'currencies': 'get_all',
ruuushhh marked this conversation as resolved.
Show resolved Hide resolved
'custom_segments': 'get',
'custom_lists': 'get',
'custom_record_types': 'get_all_by_id',
}
method = method_map.get(resource_type, 'get_all_generator')

if method in ('get', 'get_all_by_id'):
response = getattr(module, method)(internal_id)
else:
response = getattr(module, method)()

if method == 'get_all_generator':
response = [row for responses in response for row in responses]

return json.loads(json.dumps(response, default=str))
Comment on lines +1166 to +1194
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and improve type safety.

The method needs several improvements for robustness and type safety:

  1. Add error handling for invalid resource types
  2. Add input validation for internal_id
  3. Add return type hint
  4. Replace getattr with direct attribute access for module

Here's the suggested implementation:

-    def get_accounting_fields(self, resource_type: str, internal_id: str):
+    def get_accounting_fields(self, resource_type: str, internal_id: str) -> Union[List, Dict]:
+        """
+        Retrieve accounting fields for a specific resource type and internal ID.
+
+        Args:
+            resource_type (str): The type of resource to fetch.
+            internal_id (str): The internal ID of the resource.
+
+        Returns:
+            Union[List, Dict]: Parsed JSON representation of the resource data.
+            
+        Raises:
+            ValueError: If resource_type is invalid or internal_id is empty.
+        """
+        if not resource_type:
+            raise ValueError('Resource type is required')
+        if not internal_id:
+            raise ValueError('Internal ID is required')
+
         module = getattr(self.connection, resource_type)
         method_map = {
             'currencies': 'get_all',
             'custom_segments': 'get',
             'custom_lists': 'get',
             'custom_record_types': 'get_all_by_id',
         }
-        method = method_map.get(resource_type, 'get_all_generator')
+        method_name = method_map.get(resource_type, 'get_all_generator')
+        method = getattr(module, method_name)

-        if method in ('get', 'get_all_by_id'):
-            response = getattr(module, method)(internal_id)
+        if method_name in ('get', 'get_all_by_id'):
+            response = method(internal_id)
         else:
-            response = getattr(module, method)()
+            response = method()

-        if method == 'get_all_generator':
+        if method_name == 'get_all_generator':
             response = [row for responses in response for row in responses]

         return json.loads(json.dumps(response, default=str))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def get_accounting_fields(self, resource_type: str, internal_id: str):
"""
Retrieve accounting fields for a specific resource type and internal ID.
Args:
resource_type (str): The type of resource to fetch.
internal_id (str): The internal ID of the resource.
Returns:
list or dict: Parsed JSON representation of the resource data.
"""
module = getattr(self.connection, resource_type)
method_map = {
'currencies': 'get_all',
'custom_segments': 'get',
'custom_lists': 'get',
'custom_record_types': 'get_all_by_id',
}
method = method_map.get(resource_type, 'get_all_generator')
if method in ('get', 'get_all_by_id'):
response = getattr(module, method)(internal_id)
else:
response = getattr(module, method)()
if method == 'get_all_generator':
response = [row for responses in response for row in responses]
return json.loads(json.dumps(response, default=str))
def get_accounting_fields(self, resource_type: str, internal_id: str) -> Union[List, Dict]:
"""
Retrieve accounting fields for a specific resource type and internal ID.
Args:
resource_type (str): The type of resource to fetch.
internal_id (str): The internal ID of the resource.
Returns:
Union[List, Dict]: Parsed JSON representation of the resource data.
Raises:
ValueError: If resource_type is invalid or internal_id is empty.
"""
if not resource_type:
raise ValueError('Resource type is required')
if not internal_id:
raise ValueError('Internal ID is required')
module = getattr(self.connection, resource_type)
method_map = {
'currencies': 'get_all',
'custom_segments': 'get',
'custom_lists': 'get',
'custom_record_types': 'get_all_by_id',
}
method_name = method_map.get(resource_type, 'get_all_generator')
method = getattr(module, method_name)
if method_name in ('get', 'get_all_by_id'):
response = method(internal_id)
else:
response = method()
if method_name == 'get_all_generator':
response = [row for responses in response for row in responses]
return json.loads(json.dumps(response, default=str))


def get_exported_entry(self, resource_type: str, export_id: str):
module = getattr(self.connection, resource_type)
response = getattr(module, 'get')(export_id)
return json.loads(json.dumps(response, default=str))
Comment on lines +1196 to +1199
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and improve type safety.

The method needs several improvements for robustness and type safety:

  1. Add error handling for invalid resource types
  2. Add input validation for export_id
  3. Add type hints
  4. Replace getattr with direct attribute access

Here's the suggested implementation:

-    def get_exported_entry(self, resource_type: str, export_id: str):
+    def get_exported_entry(self, resource_type: str, export_id: str) -> Dict:
+        """
+        Retrieve a specific exported entry.
+
+        Args:
+            resource_type (str): The type of resource to fetch.
+            export_id (str): The export ID of the entry.
+
+        Returns:
+            Dict: Parsed JSON representation of the exported entry.
+
+        Raises:
+            ValueError: If resource_type is invalid or export_id is empty.
+        """
+        if not resource_type:
+            raise ValueError('Resource type is required')
+        if not export_id:
+            raise ValueError('Export ID is required')
+
         module = getattr(self.connection, resource_type)
-        response = getattr(module, 'get')(export_id)
+        response = module.get(export_id)
         return json.loads(json.dumps(response, default=str))

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.8.0)

1198-1198: Do not call getattr with a constant attribute value. It is not any safer than normal property access.

Replace getattr with attribute access

(B009)


def handle_taxed_line_items(self, base_line, line, workspace_id, export_module, general_mapping: GeneralMapping):
"""
Handle line items where tax is applied or modified by the user.
Expand Down
6 changes: 3 additions & 3 deletions apps/workspaces/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ def has_permission(self, request, view):
workspace_users = Workspace.objects.filter(pk=workspace_id).values_list('user', flat=True)
return self.validate_and_cache(workspace_users, user, workspace_id, True)

class IsAuthenticatedForTest(permissions.BasePermission):
class IsAuthenticatedForInternalAPI(permissions.BasePermission):
"""
Custom auth for preparing a workspace for e2e tests
Custom auth for internal APIs
"""
def has_permission(self, request, view):
# Client sends a token in the header, which we decrypt and compare with the Client Secret
cipher_suite = Fernet(settings.ENCRYPTION_KEY)
try:
decrypted_password = cipher_suite.decrypt(request.headers['X-E2E-Tests-Client-ID'].encode('utf-8')).decode('utf-8')
decrypted_password = cipher_suite.decrypt(request.headers['X-Internal-API-Client-ID'].encode('utf-8')).decode('utf-8')
if decrypted_password == settings.E2E_TESTS_CLIENT_SECRET:
return True
Comment on lines +45 to 47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance security measures

The current implementation could be strengthened with additional security measures.

Consider these improvements:

  1. Add rate limiting to prevent brute force attempts
  2. Implement request timestamp validation to prevent replay attacks
  3. Add IP allowlisting for internal services

Example implementation:

from django.core.cache import cache
from django.utils import timezone
from rest_framework.exceptions import Throttled

class IsAuthenticatedForInternalAPI(permissions.BasePermission):
    def has_permission(self, request, view):
        # Rate limiting
        cache_key = f"internal_api_auth_{request.META.get('REMOTE_ADDR')}"
        attempts = cache.get(cache_key, 0)
        if attempts >= 5:  # 5 attempts per minute
            raise Throttled()
        cache.set(cache_key, attempts + 1, 60)  # 60 seconds expiry

        # IP allowlist check
        client_ip = request.META.get('REMOTE_ADDR')
        if client_ip not in settings.INTERNAL_API_ALLOWED_IPS:
            logger.warning(f'Unauthorized IP attempt: {client_ip}')
            return False

        # Existing authentication logic
        cipher_suite = Fernet(settings.ENCRYPTION_KEY)
        try:
            decrypted_password = cipher_suite.decrypt(
                request.headers['X-Internal-API-Client-ID'].encode('utf-8')
            ).decode('utf-8')
            
            if decrypted_password == settings.E2E_TESTS_CLIENT_SECRET:
                return True

except:
Expand Down
4 changes: 2 additions & 2 deletions apps/workspaces/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from apps.workspaces.actions import export_to_netsuite
from .serializers import LastExportDetailSerializer, WorkspaceSerializer, FyleCredentialSerializer, NetSuiteCredentialSerializer, \
ConfigurationSerializer, WorkspaceScheduleSerializer
from .permissions import IsAuthenticatedForTest
from .permissions import IsAuthenticatedForInternalAPI


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -484,7 +484,7 @@ class SetupE2ETestView(viewsets.ViewSet):
NetSuite Workspace
"""
authentication_classes = []
permission_classes = [IsAuthenticatedForTest]
permission_classes = [IsAuthenticatedForInternalAPI]

def post(self, request, **kwargs):
"""
Expand Down
1 change: 1 addition & 0 deletions fyle_netsuite_api/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
'apps.netsuite',
'django_q',
'django_filters',
'apps.internal'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Yo, we got a security situation that needs attention!

Listen up! Just spotted something that's making me lose myself in the security concerns. That AccountingFieldsView is running wild with no authentication or permissions - that's like leaving the door wide open at 8 Mile!

  • apps/internal/views.py: View's got empty authentication and permission classes:
authentication_classes = []
permission_classes = []
  • Exposing accounting fields at /accounting_fields/ without any security checks
  • No middleware protection in sight

Better lock this down before someone takes advantage, know what I'm saying?
You need to:

  1. Add proper authentication classes
  2. Set up permission classes
  3. Consider adding request validation

Don't let this internal app be your security's kryptonite!

🔗 Analysis chain

Yo, let's verify this internal app integration!

Listen up! Before we drop this new app into production, we gotta make sure it's legit. Here's what's on my mind:

Straight from the underground, here's what you need to know:

  1. Make sure this internal app got proper authentication lockdown
  2. Keep them API endpoints restricted to admin users only
  3. Document this app's purpose in the README, don't leave us hanging!

Yo, and one more thing - consider adding some middleware specifically for this internal app if you're exposing sensitive operations. Keep it secure, keep it clean!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Spit some facts about this internal app structure
echo "Checking if this internal app is properly structured..."
fd -t f "^__init__.py|^apps.py|^models.py|^views.py|^urls.py$" "apps/internal"

# Look for any sensitive stuff that might be exposed
echo "Scanning for potential security gotchas..."
rg -g "apps/internal/**" -i "password|secret|key|token|credential"

Length of output: 816


Script:

#!/bin/bash
# Yo, let's check what security we got in these views
echo "Dropping bars about view security..."
rg -g "apps/internal/views.py" -A 5 "class|def"

# Check if we're using any authentication decorators
echo "Checking authentication flow..."
rg -g "apps/internal/views.py" "@.*_required|permission_required|login_required"

# Let's see what URLs we're exposing
echo "Scanning URL patterns..."
rg -g "apps/internal/urls.py" "path|url"

# Check if we got any middleware specifically for this
echo "Looking for custom middleware..."
rg -g "apps/internal/**" "class.*Middleware"

Length of output: 1288

]

MIDDLEWARE = [
Expand Down
1 change: 1 addition & 0 deletions fyle_netsuite_api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@
path('api/workspaces/', include('apps.workspaces.urls')),
path('api/user/', include('apps.users.urls')),
path('api/v2/workspaces/', include('apps.workspaces.apis.urls')),
path('api/internal/', include('apps.internal.urls')),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Yo, this endpoint's more exposed than 8 Mile Road! Security's MIA!

Listen up, I'm dropping truth bombs here:

  • Your AccountingFieldsView is running wild with empty security lists:
    authentication_classes = []
    permission_classes = []
  • That's like leaving the door wide open, no bouncer, no security, everybody getting in!
  • You're exposing accounting data through /api/internal/ without any authentication checks

You better fix this before it goes south:

  • Add proper authentication classes (like TokenAuthentication or SessionAuthentication)
  • Set up permission classes (maybe IsAuthenticated or custom permissions)
  • Consider adding rate limiting to prevent abuse

This ain't a freestyle session where anything goes - lock it down!

🔗 Analysis chain

Yo, hold up! Let's talk about securing these internal APIs!

Listen up, we're exposing internal endpoints here, and that's some serious business. We better make sure these routes are locked down tight with proper authentication and authorization.

Let me drop these bars to check for any security middleware:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Yo, let's check if we got security measures in place
echo "Checking for authentication middleware..."
rg -l "AuthenticationMiddleware|permission_classes|authentication_classes" apps/internal/

echo "Looking for security decorators..."
ast-grep --pattern 'class $_ {
  $$$
  @permission_required($$$)
  $$$
}'

Length of output: 362


Script:

#!/bin/bash
# Yo, let's dive deeper into these security measures
echo "Checking the actual security implementation in views..."
rg -A 5 "permission_classes|authentication_classes" apps/internal/views.py

echo "\nLooking for any middleware configuration..."
rg -l "MIDDLEWARE" fyle_netsuite_api/settings/

echo "\nChecking what's in those middleware settings..."
for f in $(fd -t f . fyle_netsuite_api/settings/); do
    echo "\nFile: $f"
    rg "MIDDLEWARE.*=.*\[" -A 10 "$f"
done

echo "\nLet's see what's in those internal views..."
cat apps/internal/views.py

Length of output: 1918

]
42 changes: 42 additions & 0 deletions tests/test_internal/test_actions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
from apps.internal.actions import get_accounting_fields, get_exported_entry
from tests.test_netsuite.fixtures import data

def test_get_accounting_fields(db, mocker):
query_params = {
'org_id': 'or79Cob97KSh',
'resource_type': 'employees',
}
mocker.patch(
'netsuitesdk.api.employees.Employees.get_all_generator',
return_value=data['get_all_employees']
)

mocker.patch('netsuitesdk.api.currencies.Currencies.get_all')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix empty mock for Currencies.get_all

The mock for Currencies.get_all is missing a return value, which could lead to unexpected behavior.

-    mocker.patch('netsuitesdk.api.currencies.Currencies.get_all')
+    mocker.patch(
+        'netsuitesdk.api.currencies.Currencies.get_all',
+        return_value=data['get_all_currencies']
+    )

Committable suggestion skipped: line range outside the PR's diff.


mocker.patch(
'netsuitesdk.api.custom_lists.CustomLists.get',
return_value=data['get_custom_list']
)

fields = get_accounting_fields(query_params)
assert fields is not None

query_params['resource_type'] = 'custom_lists'
query_params['internal_id'] = '1'
fields = get_accounting_fields(query_params)
assert fields is not None

Comment on lines +4 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add docstring and improve test assertions

The test function would benefit from:

  1. A docstring explaining the test scenarios
  2. Assertions on the actual content of returned fields
  3. Using a fixture for org_id instead of hardcoding
 def test_get_accounting_fields(db, mocker):
+    """
+    Test get_accounting_fields action with different resource types:
+    1. Test with employees resource type
+    2. Test with custom_lists resource type
+    """
     query_params = {
-        'org_id': 'or79Cob97KSh',
+        'org_id': pytest.fixture('org_id'),
         'resource_type': 'employees',
     }

Committable suggestion skipped: line range outside the PR's diff.


def test_get_exported_entry(db, mocker):
query_params = {
'org_id': 'or79Cob97KSh',
'resource_type': 'vendor_bills',
'internal_id': '1'
}
mocker.patch(
'netsuitesdk.api.vendor_bills.VendorBills.get',
return_value={'summa': 'hehe'}
)

entry = get_exported_entry(query_params)
assert entry is not None
Comment on lines +30 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve test data and assertions

Several improvements needed:

  1. The mock data uses placeholder values ('summa': 'hehe') instead of realistic test data
  2. The assertion only checks if entry is not None, but should verify the structure and content
  3. org_id and internal_id should use fixtures

Here's how to improve it:

 def test_get_exported_entry(db, mocker):
+    """
+    Test get_exported_entry action retrieves correct vendor bill data
+    """
     query_params = {
-        'org_id': 'or79Cob97KSh',
-        'resource_type': 'vendor_bills',
-        'internal_id': '1'
+        'org_id': pytest.fixture('org_id'),
+        'resource_type': 'vendor_bills',
+        'internal_id': pytest.fixture('vendor_bill_id')
     }
+    expected_vendor_bill = {
+        'internalId': '1',
+        'tranId': 'BILL#123',
+        'amount': 100.00,
+        'status': 'PENDING'
+    }
     mocker.patch(
         'netsuitesdk.api.vendor_bills.VendorBills.get',
-        return_value={'summa': 'hehe'}
+        return_value=expected_vendor_bill
     )

     entry = get_exported_entry(query_params)
-    assert entry is not None
+    assert entry == expected_vendor_bill
+    assert 'internalId' in entry
+    assert 'tranId' in entry
+    assert 'amount' in entry
+    assert 'status' in entry

Committable suggestion skipped: line range outside the PR's diff.

61 changes: 61 additions & 0 deletions tests/test_internal/test_views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import pytest
from unittest.mock import patch
from django.urls import reverse

from apps.workspaces.permissions import IsAuthenticatedForInternalAPI

from tests.test_netsuite.fixtures import data


@pytest.mark.django_db(databases=['default'])
@patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True)
def test_netsutie_fields_view(db, api_client, mocker):
url = reverse('accounting-fields')

response = api_client.get(url)
assert response.status_code == 400

response = api_client.get(url, {'org_id': 'or79Cob97KSh'})
assert response.status_code == 400

response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'custom_segments'})
assert response.status_code == 400

mocker.patch(
'netsuitesdk.api.custom_lists.CustomLists.get',
return_value=data['get_custom_list']
)

response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'custom_lists', 'internal_id': '1'})
assert response.status_code == 200

mocker.patch(
'netsuitesdk.api.employees.Employees.get_all_generator',
return_value=data['get_all_employees']
)

response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'employees'})
assert response.status_code == 200
Comment on lines +10 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage with additional assertions

The test could be more comprehensive:

  1. Verify the structure and content of the response data for successful requests
  2. Test error scenarios with invalid resource_types
  3. Test error handling when NetSuite SDK calls fail

Consider adding:

# Test invalid resource type
response = api_client.get(url, {
    'org_id': TEST_ORG_ID,
    'resource_type': 'invalid_type'
})
assert response.status_code == 400

# Verify response structure
response = api_client.get(url, {
    'org_id': TEST_ORG_ID,
    'resource_type': 'employees'
})
assert response.status_code == 200
assert 'data' in response.json()
assert isinstance(response.json()['data'], list)

# Test SDK error handling
mocker.patch(
    'netsuitesdk.api.employees.Employees.get_all_generator',
    side_effect=Exception('NetSuite API error')
)
response = api_client.get(url, {
    'org_id': TEST_ORG_ID,
    'resource_type': 'employees'
})
assert response.status_code == 500



@pytest.mark.django_db(databases=['default'])
@patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True)
def test_exported_entry_view(db, api_client, mocker):
url = reverse('exported-entry')

response = api_client.get(url)
assert response.status_code == 400

response = api_client.get(url, {'org_id': 'or79Cob97KSh'})
assert response.status_code == 400

response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'vendor_bills'})
assert response.status_code == 400

mocker.patch(
'netsuitesdk.api.vendor_bills.VendorBills.get',
return_value={'summa': 'hehe'}
)
Comment on lines +55 to +58
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use representative test data

The mock return value {'summa': 'hehe'} is not representative of actual vendor bill data. Consider using realistic test data that matches the NetSuite API response structure.

-        return_value={'summa': 'hehe'}
+        return_value={
+            'internalId': '1',
+            'tranId': 'BILL123',
+            'total': 100.00,
+            'status': 'PENDING_APPROVAL',
+            'memo': 'Test Bill'
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mocker.patch(
'netsuitesdk.api.vendor_bills.VendorBills.get',
return_value={'summa': 'hehe'}
)
mocker.patch(
'netsuitesdk.api.vendor_bills.VendorBills.get',
return_value={
'internalId': '1',
'tranId': 'BILL123',
'total': 100.00,
'status': 'PENDING_APPROVAL',
'memo': 'Test Bill'
}
)


response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'vendor_bills', 'internal_id': '1'})
assert response.status_code == 200
Comment on lines +41 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage for exported entry view

Similar to the previous test, this one could benefit from:

  1. Verifying response data structure
  2. Testing different resource types
  3. Testing error handling for SDK failures

Consider adding:

# Test different resource types
for resource_type in ['vendor_bills', 'expense_reports', 'journal_entries']:
    mocker.patch(
        f'netsuitesdk.api.{resource_type}.{resource_type.title().replace("_", "")}.get',
        return_value=data[f'get_{resource_type}']
    )
    response = api_client.get(url, {
        'org_id': TEST_ORG_ID,
        'resource_type': resource_type,
        'internal_id': '1'
    })
    assert response.status_code == 200
    assert 'data' in response.json()
    
# Test SDK error handling
mocker.patch(
    'netsuitesdk.api.vendor_bills.VendorBills.get',
    side_effect=Exception('NetSuite API error')
)
response = api_client.get(url, {
    'org_id': TEST_ORG_ID,
    'resource_type': 'vendor_bills',
    'internal_id': '1'
})
assert response.status_code == 500

Loading