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

Add option for custom auth #1280

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

patrykkotlowski-dsstream
Copy link

@patrykkotlowski-dsstream patrykkotlowski-dsstream commented Aug 30, 2024

Custom authentication

This PR provides an option for the users to use custom authentication mechanism.

What's changed:

  • New options were added to the configuration
  • There is an option to create custom oauth provider
  • There is an option for custom JWT token check.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 26, 2024
@dokterbob dokterbob added enhancement New feature or request and removed bug Something isn't working labels Sep 26, 2024
@dokterbob
Copy link
Collaborator

Thanks for the contrib @patrykkotlowski-dsstream!

Do you see any way of using this work to arrive at a more generic pluggable auth? We're really looking forward to move support of auth frameworks more towards the community, so we'd rather remove than add new methods.

But if yours could be refactored towards having more pluggable auth methods (e.g. all auth methods are classes) that can be easily swapped, which can be returned from a single call back hook, that might be really nice.

In addition, before we merge this in, we'd really like to see both E2E and unit tests. Otherwise, it's really hard for us to ensure that it won't break time and again while building on other stuff.

What do you think?

@patrykkotlowski-dsstream
Copy link
Author

Hi, thank you for the feedback!

I would be more than happy to add both E2E and unit tests to ensure the stability of the implementation. Also, I’m definitely interested in refactoring the auth methods in the future to make them more pluggable, as suggested. However, if it’s okay with you, I’d prefer to handle that refactor in a separate PR, since I’m currently focused on adding the CustomAuth provider as quickly as possible.

Let me know your thoughts!

@danpe
Copy link

danpe commented Oct 15, 2024

Would be great if the custom JWT token check will be able to use something like https://github.com/TCatshoek/fastapi-nextauth-jwt

@dokterbob
Copy link
Collaborator

https://github.com/TCatshoek/fastapi-nextauth-jwt

Where I want to be, honestly, is we don't implement auth ourselves but rather rely on a well-supported libraries (client and server-side) in order to reduce attack and maintenance surface.

@patrykkotlowski-dsstream Happy to merge once test coverage lands (at least E2E, unit test is nice to have at this point). Definitely, the refactor should not be part of this request!

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 30, 2024
@lwieczorek-dss
Copy link

https://github.com/TCatshoek/fastapi-nextauth-jwt

Where I want to be, honestly, is we don't implement auth ourselves but rather rely on a well-supported libraries (client and server-side) in order to reduce attack and maintenance surface.

@patrykkotlowski-dsstream Happy to merge once test coverage lands (at least E2E, unit test is nice to have at this point). Definitely, the refactor should not be part of this request!

Hi folks, we've covered the code with unit tests but we are not sure about the E2E as OAuth for default supported providers is not covered with E2E tests.

@dokterbob
Copy link
Collaborator

https://github.com/TCatshoek/fastapi-nextauth-jwt

Where I want to be, honestly, is we don't implement auth ourselves but rather rely on a well-supported libraries (client and server-side) in order to reduce attack and maintenance surface.
@patrykkotlowski-dsstream Happy to merge once test coverage lands (at least E2E, unit test is nice to have at this point). Definitely, the refactor should not be part of this request!

Hi folks, we've covered the code with unit tests but we are not sure about the E2E as OAuth for default supported providers is not covered with E2E tests.

That's exactly one of the problems. ;)
Auth is definitely something that requires testing!

Copy link
Collaborator

@dokterbob dokterbob left a comment

Choose a reason for hiding this comment

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

Thanks so much for your endeavours! This is actually a really great feature!
However...

Given that I aim to move towards using an external OAuth library, I wonder a bit what to do with this PR -- as it stands it would get more users to rely on OAuthProvider which is exactly what I aim to remove from the codebase!

Then again, perhaps we can evolve this PR into using a 3rd party library already? Alternatively, we can leave this one open and I'll attempt to adopt it to the 'cleaner' OAuth approach -- although it could take ~6 weeks for me to do that.

