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

Server Side Rendering #59

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

Server Side Rendering #59

wants to merge 15 commits into from

Conversation

loks0n
Copy link
Member

@loks0n loks0n commented Aug 31, 2023

No description provided.


[design-proposal]: #design-proposal

**Problem #1: Solution A - Return session token in response body**

Choose a reason for hiding this comment

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

I like solution A, can't see why not.
Such token should be hidden by default, and only exposed when creating session.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by such token should be hidden by default?

Choose a reason for hiding this comment

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

You had 2 solutions. One to have it in response body, one to set it as response header.

I like setting it in response body. But if in future you do getSession, you should no longer get secret. You should only get it once when you first create it.


**Problem #3: Solution A - setSession helper method**

A new SDK helper method, called `setSession`, to set the session token of future requests.

Choose a reason for hiding this comment

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

SetSession or SetToken? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel we generally use token for temporary keys.
This helper could take either the entire session object, or just the secret, in which case setSecret or setSessionSecret might be worth considering

Choose a reason for hiding this comment

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

We have setKey for setting API key. So to follow that we should do setSecret for setting session secret.

But that sounds so undescriptive.. I would vote for setSessionSecret, but then we should consider doing setApiKey.

Copy link
Member Author

@loks0n loks0n Sep 21, 2023

Choose a reason for hiding this comment

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

I could live with setSecret, although one advantage of setSession is it makes more sense if used in combination with setKey as may be required to solve rate limiting

5. User authenticates with the OAuth2 provider.
6. OAuth2 provider redirects the browser back to the Appwrite API.
7. Appwrite API sets the session cookie on the Appwrite domain.
8. Appwrite API redirects the browser back to the client application. **Now, the redirect URL includes a userId & temporary token.** e.g. `myssrapp.com/oauth2/success?userId=loks0n&token=TEMPORARY_TOKEN`

Choose a reason for hiding this comment

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

Currently OAuth works with 1 line of code.
Wit this, it wont. You will need to add extra logic on page load that looks for those params, and then runs method to finish sign-in process.

Similiar to Magic URL - After sending mail, you need 1 extra page that takes URL parals and finishes the logic.

That's kinda annoying :/ Can we find a way to automate this, somehow? Or simplify?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, I can't see a way to simplify this flow: we are at the mercy of browser cookie security.

Here's why:

  • We need a place to store a session on the client, and send it with browser GET requests.
  • Local storage doesn't work because browsers don't send it with requests. We need to use secure cookies.
  • Browsers only send cookies that belong to requested domain.
  • Domains may only set cookies for their own domain.

The extra page isn't so much of an issue if we have a universal token -> session endpoint, right?

Choose a reason for hiding this comment

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

I see. I agree it's better to ask developer to write more code than to have it not work on some setups like Safari or phone devices.

Might be worth checking competitors and see if they have setup this complex. if not, maybe they found some trick.

Copy link
Member Author

Choose a reason for hiding this comment

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

Supabase and Firebase are using the same method, unfortunately.


2.1. Should we remove the undocumented workaround?

2.2. Do we need to add a new endpoint to exchange the temporary token for a session token? Can we reuse the existing magic URL exchange endpoint?

Choose a reason for hiding this comment

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

I would not reuse.

Buut I like the idea. But it's of a bigger scope.

My idea is that there should be universal "token -> session" endpoint, where token would also know type of session, like magicUrl or oauth2. Why? Well because then we can add endpoint users.createCustomSession(userId), which returns such token. Then we can exchange this token for session.

This will allow fully custom auth flows. For example, with APpwrite Functions, this will allow logging into account by entering code that Discord bot sent you as private message.. Whatever you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 for Universal 'token -> session' endpoint

Choose a reason for hiding this comment

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

@eldadfux Do you think it would be worth as part of SSR rework? It is pretty highly requested feature. I think this will be most common blocker for any bigger company.

Copy link
Member

Choose a reason for hiding this comment

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

We can.

Choose a reason for hiding this comment

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

Heard of requests for this in various avenues. Is a build/scale stage issue.


**Problem #3**

3.1. Should the helper method be available in Server Side SDKs? How do we seperate requests that are authenticated with a session token, and requests that are authenticated with an API key?

Choose a reason for hiding this comment

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

I would add them to server sdks, yes. I think they could be useful.
To differentiate we can just use another header name, like x-appwtite-token

@loks0n loks0n requested a review from TGlide September 15, 2023 11:36

**What problem are you trying to solve?**

Many popular web frameworks, such as Next.js, Nuxt.jsm and SvelteKit, support and recommend SSR, by default. When developers attempt to use Appwrite Auth with these frameworks, they run into issues with the current implementation.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Many popular web frameworks, such as Next.js, Nuxt.jsm and SvelteKit, support and recommend SSR, by default. When developers attempt to use Appwrite Auth with these frameworks, they run into issues with the current implementation.
Many popular web frameworks, such as Next.js, Nuxt.js and SvelteKit, support and recommend SSR, by default. When developers attempt to use Appwrite Auth with these frameworks, they run into issues with the current implementation.

