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

[WIP] Better auth #60

Closed
wants to merge 63 commits into from
Closed

Conversation

vporton
Copy link
Contributor

@vporton vporton commented Oct 19, 2023

This is a work in progress to solve issue #10. This PR is dependent on PR #56.

@apssouza22
Copy link
Owner

@apssouza22 I have a question for you:

        this.sessionData = {
            token: params["token"] || "",
            user: params["user"] || "",
            app: params["app"] || "chat",
            isPluginMode: params["pluginMode"]=="true" || false,
            appColor: params["appColor"] || "black",
            sessionId:  localStorage.getItem("sessionId"),
        }

app is stored in the session. Does it mean that the UI is centered on one app at a time? If yes, why so? AI is able to keep in memory several apps simultaneously. For me it seems a wrong design decision to work with just one app. Please, explain.

@vporton apps here is chatflow app. We can not run more than one at the time

@vporton
Copy link
Contributor Author

vporton commented Oct 21, 2023

apps here is chatflow app. We can not run more than one at the time

@apssouza22 A chatflow app is any API that user documented, right? Or what is it?

Why can't we run more than one at a time?

@apssouza22
Copy link
Owner

Yes, but os docs will be linked to one app. It is just one way of organizing things.

One user can have multiple apps and one app can have multiple docs

@vporton
Copy link
Contributor Author

vporton commented Oct 22, 2023

@apssouza22 What are "os docs"?

You didn't answer why we need to store the app in session data, not instead decide the app by the AI at each step (not the entire session) separately.

@apssouza22
Copy link
Owner

@vporton because u need to search only docs for the the given app.

Don't make a big deal of it. It is just app id in the session to tell the backend where to retrieve docs from

@vporton
Copy link
Contributor Author

vporton commented Oct 27, 2023

A good behavior of the app is to expire authentication after a certain time interval after the last operation that the user did.

Because expiration time is stored on the client side, to implement this delayed expiration, we need to re-download JWT token on each request (not only on login/signup requests).

I propose either:

  1. to download the token in a header like X-Auth-Token on every request (a little inefficient because needs to create and transfer JWT token on every request)
  2. move expiration to server-side (remove exp from the token and let the server decide).

@apssouza22 I need your principal agreement for either 1 or 2. I prefer 2.

@apssouza22
Copy link
Owner

A good behavior of the app is to expire authentication after a certain time interval after the last operation that the user did.

Because expiration time is stored on the client side, to implement this delayed expiration, we need to re-download JWT token on each request (not only on login/signup requests).

I propose either:

  1. to download the token in a header like X-Auth-Token on every request (a little inefficient because needs to create and transfer JWT token on every request)
  2. move expiration to server-side (remove exp from the token and let the server decide).

@apssouza22 I need your principal agreement for either 1 or 2. I prefer 2.

Let's stick with the standard. If the token is expired the user need to loging again.
Indeed we are not redirecting the user to login again if the token is expired. If you want to address that, it should be a simple PR in makeRequest call to look for response code 401

@apssouza22 apssouza22 closed this Nov 29, 2023
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.

2 participants