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

Elements don't display when using authentication in 1.3.0 & 2.0.dev1 #1472

Closed
hayescode opened this issue Oct 23, 2024 · 18 comments · Fixed by #1474
Closed

Elements don't display when using authentication in 1.3.0 & 2.0.dev1 #1472

hayescode opened this issue Oct 23, 2024 · 18 comments · Fixed by #1474
Labels
auth Pertaining to authentication. bug Something isn't working needs-triage

Comments

@hayescode
Copy link
Contributor

hayescode commented Oct 23, 2024

Describe the bug
Since updating version from 2.0.dev0 to 1.3.0 & 2.0.dev1 no elements display when using authentication or authentication+data layer. The elements are successfully persisted, and resuming chats display the elements correctly, but not when the user is actually interacting. They either say "An error has occurred, or loading, or some other error.

I believe this is related to Authentication but I have included examples with the data layer too.

Looking at the diff between these versions I cannot figure it out! @dokterbob @willydouhard any ideas?

Note

Elements do not work when only using authentication (no data layer)

Example 1

import chainlit as cl
import pandas as pd
import chainlit.data as cl_data
from chainlit.data.sql_alchemy import SQLAlchemyDataLayer
from chainlit.data.storage_clients.azure import AzureStorageClient

adls = AzureStorageClient(account_url=..., container=..., credential=credential, sas_token=...)
db = cl_data._data_layer = SQLAlchemyDataLayer(conninfo=..., ssl_require=True, storage_provider=adls, user_thread_limit=100, show_logger=True)

@cl.oauth_callback
def login(provider_id: str, token: str, raw_user_data: Dict[str, str], default_user: cl.User, id_token: Optional[str]) -> Optional[cl.PersistedUser]:
    return default_user

@cl.on_chat_start
async def start():
    text_content = "Hello, this is a text element."
    elements = [
        cl.Text(name="simple_text", content=text_content, display="inline")
    ]

    await cl.Message(
        content="Check out this text element!",
        elements=elements,
    ).send()

image

Example 1 (Auth Only)

import chainlit as cl

@cl.oauth_callback
def login(provider_id: str, token: str, raw_user_data: Dict[str, str], default_user: cl.User, id_token: Optional[str]) -> Optional[cl.PersistedUser]:
    return default_user

@cl.on_chat_start
async def start():
    text_content = "Hello, this is a text element."
    elements = [
        cl.Text(name="simple_text", content=text_content, display="inline")
    ]

    await cl.Message(
        content="Check out this text element!",
        elements=elements,
    ).send()

image

Example 2

import chainlit as cl
import pandas as pd
import chainlit.data as cl_data
from chainlit.data.sql_alchemy import SQLAlchemyDataLayer
from chainlit.data.storage_clients.azure import AzureStorageClient

adls = AzureStorageClient(account_url=..., container=..., credential=credential, sas_token=...)
db = cl_data._data_layer = SQLAlchemyDataLayer(conninfo=..., ssl_require=True, storage_provider=adls, user_thread_limit=100, show_logger=True)

@cl.oauth_callback
def login(provider_id: str, token: str, raw_user_data: Dict[str, str], default_user: cl.User, id_token: Optional[str]) -> Optional[cl.PersistedUser]:
    return default_user

@cl.on_chat_start
async def start():
    iris = pd.read_csv(
        "https://raw.githubusercontent.com/mwaskom/seaborn-data/master/iris.csv"
    )

    elements = [cl.Dataframe(data=iris_doubled, display="inline", name="Dataframe")]

    step = cl.Step(name="dataframe", parent_id=cl.context.current_step.id)
    await step.stream_token("This message has a Dataframe")
    step.elements = elements
    await step.update()

image

@dosubot dosubot bot added auth Pertaining to authentication. bug Something isn't working labels Oct 23, 2024
@hayescode
Copy link
Contributor Author

@dosu any ideas?

Copy link

dosubot bot commented Oct 23, 2024

The issue you're experiencing with elements not displaying when using authentication in version 2.0.dev1 could be related to changes in how authentication and data layers interact with the UI rendering process. Here are a few things you might want to check or try:

  1. Authentication Callback: Ensure that the @cl.oauth_callback function is correctly implemented and that it returns a valid cl.PersistedUser object. Any issues in this callback might prevent the UI from rendering elements properly.

  2. Error Handling: Check if there are any error logs or console messages that provide more details about the error. This might give you clues about what is going wrong during the element rendering process.

  3. Element Initialization: Make sure that the elements are being initialized and passed correctly to the cl.Message. Any issues in the element creation or their properties might cause rendering issues.

  4. Version-Specific Changes: Since there might be changes between versions, review the release notes or documentation for any breaking changes or new requirements related to authentication and element rendering.

  5. Simplify the Setup: Try running a minimal example without the data layer to isolate whether the issue is specifically with authentication or if it's a combination of both authentication and data layer.

If these steps don't resolve the issue, it might be helpful to look into any recent changes in the Chainlit codebase related to authentication and UI rendering, as there might be undocumented changes affecting this behavior.

To continue talking to Dosu, mention @dosu.