Sorry 😬

Also, I'd say the problem is not only Appwrite Auth. I've also run into issues with file uploads. I've managed to get it working, but it was troublesome to say the least.

Comment on lines 108 to 118
```js
import { Client, Account } from "appwrite";

const client = new Client();
client.setEndpoint("https://cloud.appwrite.io/v1");
client.setProject("PROJECT_ID");

const account = new Account(client);
const session = await account.createAnonymousSession();
const sessionToken = session.token;
```
Copy link

Choose a reason for hiding this comment

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

This works, but what do we then do with the sessionToken? How would a more complete flow look like?

For reference, this is what I do now to achieve SSR in auth.

Return the session cookie in the response headers.

```js
import { parse } from "set-cookie-parser";
Copy link

Choose a reason for hiding this comment

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

I don't think we should require an external lib to be able to use cookies. An inbuilt helper would be much friendlier

Copy link
Member

Choose a reason for hiding this comment

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

+1

Comment on lines 213 to 214
- Tutorial for using Appwrite and Next.js for SSR
- Tutorial for using Appwrite and SvelteKit for SSR
Copy link

Choose a reason for hiding this comment

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

We should include more frameworks here

Copy link
Member

Choose a reason for hiding this comment

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

+1


2.2. Do we need to add a new endpoint to exchange the temporary token for a session token? Can we reuse the existing magic URL exchange endpoint?

2.3. The Create OAuth2 Session endpoints are not included with Server Side SDKs. Should we include them? If so, server side apps need the Location header to forward to the user. How should we make this accessible from the SDK?
Copy link

Choose a reason for hiding this comment

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

By server-side SDK, do you mean node-appwrite?

Copy link

Choose a reason for hiding this comment

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

If so, would you require to have both node-appwrite and appwrite installed in an SSR framework? When would each be used?

Copy link
Member Author

@loks0n loks0n Sep 15, 2023

Choose a reason for hiding this comment

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

By server-side SDK, do you mean node-appwrite?

Yeah, node-appwrite and the other server SDKs

If so, would you require to have both node-appwrite and appwrite installed in an SSR framework? When would each be used?

This is what I'm getting at, in an ideal world I think we should have a single SDK that handles any compatibility issues.
Second best and easier option is to recommend using node-appwrite in server code and appwrite in all client code.

Copy link
Member

Choose a reason for hiding this comment

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

I guess for all fullstack / web work you should use appwrite for any admin related tasks you should use node-appwrite which is basically and admin (or console) like SDK.


**General**

4.1. Incorporating some of these SSR changes reduces the distinction between the Server Side and Client Side SDKs. Which SDKs would be recommended for SSR applications?
Copy link

Choose a reason for hiding this comment

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

Exactly, it gets harder to differentiate. Also, I worry about rate-limiting, which would currently happen using SSR, right?

Copy link
Member

Choose a reason for hiding this comment

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

@loks0n let's check where can rate limit get in the way. If our key is email+ip we should be fine. If the abuse key for an endpoint is only for IP it will be useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rate limiting is an issue, I've added a section outlining some possible solutions

});
```

We can provide a helper method for popular SSR frameworks to set the cookie in the response.
Copy link

Choose a reason for hiding this comment

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

Love this! Would be great for adoption.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, this is a must. Can we include it in the main SDK or do we have to create separated packages for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be in the main SDK :)

Choose a reason for hiding this comment

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

@loks0n What's the expected effect on package size, build time, etc.

If it's trivial we can add, but I wonder when we draw the line for framework specific baggage in the main SDK

Copy link

Choose a reason for hiding this comment

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

@gewenyu99 in general nowadays tree-shaking should deal with most issues regarding bundle size on your app's final output, build time too. The package size would be bigger on the node-modules folder though.

Alternatively, you can monorepo the SDK, and add adapters. E.g. @appwrite.io/adapter-svelte

loks0n and others added 2 commits September 15, 2023 12:54
Co-authored-by: Thomas G. Lopes <26071571+TGlide@users.noreply.github.com>
@eldadfux
Copy link
Member

Can we try and redirect OAuth flow back to the SSR server itself with relevant params (including a secret) on a dedicated route and use it to set the local cookie in a similar way to what we do for a regular session?

@loks0n
Copy link
Member Author

loks0n commented Sep 20, 2023

Can we try and redirect OAuth flow back to the SSR server itself with relevant params (including a secret) on a dedicated route and use it to set the local cookie in a similar way to what we do for a regular session?

Are you suggesting to replace the redirect from the OAuth2 provider to Appwrite with OAuth2 to SSR? The Appwrite session is not yet created at this point, and therefore the server would still need to exchange the temporary token

@loks0n loks0n mentioned this pull request Oct 11, 2023
5 tasks
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.

6 participants