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.

Sign up for GitHub

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

fix: auth schemes #1226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: auth schemes #1226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b1061b
fix: auth schemes
Devanshusisodiya Jan 26, 2025
277eae9
test: added test
Devanshusisodiya Jan 26, 2025
8524a9e
chore: fmt chk
Devanshusisodiya Jan 27, 2025
ec3e8a4
fix: added exception handling
Devanshusisodiya Jan 28, 2025
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
8 changes: 6 additions & 2 deletions python/composio/cli/add.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,12 +238,16 @@ def add_integration(
AuthSchemeType,
click.prompt(
"Select auth mode: ",
type=click.Choice(choices=list(auth_modes)),
type=click.Choice(choices=t.cast(t.List[str], auth_modes)),
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
),
)
auth_scheme = auth_modes[auth_mode]

if auth_mode.lower() in ("basic", "api_key", "bearer_token"):
if auth_mode is not None and auth_mode.lower() in (
"basic",
"api_key",
"bearer_token",
):
Comment on lines 245 to +250
Copy link

Choose a reason for hiding this comment

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

Missing None check for auth_mode before accessing lower() could cause AttributeError. Should check auth_mode is not None before calling lower().

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if auth_mode.lower() in ("basic", "api_key", "bearer_token"):
if auth_mode is not None and auth_mode.lower() in (
"basic",
"api_key",
"bearer_token",
):
if auth_mode is not None and auth_mode.lower() in (
"basic",
"api_key",
"bearer_token",
):

Comment on lines 245 to +250
Copy link

Choose a reason for hiding this comment

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

Missing None check for auth_mode before accessing lower() could cause AttributeError. Should check auth_mode is not None before calling lower().

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if auth_mode.lower() in ("basic", "api_key", "bearer_token"):
if auth_mode is not None and auth_mode.lower() in (
"basic",
"api_key",
"bearer_token",
):
if auth_mode is not None and auth_mode.lower() in (
"basic",
"api_key",
"bearer_token",
):