@hayescode hayescode changed the title Elements don't display when using authentication in 2.0.dev1 Elements don't display when using authentication in 1.3.0 & 2.0.dev1 Oct 23, 2024
@hayescode
Copy link
Contributor Author

FOUND IT! @dokterbob #1441 is the culprit. Reverting just this change fixes it. It looks like @wenboown found this issue as well.

@wenboown
Copy link

yes, it took me a full day to debug this problem since it is not obvious where to look. Hopefully the team can provide a patch soon.

@hayescode
Copy link
Contributor Author

@wenboown 100% this was difficult to find.

The issue is specifically here. Commenting this out makes Elements work properly again.

@dokterbob @willydouhard I'm going to make a PR to revert this.

@xavidop
Copy link

xavidop commented Oct 23, 2024

I am facing the same issue, gonna revert back to 1.2.0 for now

@hayescode
Copy link
Contributor Author

#1474

@dokterbob
Copy link
Collaborator

Thanks for your courage figuring this out. I think I see the problem already, let me attempt a quick fix.

@dokterbob
Copy link
Collaborator

dokterbob commented Oct 24, 2024

I'd like to try for a proper fix rather than re-introducing a significant security issue.

Replication

I just replicated it with this minimal example:

from typing import Dict, Optional
import chainlit as cl


@cl.password_auth_callback
def auth_callback(username: str, password: str):
    # Fetch the user matching username from your database
    # and compare the hashed password with the value stored in the database
    if (username, password) == ("admin", "admin"):
        return cl.User(
            identifier="admin", metadata={"role": "admin", "provider": "credentials"}
        )
    else:
        return None


@cl.on_chat_start
async def start():
    text_content = "Hello, this is a text element."
    elements = [cl.Text(name="simple_text", content=text_content, display="inline")]

    await cl.Message(
        content="Check out this text element!",
        elements=elements,
    ).send()

Observations

  • When I disable auth, the problem doesn't occur.
  • When I remove the if current_user: sections in get_file and upload_file, it still occurs.
  • When I remove current_user: Annotated[Union[User, PersistedUser], Depends(get_current_user)] from get_file, it doesn't occur.

Root cause

It seems that the client is not providing the required Authorization header:
image

Responsible code in frontend

Two places (get and upload).

Upload is already supplying the authorization header:

Getter just uses a url and session id property. Perhaps we can add useAuth here to get the token and set the accessToken on the element, generating the request that way?

if (!element.url && element.chainlitKey) {

It seems that all elements use useFetch(), which is only used for fetching elements (data frames, plotly and text):
image

Could we simply add the access token to useFetch()?

Alternatively, if we migrate to using a cookie for authentication, it would solve a lot of potential security pit falls and we won't need to supply auth headers explicitly.

@hayescode
Copy link
Contributor Author

@dokterbob Feel free to close my PR if you can fix this without reverting the reason this was added in the 1st place. Once merged please make new releases because besides this issue this release improved lots of other things the community wants but we can't upgrade because of this.

@dokterbob
Copy link
Collaborator

@dokterbob Feel free to close my PR if you can fix this without reverting the reason this was added in the 1st place. Once merged please make new releases because besides this issue this release improved lots of other things the community wants but we can't upgrade because of this.

I'm on it! Thanks for the report!

dokterbob added a commit that referenced this issue Oct 24, 2024
@dokterbob
Copy link
Collaborator

This is far as I've gotten today. Test replicates it but useFetch() is failing somehow. I'm not at all experienced with React (more of a Vue guy) so any help here is appreciated: https://github.com/Chainlit/chainlit/tree/dokterbo/1472-elements-data-layer

@dokterbob
Copy link
Collaborator

Maybe we can even fetch stuff in the same way we do elsewhere in the API and get rid of the entire useFetch system?

@wenboown
Copy link

Alternatively, if we migrate to using a cookie for authentication, it would solve a lot of potential security pit falls and we won't need to supply auth headers explicitly.

How much work would this involve and whether it is on your roadmap for future development already? It might be better to "do it properly" rather than quick patch if you already plan to spend time on migrating to cookie?

@hayescode
Copy link
Contributor Author

I'm not at all experienced with React

@dokterbob me neither, sorry i can't help on that front

@hayescode
Copy link
Contributor Author

@dokterbob why don't you merge #1474 and make a 1.3.1 release while you figure this out if it's going to take more than 1 day? The last stable release imo is 1.1.404 which is what my users are on and that release contains this "massive vunerability" that exists since forever. At least a new release would be useable in the meantime.

@dokterbob
Copy link
Collaborator

@dokterbob why don't you merge #1474 and make a 1.3.1 release while you figure this out if it's going to take more than 1 day? The last stable release imo is 1.1.404 which is what my users are on and that release contains this "massive vunerability" that exists since forever. At least a new release would be useable in the meantime.

In short, I'm reluctant shipping a release knowingly re-introducing a high risk security issue. That doesn't seem to me to be the responsible thing to do. I hope to push a fix out this morning though.

In the mean time, you are welcome to just run chainlit from the hotfix branch:

pip install git+https://github.com/hayescode/chainlit@patch-3#subdirectory=backend/

@dokterbob
Copy link
Collaborator

@hayescode Are you on Discord? Would love to connect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Pertaining to authentication. bug Something isn't working needs-triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants