Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
6 changes: 6 additions & 0 deletions backend/aci/common/schemas/security_scheme.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ class OAuth2Scheme(BaseModel):
redirect_url: str | None = Field(
default=None, min_length=1, max_length=2048, description="Redirect URL for OAuth2 callback."
)
exchange_token_url: str | None = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of adding exchange_token_url directly, which is very rare and only applies to Instagram, i'm thinking maybe we should have a custom_data (or some other better name) dict, and allow people to put any additional data that is required but specific to that App they're integrating.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense! How about we hardcode exchange_token_url for Instagram for now? If we run into a similar case with other integrations in the future, we could really consider introducing the custom_data approach you mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

it will take quite a lot effort to make it backward compatible if we change this in the future, so rather do it now with a generic custom dict

Copy link
Author

Choose a reason for hiding this comment

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

no probelm, will add it now

default=None,
min_length=1,
max_length=2048,
description="URL for exchanging short-lived tokens to long-lived tokens (Instagram specific)",
)


# NOTE: need to show these fields for custom oauth2 app feature.
Expand Down
62 changes: 61 additions & 1 deletion backend/aci/server/oauth2_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ def __init__(
access_token_url: str,
refresh_token_url: str,
token_endpoint_auth_method: str | None = None,
exchange_token_url: str | None = None,
):
"""
Initialize the OAuth2Manager
Expand All @@ -39,6 +40,7 @@ def __init__(
token_endpoint_auth_method:
client_secret_basic (default) | client_secret_post | none
Additional options can be achieved by registering a custom auth method
exchange_token_url: The URL for exchanging short-lived tokens to long-lived tokens (Instagram specific)
"""
self.app_name = app_name
self.client_id = client_id
Expand All @@ -48,6 +50,7 @@ def __init__(
self.access_token_url = access_token_url
self.refresh_token_url = refresh_token_url
self.token_endpoint_auth_method = token_endpoint_auth_method
self.exchange_token_url = exchange_token_url

# TODO: need to close the client after use
# Add an aclose() helper (or implement __aenter__/__aexit__) and make callers invoke it during shutdown.
Expand Down Expand Up @@ -161,7 +164,48 @@ async def refresh_token(
logger.error(f"Failed to refresh access token, app_name={self.app_name}, error={e}")
raise OAuth2Error("Failed to refresh access token") from e

def parse_fetch_token_response(self, token: dict) -> OAuth2SchemeCredentials:
async def exchange_short_lived_token(self, short_lived_token: str) -> dict[str, Any]:
"""
Exchange short-lived access token for long-lived access token.
This is specific to Instagram's API requirements.

Args:
short_lived_token: The short-lived access token from the initial OAuth flow

Returns:
Token response dictionary with long-lived access token
"""
if self.app_name != "INSTAGRAM":
raise OAuth2Error("Token exchange is only supported for Instagram")

if not self.exchange_token_url:
raise OAuth2Error("Exchange token URL is not configured for Instagram")

try:
response = await self.oauth2_client.get(
self.exchange_token_url,
params={
"grant_type": "ig_exchange_token",
"client_secret": self.client_secret,
"access_token": short_lived_token,
},
timeout=30.0,
)
response.raise_for_status()

token_data = cast(dict[str, Any], response.json())
logger.info(
f"Successfully exchanged short-lived token for long-lived token, app_name={self.app_name}"
)
return token_data

except Exception as e:
logger.error(
f"Failed to exchange short-lived token, app_name={self.app_name}, error={e}"
)
raise OAuth2Error("Failed to exchange short-lived token for long-lived token") from e

async def parse_fetch_token_response(self, token: dict) -> OAuth2SchemeCredentials:
"""
Parse OAuth2SchemeCredentials from token response with app-specific handling.

Expand All @@ -181,6 +225,22 @@ def parse_fetch_token_response(self, token: dict) -> OAuth2SchemeCredentials:
logger.error(f"Missing authed_user in Slack OAuth response, app={self.app_name}")
raise OAuth2Error("Missing access_token in Slack OAuth response")

# handle Instagram's special case - exchange short-lived token for long-lived token
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the exchange short lived token for long live access token should happen here (parse_fetch_token_response)?
Ideally this should happen right after we get the short lived token.
Should we:

  • in fetch_token, we do special handling for instagram authorization (get short lived token + access token)
  • then we can parse the response from above normally with parse_fetch_token_response (might need special handling there as well, because instagram use existing access_token for refresh request instead of a separate refresh_token. And we need to store the expires_at for the access_token as well, which should probably 2 days before 60 days time line just to have some safe margin.)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I'll move the Instagram-specific handling logic into fetch_token.

As for the second suggestion, I don't think we'll refresh the Instagram access_token, since we can't guarantee it's still valid at refresh time — even with a safe margin. My initial thought was similar to yours, but in practice, we only validate the token when the user tries to perform Instagram-related actions.
So if we don't refresh it, just storing the original expires_at is fine. And if we do refresh, we still can't be sure the token remains valid despite the margin.

Or is there any other consideration for having a safe margin even if we don't refresh the access_token?

Copy link
Author

Choose a reason for hiding this comment

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

Hey, we could also add a refresh process for Instagram. If the access token has already expired when the refresh is triggered, the backend will return an error and prompt the user to reauthorize. As you suggested, we need to set the expires_at to be 2 days earlier than the 60-day limit.
Which approach do you think would work better?

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to consider both cases, because we don't know at what time the expiration check will happen:

  • when token is near expiration, for example (we set the threshhold to) 1 day before expiration: then we refresh it
  • when token is already expired (per instagram time): then there is no way to refresh, so we have to ask user to re-authorize.

I don't quite get - "since we can't guarantee it's still valid at refresh time — even with a safe margin."
we know the access token's expiration date, so why would we not know if that (long live) access token is expired or not? We only refresh if it's still valid at check time, if no longer valid at check time we ask for re-authorize

Copy link
Author

Choose a reason for hiding this comment

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

just to explain it: I thought that we must guarantee the long-lived access token valid and avoid the error when refreshing lol.
anyway, will update as you suggested, actually I also think it's a better solution.

if self.app_name == "INSTAGRAM":
if "access_token" in data:
short_lived_token = data["access_token"]
logger.info(
f"Exchanging short-lived token for long-lived token, app_name={self.app_name}"
)
long_lived_token_response = await self.exchange_short_lived_token(short_lived_token)
# Update data with long-lived token response: add expires_in and token_type, update access_token
data.update(long_lived_token_response)
else:
logger.error(
f"Missing access_token in Instagram OAuth response, app={self.app_name}"
)
raise OAuth2Error("Missing access_token in Instagram OAuth response")

if "access_token" not in data:
logger.error(f"Missing access_token in OAuth response, app={self.app_name}")
raise OAuth2Error("Missing access_token in OAuth response")
Expand Down
9 changes: 8 additions & 1 deletion backend/aci/server/routes/linked_accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ async def link_oauth2_account(
access_token_url=oauth2_scheme.access_token_url,
refresh_token_url=oauth2_scheme.refresh_token_url,
token_endpoint_auth_method=oauth2_scheme.token_endpoint_auth_method,
exchange_token_url=oauth2_scheme.exchange_token_url,
)

path = request.url_for(LINKED_ACCOUNTS_OAUTH2_CALLBACK_ROUTE_NAME).path
Expand Down Expand Up @@ -433,6 +434,11 @@ async def linked_accounts_oauth2_callback(

# check for state
state_jwt = request.query_params.get("state")
# Special handling for Instagram: remove #_ suffix if present
if state_jwt and state_jwt.endswith("#_"):
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why this special handling is needed?

Copy link
Author

Choose a reason for hiding this comment

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

image

This is another difference of instagram from other apps, fyi: business-login

Copy link
Contributor

Choose a reason for hiding this comment

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

can you verify it? because it says code in the screenshot, but you're getting that from the state

Copy link
Author

Choose a reason for hiding this comment

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

I have verified it before and the #_ will append to the end of the redirect url instead of the end of the parameter code.

Here is a real url showing in the address bar when trying to get the access token: https://8c2912846817.ngrok-free.app/v1/linked-accounts/oauth2/callback?code=AQBA7HITpIFnpYTWMW-lrx6GmFBJc44dWYMnqrpKVfL8_XKKyvekba6gRsmjis_yBfyKodVMsWYryfAcDRNrkDQ2e7i8ZIct1OzHP2jbvNya0XmMT533krdgWaJT7f3rnYarAUgxHlz8Qd95HTjrSlgVpWyKqLbDFDxWL2Gd8DPJ4Iyf8_OzWK_9PFNsaGXW9ppOf5yKGPmRxVpIKbazC7VGCiiUwWBSMFDKcpw7f_cRA&state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJwcm9qZWN0X2lkIjoiNjJhZmU3YWMtYzZjMS00YzQ5LTlhMzEtM2JhNWIwMDBhMTdlIiwiYXBwX25hbWUiOiJJTlNUQUdSQU0iLCJsaW5rZWRfYWNjb3VudF9vd25lcl9pZCI6Inl1ZXR3byIsImNsaWVudF9pZCI6IjI5NTU5NTk0NDQ1ODM3OTUiLCJyZWRpcmVjdF91cmkiOiJodHRwczovLzhjMjkxMjg0NjgxNy5uZ3Jvay1mcmVlLmFwcC92MS9saW5rZWQtYWNjb3VudHMvb2F1dGgyL2NhbGxiYWNrIiwiY29kZV92ZXJpZmllciI6IklpRzJGQVFxZzZKaU1TVTZqVWduUHZMbE0wVlJTYTVIZXllN2VCYWNTakJzNkR0SSIsImFmdGVyX29hdXRoMl9saW5rX3JlZGlyZWN0X3VybCI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMC9hcHBjb25maWdzL0lOU1RBR1JBTSJ9.e0cLM1QXwEZJUd6PhRia8BQMrz6jb_PsVeLqR9DO7q0#_

but I still added some log just now and also reauthorized. I found there is no need to make any special handling since everything after #_ won't pass to backend for some reason.
will remove the related code.
image

state_jwt = state_jwt[:-2] # Remove the last 2 characters (#_)
logger.info("Removed Instagram #_ suffix from state")

if not state_jwt:
logger.error(
"OAuth2 account linking callback received, missing state",
Expand Down Expand Up @@ -501,14 +507,15 @@ async def linked_accounts_oauth2_callback(
access_token_url=oauth2_scheme.access_token_url,
refresh_token_url=oauth2_scheme.refresh_token_url,
token_endpoint_auth_method=oauth2_scheme.token_endpoint_auth_method,
exchange_token_url=oauth2_scheme.exchange_token_url,
)

token_response = await oauth2_manager.fetch_token(
redirect_uri=state.redirect_uri,
code=code,
code_verifier=state.code_verifier,
)
security_credentials = oauth2_manager.parse_fetch_token_response(token_response)
security_credentials = await oauth2_manager.parse_fetch_token_response(token_response)

# if the linked account already exists, update it, otherwise create a new one
# TODO: consider separating the logic for updating and creating a linked account or give warning to clients
Expand Down
13 changes: 13 additions & 0 deletions backend/aci/server/security_credentials_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
OAuth2SchemeCredentials,
SecuritySchemeOverrides,
)
from aci.server import config
from aci.server.oauth2_manager import OAuth2Manager

logger = get_logger(__name__)
Expand Down Expand Up @@ -96,6 +97,17 @@ async def _get_oauth2_credentials(
linked_account.security_credentials
)
if _access_token_is_expired(oauth2_scheme_credentials):
if app.name == "INSTAGRAM":
Copy link
Contributor

Choose a reason for hiding this comment

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

just make sure to test if re-autorize works as expected when you're doing e2e testing

Copy link
Author

Choose a reason for hiding this comment

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

image image

This is the dev version fyi.

logger.error(
f"Access token expired, please re-authorize, linked_account_id={linked_account.id}, "
f"security_scheme={linked_account.security_scheme}, app={app.name}"
)
# NOTE: this error message could be used by the frontend to guide the user to re-authorize
raise OAuth2Error(
f"Access token expired. Please re-authorize at: "
f"{config.DEV_PORTAL_URL}/appconfigs/{app.name}"
)

logger.warning(
f"Access token expired, trying to refresh linked_account_id={linked_account.id}, "
f"security_scheme={linked_account.security_scheme}, app={app.name}"
Expand Down Expand Up @@ -155,6 +167,7 @@ async def _refresh_oauth2_access_token(
scope=oauth2_scheme_credentials.scope,
authorize_url=oauth2_scheme.authorize_url,
access_token_url=oauth2_scheme.access_token_url,
exchange_token_url=oauth2_scheme.exchange_token_url,
refresh_token_url=oauth2_scheme.refresh_token_url,
token_endpoint_auth_method=oauth2_scheme.token_endpoint_auth_method,
)
Expand Down
27 changes: 27 additions & 0 deletions backend/apps/instagram/app.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "INSTAGRAM",
"display_name": "Instagram",
"logo": "https://raw.githubusercontent.com/aipotheosis-labs/aipolabs-icons/refs/heads/main/apps/instagram.svg",
"provider": "Meta Platforms, Inc.",
"version": "1.0.0",
"description": "The Instagram API allows developers to access and manage Instagram resources programmatically. It provides functionality for publishing content, retrieving user information, fetching post data, tracking user feeds, and managing direct messages through RESTful HTTP calls.",
"security_schemes": {
"oauth2": {
"location": "header",
"name": "Authorization",
"prefix": "Bearer",
"client_id": "{{ AIPOLABS_INSTAGRAM_APP_CLIENT_ID }}",
"client_secret": "{{ AIPOLABS_INSTAGRAM_APP_CLIENT_SECRET }}",
"scope": "instagram_business_basic instagram_business_content_publish instagram_business_manage_messages instagram_business_manage_comments instagram_business_manage_insights",
"authorize_url": "https://www.instagram.com/oauth/authorize",
"access_token_url": "https://api.instagram.com/oauth/access_token",
"exchange_token_url": "https://graph.instagram.com/access_token",
"refresh_token_url": "https://graph.instagram.com/refresh_access_token",
"token_endpoint_auth_method": "client_secret_post"
}
},
"default_security_credentials_by_scheme": {},
"categories": ["Social Media", "User Data"],
"visibility": "public",
"active": true
}
Loading
Loading