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

backend: Add refresh token logic to OAuth providers #252

Closed
wants to merge 44 commits into from

Conversation

tianjing-li
Copy link
Collaborator

@tianjing-li tianjing-li commented Jun 20, 2024

Thank you for contributing to the Cohere Toolkit!

  • PR title: "area: description"

    • Where "area" is whichever of interface, frontend, model, tools, backend, etc. is being modified. Use "docs: ..." for purely docs changes, "infra: ..." for CI changes.
    • Example: "deployment: add Azure model option"
  • PR message: Delete this entire checklist and replace with

    • Description: a description of the change
    • Issue: the issue # it fixes, if applicable
    • Dependencies: any dependencies required for this change
  • Add tests and docs: Please include testing and documentation for your changes

  • Lint and test: Run make lint and make run-tests

AI Description

This PR introduces changes related to authentication and authorization, including:

  • OAuth app configuration: The PR adds instructions for configuring OAuth apps, emphasizing the need to whitelist the Redirect URI and providing an example.
  • Refresh tokens: It also introduces the ability to enable refresh tokens by implementing the get_refresh_token_params() method in the auth strategy class.
  • Auth strategy retrieval: The code now includes a function, get_auth_strategy, to retrieve the authentication strategy based on the strategy name.
  • Logging middleware: A logging middleware class has been added to log request details, including the method, URL path, and headers.
  • JWT token refresh: The JWTService class now includes a refresh_jwt method to refresh JWT tokens.
  • Token validation: The validate_authorization function has been updated to perform additional checks on the JWT token, including verifying its structure and checking if it's blacklisted or expired.
  • Dependency updates: The PR also updates dependencies, including the Node version in the Dockerfile and packages in package.json and package-lock.json.
  • Test endpoint: A test endpoint (/test-auth) has been added to test authorization bearer validation.
  • Auth strategy tests: Additional tests have been included to validate different scenarios, such as an invalid token, an expired token, and a blacklisted token.
  • Miscellaneous changes: The PR also includes updates to various files related to the Cohere client and authentication.

@tianjing-li tianjing-li marked this pull request as draft June 20, 2024 17:54
@tianjing-li tianjing-li changed the base branch from main to develop July 5, 2024 21:39
Base automatically changed from develop to main July 15, 2024 20:33
# REFRESH_AVAILABILITY_HOURS = 36
AUTH_EXPIRY_SECONDS = 60 * 2
JWT_EXPIRY_SECONDS = 30
REFRESH_AVAILABILITY_SECONDS = 15
Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc, this may be too small of a value? if im right in thinking, i would suggest changing the actual authentication expiry of the current access_token to a value that is higher? IE, access_token validity==1hr && refresh_token validity==6-24hrs etc

i assume since this is local deployment/backend, these can be set as per the environment requirements so it may be best to go with a boilerplate approach and add some definition around the struct for easy adoption

apologies if i am missing something here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The commented out timings here are intended for production, while the shorter times in seconds are just for testing on this branch to make sure things are expired or refreshed properly

- "refreshable": Token is valid and within the refresh availability window.
- "expired": Token is expired or blacklisted.
- "invalid": Token is invalid.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on adding something here to prevent replays of refresh tokens?

src/backend/services/auth/jwt.py Outdated Show resolved Hide resolved
Copy link
Contributor

@GangGreenTemperTatum GangGreenTemperTatum left a comment

Choose a reason for hiding this comment

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

this looks great so far, thank you! :) will keep an eye out for replay prevention of existing refresh_tokens coming, let me know if you need any other support here and happy to do local branch testing for you when ready 👍

src/backend/services/auth/jwt.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants