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

Fix SDK usage of Auth (JSON vs Cookie, Static vs Dynamic) #7112

Closed
3 tasks done
kees-tce opened this issue Jul 31, 2021 · 19 comments · Fixed by #7174 or #9080
Closed
3 tasks done

Fix SDK usage of Auth (JSON vs Cookie, Static vs Dynamic) #7112

kees-tce opened this issue Jul 31, 2021 · 19 comments · Fixed by #7174 or #9080
Assignees

Comments

@kees-tce
Copy link

kees-tce commented Jul 31, 2021

Preflight Checklist

Describe the Bug

Not sure if it is supposed to work but when I copy paste the advanced example from the SDK docs here it fails to run and errors out at await auth.refresh()

To Reproduce

Copy paste the advanced solution and try to run it.

What version of Directus are you using?

RC86

What version of Node.js are you using?

Na

What database are you using?

Na

What browser are you using?

All

What operating system are you using?

Na

How are you deploying Directus?

Na

@rijkvanzanten
Copy link
Member

Seeing that that example starts with WARNING: OK, not exactly a warning, but this isn't needed at all., I figured it was a little less confusing to just not have that (broken) example in there at all. Was there anything in the regular examples you were trying to do but got stuck on @kees-tce?

Groetjes!

@j3n57h0m45
Copy link
Contributor

j3n57h0m45 commented Aug 6, 2021

@rijkvanzanten
ok, so with the advanced example gone... how can I set up the directus authentication to force json - while keeping Transport and Storage as the default values?

The json mode is quite useful if utilizing directus auth within 3rd party apps (such as svelte).

@kees-tce
Copy link
Author

kees-tce commented Aug 6, 2021

@rijkvanzanten
ok, so with the advanced example gone... how can I set up the directus authentication to force json - while keeping Transport and Storage as the default values?

The json mode is quite useful if utilizing directus auth within 3rd party apps (such as svelte).

@rijkvanzanten this was exactly what I was also trying to achieve, so I guess this is the answer to your question above :)

@keesvanbemmel
Copy link
Contributor

Bump @rijkvanzanten :)

Of in mooi Nederlands: vergeet-me-nietje!

@j3n57h0m45
Copy link
Contributor

This it what I use in a svelte project. However you need to make sure that the window object is available or LocalStorage() will fail.

import { Directus, Auth, LocalStorage, AxiosTransport } from '@directus/sdk'

let sdk = {}

const url = ''
const storage = new LocalStorage() // : new MemoryStorage()
// Transport used to communicate with the server.
const transport = new AxiosTransport(url, storage, async () => {
  await auth.refresh() // This is how axios checks for refresh
})
const options = {
  mode: 'json',
  refresh: { auto: true, time: 3000}
}
// Auth is how authentication is handled, stored, and refreshed.
const auth = new Auth(transport, storage, options)
sdk = new Directus(url, { auth, storage, transport })

export { sdk }

@keesvanbemmel
Copy link
Contributor

// This is how axios checks for refresh

Thanks for this! However, the problem with this example (and the one in the docs) is that your line which says "// This is how axios checks for refresh" gives me "compile" errors because "auth" is not available there.

@keesvanbemmel
Copy link
Contributor

Bump @rijkvanzanten :)

Of in mooi Nederlands: vergeet-me-nietje!

Not sure how to reach you best @rijkvanzanten :)

@rijkvanzanten
Copy link
Member

You say my name twice holding a lit candle, and I'll appear in your shadow

The current implementation of the SDK is clearly not intuitive enough for this sort of use case. I think the best approach is to tweak the SDK a bit to make the switch between doing things "automagically" under the hood, and doing things manual with the tokens in JSON more user friendly. The same thing applies to using the SDK with a static token, having to override the auth class and whatnot just to have it rely on a static token is a bit overkill.

Lets use this thread and issue as a way to discuss the ideal way this should work. I believe the changes that would be required to make this intuitive are minimal (most if not all the methods for data are fine, it's mostly the configuration/usage of authentication that's a bit finicky), so hopefully we can figure it out and work it in before the next release 👍🏻

@rijkvanzanten rijkvanzanten reopened this Aug 26, 2021
@rijkvanzanten rijkvanzanten changed the title SDK doc example incorrect Fix SDK usage of Auth (JSON vs Cookie, Static vs Dynamic) Aug 26, 2021
@keesvanbemmel
Copy link
Contributor

I would love to talk you through my usecase and see what we can come up with. I have a working auth implementation for Vue Storefront, but it seems a little bloated.

Perhaps schedule a 15 minute call somewhere next week?

@rijkvanzanten
Copy link
Member

rijkvanzanten commented Aug 27, 2021

Totally! Are you on our Discord yet? That's a little easier to schedule 🙂 Just shoot me a DM there, my user is Rijk#8055

@rijkvanzanten
Copy link
Member

@keesvanbemmel 😄

