feat: Instagram Graph API integration and custom OAuth2 implementation #580
Conversation
YueTwo
had a problem deploying
to
CICD_FOR_FORKED_REPO
GitHub Actions
Failure
YueTwo
had a problem deploying
to
CICD_FOR_FORKED_REPO
GitHub Actions
Failure
|
@YueTwo is attempting to deploy a commit to the Proxy Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Instagram-specific OAuth2 handling (short-lived → long-lived token exchange, IG refresh flow, expires_at adjustment), makes token parsing asynchronous, introduces OAuth2Scheme.custom_data and threads it into OAuth2Manager, updates linked account routes and credential refresh flows, and adds Instagram app metadata and API function specs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LinkedAccountsRoute
participant OAuth2Manager
participant InstagramAPI
User->>LinkedAccountsRoute: callback (code, state)
LinkedAccountsRoute->>OAuth2Manager: construct(with custom_data) -> fetch_token
OAuth2Manager->>OAuth2Manager: obtain initial token response
alt provider == INSTAGRAM and access_token present
OAuth2Manager->>InstagramAPI: GET exchange_token_url?grant_type=ig_exchange_token&client_secret&access_token=short_lived
InstagramAPI-->>OAuth2Manager: long-lived token response
OAuth2Manager->>OAuth2Manager: merge data, compute expires_at (expires_in - 86400)
else non-Instagram
OAuth2Manager->>OAuth2Manager: standard token parsing/refresh
end
OAuth2Manager-->>LinkedAccountsRoute: OAuth2SchemeCredentials (awaited)
LinkedAccountsRoute-->>User: complete linking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
✨ No issues found! Your code is sparkling clean! ✨ Need help? Join our Discord for support! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/aci/server/routes/linked_accounts.py (1)
437-441: Consider more robust handling of Instagram state suffix removal.The current implementation removes the
#_suffix but doesn't validate that the state remains valid after removal. Consider checking if the resulting state is not empty.- # Special handling for Instagram: remove #_ suffix if present - if state_jwt and state_jwt.endswith("#_"): - state_jwt = state_jwt[:-2] # Remove the last 2 characters (#_) - logger.info("Removed Instagram #_ suffix from state") + # Special handling for Instagram: remove #_ suffix if present + if state_jwt and state_jwt.endswith("#_"): + state_jwt = state_jwt[:-2] # Remove the last 2 characters (#_) + if not state_jwt: # Ensure state is not empty after removal + logger.error("State became empty after removing Instagram suffix") + raise OAuth2Error("Invalid state parameter after Instagram suffix removal") + logger.info("Removed Instagram #_ suffix from state")backend/aci/server/security_credentials_manager.py (1)
100-109: Correct handling of Instagram token expiration.Instagram doesn't support token refresh, so directing users to re-authorize is the appropriate approach. Consider extracting the error message to a constant for better maintainability.
+ INSTAGRAM_TOKEN_EXPIRED_MESSAGE = ( + "Access token expired. Please re-authorize at: {url}/appconfigs/{app_name}" + ) + if app.name == "INSTAGRAM": 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}" + INSTAGRAM_TOKEN_EXPIRED_MESSAGE.format( + url=config.DEV_PORTAL_URL, app_name=app.name + ) )backend/apps/instagram/functions.json (2)
230-235: Useintegerinstead ofnumberfor count-like values
limitrepresents a discrete count; JSON-Schema recommendstype: "integer"(and allowsminimum/maximum). The same comment applies to similarlimitfields throughout the file.- "limit": { - "type": "number", + "limit": { + "type": "integer",
815-821: Clarify timestamp types
since/untilare Unix timestamps; declare them asintegerand document timezone/seconds to avoid confusion.- "since": { - "type": "number", + "since": { + "type": "integer",(and likewise for
until)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/aci/common/schemas/security_scheme.py(1 hunks)backend/aci/server/oauth2_manager.py(5 hunks)backend/aci/server/routes/linked_accounts.py(3 hunks)backend/aci/server/security_credentials_manager.py(3 hunks)backend/apps/instagram/app.json(1 hunks)backend/apps/instagram/functions.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
backend/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking
Files:
backend/aci/common/schemas/security_scheme.pybackend/aci/server/security_credentials_manager.pybackend/aci/server/routes/linked_accounts.pybackend/aci/server/oauth2_manager.py
backend/aci/common/schemas/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend Pydantic models for validation should be defined in backend/aci/common/schemas/
Files:
backend/aci/common/schemas/security_scheme.py
backend/aci/server/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend FastAPI application code should be located in backend/aci/server/
Files:
backend/aci/server/security_credentials_manager.pybackend/aci/server/oauth2_manager.py
backend/apps/*/{app.json,functions.json}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Each integration in backend/apps/{app_name}/ must have both app.json and functions.json files
Files:
backend/apps/instagram/app.jsonbackend/apps/instagram/functions.json
backend/apps/*/app.json
📄 CodeRabbit Inference Engine (CLAUDE.md)
Integration app.json files must define metadata, authentication schemes, categories, and visibility
Files:
backend/apps/instagram/app.json
backend/apps/**/*.json
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend integration definitions should be stored as JSON configs in backend/apps/
Files:
backend/apps/instagram/app.jsonbackend/apps/instagram/functions.json
backend/apps/*/functions.json
📄 CodeRabbit Inference Engine (CLAUDE.md)
Integration functions.json files must define API endpoints and their schemas
Files:
backend/apps/instagram/functions.json
🧠 Learnings (13)
📓 Common learnings
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:46:57.991Z
Learning: In X.com API integrations using OAuth 2.0 Application-Only (Bearer Token) authentication, certain endpoints like `/2/tweets/{id}/liking_users` are not accessible because they require user context authentication (OAuth 1.0a User Context or OAuth 2.0 User Context). This creates intentional gaps in functionality where only publicly accessible endpoints can be implemented.
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/routes/linked_accounts.py:480-507
Timestamp: 2025-05-08T19:22:35.378Z
Learning: OAuth2 errors in this codebase should use the OAuth2Error exception class with a 500 status code, even for client-side issues like missing parameters or provider-returned errors.
📚 Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.
Applied to files:
backend/aci/common/schemas/security_scheme.pybackend/aci/server/security_credentials_manager.py
📚 Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/
Applied to files:
backend/aci/common/schemas/security_scheme.py
📚 Learning: in the oauth2 flow implementation for aci, the `rewrite_oauth2_authorization_url` method is called e...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/oauth2_manager.py:95-105
Timestamp: 2025-05-08T19:33:00.496Z
Learning: In the OAuth2 flow implementation for ACI, the `rewrite_oauth2_authorization_url` method is called externally after obtaining the authorization URL from `create_authorization_url`, rather than being integrated directly within the `create_authorization_url` method itself.
Applied to files:
backend/aci/common/schemas/security_scheme.pybackend/aci/server/security_credentials_manager.pybackend/aci/server/routes/linked_accounts.pybackend/aci/server/oauth2_manager.py
📚 Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.
Applied to files:
backend/aci/server/security_credentials_manager.pybackend/aci/server/routes/linked_accounts.py
📚 Learning: x.com api requires oauth 2.0 authorization code flow with pkce for user context endpoints, which is ...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:49:10.008Z
Learning: X.com API requires OAuth 2.0 Authorization Code Flow with PKCE for user context endpoints, which is incompatible with ACI's current OAuth 2.0 User Context implementation, creating specific authentication gaps for certain endpoints like `/2/tweets/{id}/liking_users`.
Applied to files:
backend/aci/server/security_credentials_manager.pybackend/aci/server/routes/linked_accounts.py
📚 Learning: oauth2 errors in this codebase should use the oauth2error exception class with a 500 status code, ev...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/routes/linked_accounts.py:480-507
Timestamp: 2025-05-08T19:22:35.378Z
Learning: OAuth2 errors in this codebase should use the OAuth2Error exception class with a 500 status code, even for client-side issues like missing parameters or provider-returned errors.
Applied to files:
backend/aci/server/security_credentials_manager.pybackend/aci/server/oauth2_manager.py
📚 Learning: the validate_user_access_to_org function in backend/aci/server/acl.py is planned to be removed in th...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#453
File: backend/aci/server/acl.py:22-24
Timestamp: 2025-06-04T17:35:08.995Z
Learning: The validate_user_access_to_org function in backend/aci/server/acl.py is planned to be removed in the near future, so documentation improvements are not needed for this function.
Applied to files:
backend/aci/server/security_credentials_manager.py
📚 Learning: end-user service authentication must be stored as linkedaccounts using oauth2 or api keys...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: End-user service authentication must be stored as LinkedAccounts using OAuth2 or API keys
Applied to files:
backend/aci/server/routes/linked_accounts.py
📚 Learning: applies to backend/apps/*/app.json : integration app.json files must define metadata, authentication...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/app.json : Integration app.json files must define metadata, authentication schemes, categories, and visibility
Applied to files:
backend/apps/instagram/app.json
📚 Learning: applies to backend/apps/**/*.json : backend integration definitions should be stored as json configs...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/**/*.json : Backend integration definitions should be stored as JSON configs in backend/apps/
Applied to files:
backend/apps/instagram/app.json
📚 Learning: applies to backend/apps/*/{app.json,functions.json} : each integration in backend/apps/{app_name}/ m...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/{app.json,functions.json} : Each integration in backend/apps/{app_name}/ must have both app.json and functions.json files
Applied to files:
backend/apps/instagram/app.jsonbackend/apps/instagram/functions.json
📚 Learning: applies to backend/apps/*/functions.json : integration functions.json files must define api endpoint...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/functions.json : Integration functions.json files must define API endpoints and their schemas
Applied to files:
backend/apps/instagram/app.jsonbackend/apps/instagram/functions.json
🧬 Code Graph Analysis (2)
backend/aci/server/routes/linked_accounts.py (1)
backend/aci/server/oauth2_manager.py (2)
fetch_token(118-149)parse_fetch_token_response(207-265)
backend/aci/server/oauth2_manager.py (2)
backend/aci/common/exceptions.py (1)
OAuth2Error(437-447)backend/aci/common/schemas/security_scheme.py (1)
OAuth2SchemeCredentials(180-195)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (9)
backend/aci/common/schemas/security_scheme.py (1)
82-87: LGTM! Clean addition of Instagram-specific token exchange URL.The optional
exchange_token_urlfield is well-structured with appropriate validation constraints and clear documentation. The implementation maintains backward compatibility for existing OAuth2 integrations.backend/aci/server/routes/linked_accounts.py (3)
363-363: Correctly passes the new exchange_token_url parameter.The parameter is properly passed from the OAuth2 scheme to the OAuth2Manager initialization.
510-510: Correctly passes the exchange_token_url in callback handler.Consistent with the OAuth2 flow initialization.
518-518: Correctly awaits the async parse_fetch_token_response method.The change properly handles the method's conversion to async.
backend/aci/server/security_credentials_manager.py (1)
170-170: Correctly passes exchange_token_url to OAuth2Manager.The parameter is properly propagated during token refresh operations.
backend/aci/server/oauth2_manager.py (2)
27-53: Properly integrated exchange_token_url parameter.The parameter is correctly added to the constructor and stored as an instance variable.
207-242: Async conversion handled correctly with Instagram token exchange.The method properly exchanges short-lived tokens for long-lived tokens for Instagram while maintaining existing Slack handling. The async conversion is implemented correctly.
backend/apps/instagram/functions.json (2)
15-18: Potential endpoint host mismatch – confirmserver_urldomain
graph.instagram.comis intended for the Basic Display API. Publishing content, messaging, conversations and insights for Business / Creator accounts are documented underhttps://graph.facebook.com. Using the Basic-Display host here may cause 400 / OAuth errors at runtime.Please double-check the official “Instagram Graph API” docs for v23.0 and switch the host if necessary.
1-2: Ensure companionapp.jsonis presentCoding guidelines require both
backend/apps/instagram/app.jsonandfunctions.json. Onlyfunctions.jsonappears in this diff. Please verify thatapp.jsonis included elsewhere in the PR so CI validations pass.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found across 6 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
YueTwo
had a problem deploying
to
CICD_FOR_FORKED_REPO
GitHub Actions
Failure
YueTwo
had a problem deploying
to
CICD_FOR_FORKED_REPO
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/aci/server/oauth2_manager.py (1)
167-206: Excellent implementation with minor security enhancement needed.The method correctly implements Instagram's token exchange flow with proper validation, error handling, and timeout. Consider adding HTTPS validation for the exchange URL:
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") + + if not self.exchange_token_url.startswith("https://"): + raise OAuth2Error("Exchange token URL must use HTTPS")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/aci/server/oauth2_manager.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking
Files:
backend/aci/server/oauth2_manager.py
backend/aci/server/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend FastAPI application code should be located in backend/aci/server/
Files:
backend/aci/server/oauth2_manager.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: YueTwo
PR: aipotheosis-labs/aci#580
File: backend/apps/instagram/app.json:1-27
Timestamp: 2025-08-07T14:56:50.216Z
Learning: For Instagram Business Login OAuth flow, the correct authorize_url is https://www.instagram.com/oauth/authorize (not https://api.instagram.com/oauth/authorize). This is different from the Instagram Basic Display API which uses https://api.instagram.com/oauth/authorize, and should not be changed when implementing Business Login for Instagram.
Learnt from: YueTwo
PR: aipotheosis-labs/aci#580
File: backend/apps/instagram/app.json:1-27
Timestamp: 2025-08-07T14:56:50.216Z
Learning: For Instagram Business Login OAuth flow, the correct authorize_url is https://www.instagram.com/oauth/authorize (not https://api.instagram.com/oauth/authorize). This is different from other Instagram API OAuth endpoints and should not be changed.
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:46:57.991Z
Learning: In X.com API integrations using OAuth 2.0 Application-Only (Bearer Token) authentication, certain endpoints like `/2/tweets/{id}/liking_users` are not accessible because they require user context authentication (OAuth 1.0a User Context or OAuth 2.0 User Context). This creates intentional gaps in functionality where only publicly accessible endpoints can be implemented.
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/routes/linked_accounts.py:480-507
Timestamp: 2025-05-08T19:22:35.378Z
Learning: OAuth2 errors in this codebase should use the OAuth2Error exception class with a 500 status code, even for client-side issues like missing parameters or provider-returned errors.
📚 Learning: in the oauth2 flow implementation for aci, the `rewrite_oauth2_authorization_url` method is called e...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/oauth2_manager.py:95-105
Timestamp: 2025-05-08T19:33:00.496Z
Learning: In the OAuth2 flow implementation for ACI, the `rewrite_oauth2_authorization_url` method is called externally after obtaining the authorization URL from `create_authorization_url`, rather than being integrated directly within the `create_authorization_url` method itself.
Applied to files:
backend/aci/server/oauth2_manager.py
📚 Learning: oauth2 errors in this codebase should use the oauth2error exception class with a 500 status code, ev...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/routes/linked_accounts.py:480-507
Timestamp: 2025-05-08T19:22:35.378Z
Learning: OAuth2 errors in this codebase should use the OAuth2Error exception class with a 500 status code, even for client-side issues like missing parameters or provider-returned errors.
Applied to files:
backend/aci/server/oauth2_manager.py
🔇 Additional comments (2)
backend/aci/server/oauth2_manager.py (2)
27-27: LGTM! Constructor parameter addition follows good practices.The
exchange_token_urlparameter is properly typed, documented, and assigned. The optional nature with None default is appropriate for Instagram-specific functionality.Also applies to: 43-43, 53-53
208-208: Well-implemented Instagram token exchange integration.The async method conversion and Instagram-specific logic are correctly implemented:
- Proper async/await usage for the new token exchange call
- Follows existing pattern for app-specific handling (similar to Slack)
- Maintains backward compatibility with other OAuth2 flows
- Correct error handling using OAuth2Error as required by the codebase standards
Also applies to: 228-242
| @@ -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( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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("#_"): | |||
There was a problem hiding this comment.
curious why this special handling is needed?
There was a problem hiding this comment.
This is another difference of instagram from other apps, fyi: business-login
There was a problem hiding this comment.
can you verify it? because it says code in the screenshot, but you're getting that from the state
There was a problem hiding this comment.
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.
backend/aci/server/oauth2_manager.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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": | |||
There was a problem hiding this comment.
just make sure to test if re-autorize works as expected when you're doing e2e testing
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
| @@ -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("#_"): | |||
There was a problem hiding this comment.
can you verify it? because it says code in the screenshot, but you're getting that from the state
| @@ -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( | |||
There was a problem hiding this comment.
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
backend/aci/server/oauth2_manager.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/aci/common/schemas/security_scheme.py(1 hunks)backend/aci/server/oauth2_manager.py(6 hunks)backend/aci/server/routes/linked_accounts.py(2 hunks)backend/aci/server/security_credentials_manager.py(5 hunks)backend/apps/instagram/app.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/instagram/app.json
- backend/aci/server/routes/linked_accounts.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking
Files:
backend/aci/common/schemas/security_scheme.pybackend/aci/server/security_credentials_manager.pybackend/aci/server/oauth2_manager.py
backend/aci/common/schemas/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend Pydantic models for validation should be defined in backend/aci/common/schemas/
Files:
backend/aci/common/schemas/security_scheme.py
backend/aci/server/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend FastAPI application code should be located in backend/aci/server/
Files:
backend/aci/server/security_credentials_manager.pybackend/aci/server/oauth2_manager.py
🔇 Additional comments (13)
backend/aci/common/schemas/security_scheme.py (1)
76-79: LGTM! Good implementation of the generic custom_data field.The addition of the
custom_datafield provides a flexible way to handle provider-specific configuration without hardcoding Instagram-specific fields. This approach will accommodate future integrations with similar requirements.backend/aci/server/security_credentials_manager.py (6)
20-20: LGTM! Proper import for configuration access.The import is correctly added to access
DEV_PORTAL_URLfor generating re-authorization URLs.
100-110: Verify re-authorization URL format and test user experience.The Instagram-specific re-authorization flow looks correct with proper error logging and user guidance. The URL format follows the expected pattern for linking to the dev portal app configuration.
Please verify that the re-authorization URL format is correct and test the complete user experience to ensure smooth re-authorization flow as mentioned in the past review comments.
163-163: Good addition of access_token parameter.Extracting the access_token for the refresh flow is necessary for Instagram's refresh mechanism which uses the access_token instead of a separate refresh_token.
179-179: LGTM! Proper custom_data integration.The OAuth2Manager constructor now correctly receives custom_data from the oauth2_scheme, enabling provider-specific configurations.
182-182: Correct signature update for refresh_token method.The method call now properly passes both refresh_token and access_token parameters, aligning with the updated OAuth2Manager.refresh_token signature that supports Instagram's refresh flow.
124-128: Verified 86400-second safety margin calculationConfirmed that 86400 seconds equals 24 hours (1 day) and the safety buffer is applied consistently in both locations:
- backend/aci/server/security_credentials_manager.py
- backend/aci/server/oauth2_manager.py
No changes required.
backend/aci/server/oauth2_manager.py (6)
27-27: LGTM! Clean implementation of custom_data parameter.The custom_data parameter is properly integrated into the constructor with appropriate documentation and defensive initialization.
Also applies to: 43-43, 53-53
117-157: Excellent implementation with proper security considerations.The
exchange_short_lived_tokenmethod correctly implements Instagram's token exchange requirements:
- Properly restricts usage to Instagram apps only
- Uses GET request as required by Instagram's API
- Includes timeout to prevent hanging requests
- Has comprehensive error handling and logging
- Addresses the security concerns from past reviews by adding timeout
188-204: Well-structured Instagram token exchange integration.The Instagram-specific handling in
fetch_tokenis properly implemented:
- Correctly checks for access_token presence
- Properly exchanges short-lived for long-lived tokens
- Updates the token with the exchange response
- Includes appropriate error handling for missing access_token
218-235: Excellent Instagram refresh implementation.The Instagram-specific refresh flow correctly implements the
ig_refresh_tokengrant type using the access_token, while maintaining the standard OAuth2 refresh flow for other providers.
241-241: Good conversion to async method.Converting
parse_fetch_token_responseto async aligns with the asynchronous patterns used throughout the codebase and prepares for potential future async operations.
270-274: Consistent Instagram expires_at calculation.The Instagram-specific expires_at calculation with 86400-second safety margin is consistently implemented across both token parsing and refresh flows, matching the security_credentials_manager implementation.
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
YueTwo
requested a deployment
to
CICD_FOR_FORKED_REPO
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
backend/aci/server/security_credentials_manager.py (2)
128-136: Bug: expires_at None check is incorrect; use a constant for IG marginThe condition
or not expires_attreats 0 as falsy and will raise unnecessarily. Useexpires_at is None. Also, reuse a constant for 86400.Apply this diff:
- if app.name == "INSTAGRAM": - # Reduce expiration time by 1 day (86400 seconds) for safety margin - expires_at = int(time.time()) + max(0, int(token_response["expires_in"]) - 86400) + if app.name == "INSTAGRAM": + # Reduce expiration time by 1 day for a safety margin + expires_at = int(time.time()) + max( + 0, int(token_response["expires_in"]) - IG_EXPIRY_SAFETY_MARGIN_SECONDS + ) else: expires_at = int(time.time()) + int(token_response["expires_in"]) - if not token_response.get("access_token") or not expires_at: + if not token_response.get("access_token") or expires_at is None: logger.error(
163-187: Broken IG refresh flow: reversed args and unnecessary refresh_token requirementTwo issues:
- Arguments are passed to OAuth2Manager.refresh_token in the wrong order. Its signature is (access_token, refresh_token); the current call swaps them, breaking IG refresh (it passes the refresh token where the IG endpoint expects access_token).
- IG refresh does not use a refresh_token, but the function unconditionally requires one, causing failures for IG accounts.
Apply this diff:
async def _refresh_oauth2_access_token( app_name: str, oauth2_scheme: OAuth2Scheme, oauth2_scheme_credentials: OAuth2SchemeCredentials ) -> dict: refresh_token = oauth2_scheme_credentials.refresh_token - access_token = oauth2_scheme_credentials.access_token + access_token = oauth2_scheme_credentials.access_token - if not refresh_token: - raise OAuth2Error("no refresh token found") + # Instagram refresh uses the current (long-lived) access token and does not require a refresh token + if app_name != "INSTAGRAM" and not refresh_token: + raise OAuth2Error("no refresh token found") + if app_name == "INSTAGRAM" and not access_token: + raise OAuth2Error("no access token found for Instagram refresh") # NOTE: it's important to use oauth2_scheme_credentials's client_id, client_secret, scope because # these fields might have changed for the app configuration after the linked account was created oauth2_manager = OAuth2Manager( app_name=app_name, client_id=oauth2_scheme_credentials.client_id, client_secret=oauth2_scheme_credentials.client_secret, scope=oauth2_scheme_credentials.scope, authorize_url=oauth2_scheme.authorize_url, 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, custom_data=oauth2_scheme.custom_data, ) - return await oauth2_manager.refresh_token(refresh_token, access_token) + # Call with correct argument order. For IG, refresh_token is unused. + if app_name == "INSTAGRAM": + return await oauth2_manager.refresh_token(access_token=access_token, refresh_token="") + return await oauth2_manager.refresh_token(access_token="", refresh_token=refresh_token)
♻️ Duplicate comments (1)
backend/aci/server/security_credentials_manager.py (1)
100-114: Eliminate mypy ignore and harden IG re-auth guardGood call to short-circuit re-auth for Instagram when the token is actually expired. Avoid the type: ignore and use explicit narrowing; also consider centralizing the safety margin.
Apply this diff:
- # Since _access_token_is_expired returned True, expires_at is guaranteed to be not None - actual_expires_at = oauth2_scheme_credentials.expires_at + 86400 # type: ignore[operator] - if int(time.time()) > actual_expires_at: + # Since _access_token_is_expired returned True, expires_at is guaranteed to be not None + expires_at_val = oauth2_scheme_credentials.expires_at + assert expires_at_val is not None # mypy narrowing + actual_expires_at = expires_at_val + 86400 + if int(time.time()) > actual_expires_at:Optional: introduce a module-level constant and reuse it in both places where 86400 is used.
# near the top of this module IG_EXPIRY_SAFETY_MARGIN_SECONDS = 86400 # 1 dayPlease also verify the end-to-end re-authorization UX for IG in dev and prod, per earlier review feedback (screens/link flow).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/aci/server/oauth2_manager.py(7 hunks)backend/aci/server/security_credentials_manager.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/aci/server/oauth2_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking
Files:
backend/aci/server/security_credentials_manager.py
backend/aci/server/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Backend FastAPI application code should be located in backend/aci/server/
Files:
backend/aci/server/security_credentials_manager.py
🧬 Code Graph Analysis (1)
backend/aci/server/security_credentials_manager.py (2)
backend/aci/common/exceptions.py (1)
OAuth2Error(437-447)backend/aci/server/oauth2_manager.py (1)
refresh_token(214-262)
🔇 Additional comments (1)
backend/aci/server/security_credentials_manager.py (1)
20-20: LGTM: Config import for re-auth URLUsing aci.server.config to construct the re-authorization URL is consistent with oauth2_manager.py and keeps URL configuration centralized.
| 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}" | ||
| ) | ||
| token_response = await _refresh_oauth2_access_token( | ||
| app.name, oauth2_scheme, oauth2_scheme_credentials | ||
| ) | ||
|
|
||
| # TODO: refactor parsing to _refresh_oauth2_access_token |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Centralize token parsing and expiry handling in OAuth2Manager
The TODO is on point. Move provider-specific expiry calculation (e.g., IG’s safety margin and potential absence/presence of expires_in) into OAuth2Manager so callers don’t duplicate logic and drift over time. Return a normalized structure with access_token, refresh_token (optional), and computed expires_at.
I can implement a parse_refresh_response(access_token_response: dict) -> dict[str, Any] in OAuth2Manager and update callers accordingly. Want me to open a follow-up PR?
🤖 Prompt for AI Agents
In backend/aci/server/security_credentials_manager.py around line 123, the
provider-specific token parsing and expiry logic should be moved into
OAuth2Manager: implement a method parse_refresh_response(access_token_response:
dict) -> dict[str, Any] that normalizes and returns { "access_token": str,
"refresh_token"?: str, "expires_at": int }, apply IG-specific safety margin and
robustly handle absent or malformed expires_in values when computing expires_at;
replace duplicated parsing in callers to call this new method so all token
parsing and expiry computation is centralized and consistent.
Summary
This PR implements a comprehensive Instagram Graph API integration using Business Login for Instagram, featuring a customized OAuth2 workflow and 18 REST functions for complete Instagram business account management.
Here is the most relevant document for your reference: business-login.
Key Features
insights, and messaging
Functions Added
Technical Implementation
Summary by cubic
Added full Instagram Graph API integration with 18 REST functions for business account management and a custom OAuth2 workflow supporting Instagram's token exchange and authentication flow.
Summary by CodeRabbit
New Features
Bug Fixes
Chores