return _handle_basic_auth(
entity=entity,
app_name=name,
Expand Down
21 changes: 17 additions & 4 deletions python/composio/client/collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ class AuthSchemeField(BaseModel):
class AppAuthScheme(BaseModel):
"""App authenticatio scheme."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a typo in the docstring: authenticatio should be authentication


scheme_name: str
auth_mode: AuthSchemeType
scheme_name: t.Optional[str] = None
name: t.Optional[str] = None
auth_mode: t.Optional[AuthSchemeType] = None
mode: t.Optional[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding a docstring to explain the purpose of the mode field and its relationship with auth_mode. This would help clarify why both fields exist and when each should be used.

Comment on lines +296 to +299
Copy link

Choose a reason for hiding this comment

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

The auth_mode field is made optional but there is no handling for when both auth_mode and mode are None, which could cause runtime errors when accessing these fields later.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
scheme_name: t.Optional[str] = None
name: t.Optional[str] = None
auth_mode: t.Optional[AuthSchemeType] = None
mode: t.Optional[str] = None
scheme_name: t.Optional[str] = None
name: t.Optional[str] = None
auth_mode: AuthSchemeType = AuthSchemeType.NONE
mode: str = 'none'

fields: t.List[AuthSchemeField]

proxy: t.Optional[t.Dict] = None
Expand Down Expand Up @@ -342,8 +344,13 @@ def get(self) -> t.List[AppModel]:
def get(self, name: t.Optional[str] = None) -> AppModel:
"""Get a specific app."""

def get(self, name: t.Optional[str] = None) -> t.Union[AppModel, t.List[AppModel]]:
def get(
self,
name: t.Optional[str] = None,
) -> t.Union[AppModel, t.List[AppModel]]:
"""Get apps."""
queries = {"additionalFields": "auth_schemes,test_connectors"}

if name is not None:
return self.model(
**self._raise_if_required(
Expand All @@ -353,7 +360,13 @@ def get(self, name: t.Optional[str] = None) -> t.Union[AppModel, t.List[AppModel
).json()
)

return super().get(queries={})
apps = super().get(queries=queries)
for app in apps:
if app.auth_schemes is not None:
for auth_scheme in app.auth_schemes:
if auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)
Comment on lines +367 to +368
Copy link

Choose a reason for hiding this comment

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

The code assumes auth_scheme.mode exists before checking it, which could raise AttributeError if auth_scheme is None. Should check auth_scheme first.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)
if auth_scheme is not None and auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)

Comment on lines +367 to +368
Copy link

Choose a reason for hiding this comment

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

The code assumes auth_scheme.mode exists before checking it, which could raise AttributeError if auth_scheme is None. Should check auth_scheme first.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)
if auth_scheme is not None and auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)

return apps
Comment on lines +363 to +369
Copy link

Choose a reason for hiding this comment

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

The mode field is used to set auth_mode but the original scheme_name is not mapped to name, causing potential data inconsistency when both old and new field names are used.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
apps = super().get(queries=queries)
for app in apps:
if app.auth_schemes is not None:
for auth_scheme in app.auth_schemes:
if auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)
return apps
apps = super().get(queries=queries)
for app in apps:
if app.auth_schemes is not None:
for auth_scheme in app.auth_schemes:
if auth_scheme.mode is not None:
auth_scheme.auth_mode = t.cast(AuthSchemeType, auth_scheme.mode)
auth_scheme.name = auth_scheme.scheme_name
return apps



class TypeModel(BaseModel):
Expand Down
11 changes: 10 additions & 1 deletion python/composio/tools/toolset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ def get_apps(
no_auth: t.Optional[bool] = None,
include_local: bool = True,
) -> t.List[AppModel]:
# added type ignore since method overload was not being referenced
Copy link
Collaborator

Choose a reason for hiding this comment

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

The type ignore comment should explain why the method overload is not being referenced and what's the proper way to fix it. This would help future maintainers understand if this is a temporary workaround or a permanent solution.

apps = self.client.apps.get()
if no_auth is not None:
apps = [a for a in apps if a.no_auth is no_auth]
Expand Down Expand Up @@ -1051,6 +1052,10 @@ def get_expected_params_for_user(
# without user inputs to create an integratuib, if yes then create
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a typo in the comment: integratuib should be integration

# an integration and return params from there.
for scheme in app_data.auth_schemes or []:
if scheme.auth_mode is None:
raise ComposioSDKError(
message=f"No auth scheme found for app `{app_data.name}`"
)
if auth_scheme is not None and auth_scheme != scheme.auth_mode.upper():
continue
if self._can_use_auth_scheme_without_user_input(
Expand Down Expand Up @@ -1083,6 +1088,10 @@ def fetch_expected_integration_params(
) -> t.List[AuthSchemeField]:
"""Fetch expected integration params for creating an integration."""
for scheme in app.auth_schemes or []:
if scheme.auth_mode is None:
raise ComposioSDKError(
message=f"No auth scheme found for app `{app.name}`"
)
if auth_scheme != scheme.auth_mode.upper():
continue
return [f for f in scheme.fields if not f.expected_from_customer]
Expand Down Expand Up @@ -1154,7 +1163,7 @@ def initiate_connection(
).id
except NoItemsFound:
auth_config, use_composio_auth = self._validate_auth_config(
app, auth_scheme, auth_config
app, t.cast(AuthSchemeType, auth_scheme), auth_config
)
integration = self.create_integration(
app=app,
Expand Down
13 changes: 13 additions & 0 deletions python/tests/test_tools/test_toolset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from pydantic import BaseModel, Field

from composio import Action, App
from composio.client.collections import AUTH_SCHEMES
from composio.exceptions import ApiKeyNotProvidedError, ComposioSDKError
from composio.tools.base.abs import action_registry, tool_registry
from composio.tools.base.runtime import action as custom_action
Expand Down Expand Up @@ -67,6 +68,18 @@ def test_uninitialize_app() -> None:
ComposioToolSet().get_action_schemas(actions=[Action.ATTIO_UPDATE_A_LIST])


def test_get_apps() -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle the case where no app with no_auth set to False is found to avoid potential UnboundLocalError.

toolset = ComposioToolSet()
apps = toolset.get_apps()
for app in apps:
if app.no_auth is False:
auth_app = app
break
if auth_app.auth_schemes:
for auth_scheme in auth_app.auth_schemes:
assert auth_scheme.auth_mode in AUTH_SCHEMES


class TestValidateTools:
toolset: ComposioToolSet
package = "somepackage1"
Expand Down
Loading