image

@jacobrienstra
Copy link
Contributor

jacobrienstra commented Sep 3, 2021

Hello all, working on fixing the auth for the SDK, and thought I'd outline the scenarios for which we want it to work, so we can better triage how it will work.

  1. Static Token (great, very easy to do)
  2. Dynamic Token
    • Browser
      1. Auto refresh just works, uses cookies
      2. Auto refresh just works, uses jwt
      3. Manual refresh/token management
    • Nodejs
      1. Auto refresh just works, uses jwt
      2. Manual refresh/token management

Does this seem like it covers everything? Are there any customizations people may want to make that this wouldn't allow for?
I think the problem before was that the defaults for Transport or Storage weren't created usually. I'm not sure we need a use case for custom Transport, honestly, but I could be wrong.

@rijkvanzanten
Copy link
Member

Does this seem like it covers everything?

I believe these three would cover all the use cases, yeah

I'm not sure we need a use case for custom Transport, honestly, but I could be wrong.

I agree with that. We can just make axios an internal dependency. No need to overcomplicate that.

@joselcvarela
Copy link
Member

joselcvarela commented Oct 19, 2021

Hello all,
After studying a bit the SDK these are my conclusions:

  • Is very flexible, since it allows developers pass their custom handlers for almost everything;
  • Has default handlers in case users do not pass them
  • The default handler for authentication tries to do refreshes automatically. In my opinion, this is a good point.

So, in the end, my suggestion is to just to handle this with something like (main...joselcvarela:issues/7112):

sdk.auth.mode = 'json'

Let me hear your thoughts @rijkvanzanten .

@rijkvanzanten
Copy link
Member

@joselcvarela I think the usage outlined above by @j3n57h0m45 is way too cumbersome for what it is:

import { Directus, Auth, LocalStorage, AxiosTransport } from '@directus/sdk'

let sdk = {}
const url = ''
const storage = new LocalStorage() // : new MemoryStorage()
// Transport used to communicate with the server.
const transport = new AxiosTransport(url, storage, async () => {
  await auth.refresh() // This is how axios checks for refresh
})
const options = {
  mode: 'json',
  refresh: { auto: true, time: 3000}
}
// Auth is how authentication is handled, stored, and refreshed.
const auth = new Auth(transport, storage, options)
sdk = new Directus(url, { auth, storage, transport })

export { sdk }

Ideally, I'd like that to become as simple as possible, with ways to override certain internals, for example:

import { DirectusSDK } from '@directus/sdk';

const sdk = new DirectusSDK({
  url: 'https://example.com',
  token: 'my-static-secret-token'
});

for static tokens, or

import { DirectusSDK } from '@directus/sdk';

const sdk = new DirectusSDK({
  url: 'https://example.com',
  auth: {
    autoRefresh: true
  }
});

where it "just works". Then for cases where you need a specific storage override, you can do something like

import { DirectusSDK } from '@directus/sdk';
import localForage from 'localforage';

const sdk = new DirectusSDK({
  url: 'https://example.com',
  auth: {
    autoRefresh: true,
    storage: localForage.createInstance({ name: 'session-storage' })
  }
});

@joselcvarela
Copy link
Member

@rijkvanzanten yeah I think that too.
Although, what they were talking is about the mode passed to Auth. This defines if SDK should use cookies or jwt. By default, it uses cookies for browser and jwt for environments without window like Node.js.
My suggestion is expose everything on directus instance.
By default, this should work

const sdk = new Directus('http://localhost:8055', 'static_token')
// OR
// const sdk = new Directus('http://localhost:8055')
// await sdk.auth.login({email, password})
await sdk.items('articles').readMany();

But for a more configurable way, we could provide this:

const sdk = new Directus('http://localhost:8055')
sdk.auth.mode('json').autoRefresh(true).deltaTime(3000)
sdk.auth.storage.prefix('_my_app_')

@rijkvanzanten
Copy link
Member

But for a more configurable way, we could provide this:

const sdk = new Directus('http://localhost:8055')
sdk.auth.mode('json').autoRefresh(true).deltaTime(3000)
sdk.auth.storage.prefix('_my_app_')

That way of relying on method calls for an initial setup feels a little strange to me. Any reason why that can't be part of a config object in the constructor of that Directus class?

@joselcvarela
Copy link
Member

Since we need to declare this function calls anyway, it is more clean.
Also, I think it could be more intuitive to have methods to define configurations instead of relying on a single object of configurations.
Also it gives the flexibility to developers to extend functionalities.

But you've got a point, the well known packages uses a configuration, so maybe we should go that way.

PS: I like more the chainable way 😅

@rijkvanzanten
Copy link
Member

There's no reason why we couldn't have both tho! I do believe however that for the 80/20 case it's easier to have a single config object to initialize with, and then have the methods available to adjust later on if you ever have the use case to change the values after the fact 👍🏻

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.