Happy to hear what y'all think. Whether perhaps you have the bandwidth to:

  1. Help evaluate/choose a well supported, test-covered and actively maintained OAuth client lib (e.g. implementing OAuthProvider type of functionality).
  2. Integrate it with this work, possibly refactoring the way OAuth is configured (perhaps explicit instead of implicit?).

raw_user_data: dict,
default_app_user: User,
id_token: str | None = None,
) -> User | None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason to add this callback over using @oauth_callback?

What does the value of id_token represent, in OAuth terms -- and why is it not tested?

return func


def custom_oauth_provider(func: Callable[[], OAuthProvider]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure whether custom_ is the right name here. As we'll be moving to having less and less oauth providers built in, I feel it's more likely that most if not all oauth providers will be pluggable. Additionally, I want to move towards using an externally maintained OAuth client framework instead (see #1240).

So perhaps, this could become the default way of setting up oauth providers -- in that case something like @register_oauth_provider or merely @oauth_provider might be a more appropriate name.

@@ -8,7 +8,7 @@ async def is_thread_author(username: str, thread_id: str):
raise HTTPException(status_code=400, detail="Data layer not initialized")

thread_author = await data_layer.get_thread_author(thread_id)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make sure this is removed from the PR, it is conducive to merge conflicts.

@nethi
Copy link

nethi commented Nov 20, 2024

@dokterbob @patrykkotlowski-dsstream This is a good solution for one of the Auth problem I have as well. Is there a chance of this getting merged soon ?

Here is the problem I am looking at solving:

  1. There is a non-chainlit Auth token in my system where the user is already authenticated
  2. I would pass this as Auth token to Chainlit
  3. Implement custom_authenticate_user callback to validate the token and create chainlit User object.

I suppose this would work for both chainlit REST APis and websocket connections.

@dokterbob
Copy link
Collaborator

@dokterbob @patrykkotlowski-dsstream This is a good solution for one of the Auth problem I have as well. Is there a chance of this getting merged soon ?

Here is the problem I am looking at solving:

  1. There is a non-chainlit Auth token in my system where the user is already authenticated
  2. I would pass this as Auth token to Chainlit
  3. Implement custom_authenticate_user callback to validate the token and create chainlit User object.

I suppose this would work for both chainlit REST APis and websocket connections.

It's currently blocked by requested changes in the review here. (Only) if @patrykkotlowski-dsstream is not available to address them, you're welcome to fork their branch to address the issues, resolve merge conflicts and do a 2nd PR.

@nethi
Copy link

nethi commented Nov 29, 2024

@dokterbob @patrykkotlowski-dsstream This is a good solution for one of the Auth problem I have as well. Is there a chance of this getting merged soon ?
Here is the problem I am looking at solving:

  1. There is a non-chainlit Auth token in my system where the user is already authenticated
  2. I would pass this as Auth token to Chainlit
  3. Implement custom_authenticate_user callback to validate the token and create chainlit User object.

I suppose this would work for both chainlit REST APis and websocket connections.

It's currently blocked by requested changes in the review here. (Only) if @patrykkotlowski-dsstream is not available to address them, you're welcome to fork their branch to address the issues, resolve merge conflicts and do a 2nd PR.

@patrykkotlowski-dsstream would you be looking at making requested changes ?

@dokterbob
Copy link
Collaborator

@nethi Just a little head's up that I'm in the last passes of making cookie-based auth the default #1521.

From now on, the user will authenticate with the backend, the backend sets a HTTP-only cookie and from thereon all requests are authenticated with cookies. This is required as we're serving static files, potentially scripts, potentially user or LLM-generated, and we want to make sure those won't have access to our auth token.

The actual login authentication mechanism does not change as this point (but that's on the roadmap! :p).

To have a 3rd party site authenticate against chainlit, the best approach is to:

  1. Have the 3rd party side generate a short-lived signed auth token.
  2. Have the client do a POST call to some chainlit auth endpoint, validating the token and setting the auth cookie.

(Ideally, used and unexpired short-lived tokens should be kept track of to prevent replay attacks, but that requires shared state/persistence which we currently do not have. Other than that, this is best practise.)

I believe a similar mechanism is already employed for the copilot. In the coming few days, I will have to get down into the nitty gritty of things in order to finish cookie based auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants