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 5 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.
18 changes: 18 additions & 0 deletions apps/internal/actions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
from typing import Dict

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


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

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

ns_connection = NetSuiteConnector(netsuite_credentials=ns_credentials, workspace_id=workspace_id)

return ns_connection.get_accounting_fields(resource_type, internal_id)
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.
9 changes: 9 additions & 0 deletions apps/internal/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
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

urlpatterns = [
path('accounting_fields/', AccountingFieldsView.as_view(), name='accounting-fields'),
]
41 changes: 41 additions & 0 deletions apps/internal/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
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

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()}")
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
)
32 changes: 31 additions & 1 deletion apps/netsuite/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -1162,7 +1162,37 @@ 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 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

]
Loading