Auth header in Source packages and Frontity Query Options

mmczaplinski · 11 September 2020 21:03

Fair enough, makes sense

I don’t quite see how this relates to adding custom headers. Could you explain?

mmczaplinski · 12 September 2020 00:57

I see one slight complication for implementing this:

We need to add a headers parameter to fetch() which is inside of the libraries.source.get function. So, the new signature of the fetch would be:

fetch(url, { headers });

The fetch is called here. The value of the headers parameter should be read from state.source.headers.

However, inside the libraries.source.get function we don’t have access to the frontity state!

This is a problem if the user wants to dynamically change the value of the state.source.headers and have the new headers present in the subsequent API call.

For the moment I only thought of the following solutions:

Add the state as a parameter to the libraries.source.get function . I guess this would be a horrible API but I’m mentioning it anyway.
Ignore the problem. Then we could pass the state as a parameter in the init() method of the Api here. However, this way, the user will only be able to set the headers when the app initializes (e.g. by putting them in the frontity.settings.js) and will not be able to change them after that.

Right now I just followed number 2., but I’d like to hear if anyone has another idea.

luisherranz · 14 September 2020 07:35

Yep, you’re right, that is a problem. Good catch.

Sometimes I think about adding access to state to functions in libraries but it never convinced me. I think it won’t be a good pattern. I think it’s better to pass it to the library when it’s needed.

One of the reasons is that in the future DevTools if a library mutates the state, the mutation will be attributed to the action that used the library, not to the library itself. Which in my option is correct, because gives the developer the information it needs to start debugging the problem.

A third option would be to add a headers option to libraries.source.api.get to make it more imperative:

const response = await libraries.source.api.get({
  endpoint: "posts",
  params: { slug, _embed: true, ...state.source.params },
  headers: { ...myHeaders, ...state.source.headers },
});

This would mean that it’s up to the handler creator to add the headers or not.

On the other hand, I’ve been studying all the standard request headers taking into account the headers forbidden by the browsers and I could not find any other header that makes sense, including:

The Accept header is often used to specify the type of content the browser will accept, but:
- It is optional. If not present, the browser will accept any type of content.
- People have control over their backends. If they create a custom endpoint, they can set Content-Type to text/html for example and they don’t need the Accept header.
The Content-Type header is not useful for GET requests because they don’t have a body.

That means that, even though I always try to propose solutions that are as wide as possible in terms of functionality, maybe it doesn’t make much sense for this particular case and it’d be better to add an auth field instead, to make it more implicit.

So, an alternative would be to use state.source.auth:

const state = {
  source: {
    auth: "Basic YWFhOmJiYg",
  },
};

We should then let the handler creators decide if they want to use auth for their request or not by like this:

const response = await libraries.source.api.get({
  endpoint: "posts",
  params: { slug, _embed: true, ...state.source.params },
  auth: state.source.auth,
});

And we can implement Michal’s suggestion to delete the state.source.auth value in the afterSSR action of wp-source by default:

const actions = {
  source: {
    afterSSR: ({ state }) => {
      delete state.source.auth;
    },
  },
};

And that would mean that people wanting to use the Preview in the Decoupled mode wouldn’t even have to create a package, adding the token to their frontity.settings.js file would be enough.

const settings = {
  // ...
  packages: [
    {
      name: "@frontity/wp-source",
      state: {
        source: {
          api: "https://wp.mydomain.com/wp-json"
          auth: `Bearer ${process.env.JWT_TOKEN}`,
        },
      },
    },
  ],
};

@mmczaplinski, thoughts? Should we stick to a general state.source.headers or should we use state.source.auth?

mmczaplinski · 14 September 2020 16:56

I think it makes sense that we only add state.source.auth in that case! Thanks for the proposal

If we are going to put state.source.auth in the state and recommend that the users use an env variable to load it’s value, what do you think if we support an “official” frontity env variable like FRONTITY_AUTH_TOKEN (the name is up for discussion)?

Then we could do one of two things:

populate state.source.auth with the value of FRONTITY_AUTH_TOKEN automatically. Then, if state.source.auth is also set, it would override the value from the env variable.
Require the user to specify the auth header directly and only in FRONTITY_AUTH_TOKEN and not put it in the state. Then, we could just use the value of FRONTITY_AUTH_TOKEN directly in the libraries.source.api.get().

This has the obvious disadvantage that the token would not be present in the state, but I wonder if this is really necessary? Any package that would need to read the value of the auth token, could still read it from that env variable. And since we are only dealing with the Authorization header now, I think we don’t need to (and shouldn’t) allow the users to change them dynamically.

The advantage would be that is that:
1. We don’t have to delete the token in afterSSR.
2. It is not really possible for the user to “accidentally” expose their token this way. I’m worried that despite our best efforts some users will try to set their tokens from the client or hardcode them in their frontity.settings.js file and push the code to github.

What do you think?

luisherranz · 15 September 2020 07:58

Uhm, it’s an interesting idea.

That’s a really good point.

The problem I see is that the APIs for the server and client would be different. A handler would have to implement both:

const myHandler = async ({ state, libraries, link }) => {
  const response = await libraries.source.api.get({
    endpoint: "post",
    params: { slug: params.slug },
    auth: process.env.FRONTITY_AUTH_TOKEN || state.source.auth,
  });
};

We should try to simplify that.

Also, a static token is not going to be the only way to add auth. For example:

The token may be in a URL query, like in the WordPress Preview.
The token may need to be retrieved first from an API, using a refresh token.

So maybe we can stick to state.source.auth so the handlers don’t have to take into account different server/client implementations, but add support for FRONTITY_AUTH_TOKEN by linking it’s content to state.source.auth the same way we are going to link the preview token. And make that the recommended way to add a static token.

By the way, I think this is a great moment to add automatic support for the .env file by using dotenv in our CLI, don’t you think?

luisherranz · 15 September 2020 08:10

More thoughts:

I think I wouldn’t name it token, because sometimes it won’t be a token. Maybe something like FRONTITY_SOURCE_AUTH that contains the source namespace and relates to state.source.auth:
```
// Basic auth
FRONTITY_SOURCE_AUTH="Basic YWFhOmJiYg";
// JWT static token
FRONTITY_SOURCE_AUTH="Bearer As24dO.yrTT4a.9TXAr1";
```
I proposed to have a variable query, like this: ?frontity_preview_jwt_token_v1=... in the WordPress preview support thread because I thought it’d be more versatile in case we want to change it in the future.

That may make sense from the WordPress perspective, but if we add the Basic or Bearer part to the query, I guess doesn’t make sense for the Frontity side.
```
// Basic auth
"/some-post?frontity_source_auth=Basic%20%YWFhOmJiYg"
// JWT static token
"/some-post?frontity_source_auth=Bearer%20%As24dO.yrTT4a.9TXAr1"
```

luisherranz · 15 September 2020 16:55

Sorry Michal, I missed this option!

Yeah, let’s do that

And the same for the frontity_source_auth query

mmczaplinski · 15 September 2020 17:11

yeah, no worries

yeah, you’re totally right about that. I think a static token will not make sense because of this.

Yep, that is a better name indeed.

Agreed. I ll summarize it all in the implementation proposal.

mmczaplinski · 15 September 2020 21:00

Implementation Proposal

This feature will need several components:

Add a state.source.auth property which will hold the authentication information. This could be a JTW token or a Basic Authentication string, or something else. The user can then pass a value to state.source.auth e.g. via frontity.settings.js or by setting it just like any other piece of frontity state.
```
// frontity.settings.js
const state = {
  source: {
    auth: "Basic YWFhOmJiYg",
  },
};
```
When the frontity_source_auth parameter is present in the query string, use its value to populate state.source.auth. This can be done inside actions.source.init()
If a FRONTITY_SOURCE_AUTH environment variable is set, its value should automatically be passed to state.source.auth. This can be done inside actions.source.init()

Modify libraries.source.api.get() to accept an auth parameter:

const response = await libraries.source.api.get({
  endpoint: "posts",
  params: { slug, _embed: true, ...state.source.params },
  auth: state.source.auth,
});

state.source.auth should be removed in the afterSSR action:
```
const actions = {
  source: {
    afterSSR: ({ state }) => {
      delete state.source.auth;
    },
  },
};
```
For this to work, we will also have to move the afterSSR action to run before taking the snapshot of the state, as mentioned by Luis before.

Bonus:

Let’s support for the .env file

mmczaplinski · 15 September 2020 22:50

Since we ll have to change the signature of libraries.source.api.get() the questions is:
should we change every handler in wp-source to pass the new auth parameter.

Or, should we just change the handler for postType, which is the one that is going the be required for the preview functionality.

luisherranz · 16 September 2020 08:04

As making authenticated requests bypasses the REST API cache, I would add the auth field only to the fetches that require authentication, like the revision ones for the preview.

When the token is sent via query parameter in the preview bypassing the cache may not be a problem, but if someone is using the JWT plugin to generate a permanent auth token and adds it to the FRONTITY_SOURCE_AUTH env variable, that token will be present on all Frontity requests, not only the ones with preview=true, and therefore in that situation, none of the REST API requests will be cached, which is wrong.

I think we can do that in the actions.source.init() action instead, whose purpose is that kind of initializations. Same for the FRONTITY_SOURCE_AUTH env variable. That way if other package wants to use state.source.auth or check for its existence before there’s been a call to actions.source.fetch(), everything works.

Excellent IP Michal

I think we can add another small feature to this:

Rename ?name=site to ?frontity_name=site.

The name query is too general and could collision with a real query used by the URL. We should rename it to frontity_name. It also belongs to a set of “Frontity Queries” that are special and need to be treated differently. If all of those start with frontity_ it’s going to be easier to recognize them.

The reason to do this is the next point.
Remove all the “Frontity Options” from the link.

The Frontity server adds the initialLink to the state. Then, the router uses that information to populate the state.router.link in its actions.router.init() action.

We are adding configuration queries to Frontity that are not related to the WordPress URL that the user is visiting. For example, if we use:
- When we use https://www.mydomain.com/some-post?frontity_name=site2 we are telling Frontity that it should load the site with name site2 (maybe it has a new theme you are testing), but it has nothing to do with the URL we want to load, which is https://www.mydomain.com/some-post.
  
  In this case, state.router.link should be https://www.mydomain.com/some-post and the frontity_name query should be stored in a different place.
- The same case can be the new ?frontity_source_auth=... query.
- Another case we will add soon (for the embedded mode) is the dynamic public path: ?frontity_public_path=...
My current thought is that we could remove all the queries that start with frontity_ from the initialLink at this point and create a new field for those queries. Maybe called simply options.

So, loading the https://www.mydomain.com/some-post?frontity_name=site2&frontity_source_auth=XXX URL will result in:
- state.frontity.initialLink: https://www.mydomain.com/some-post
- state.frontity.options: { name: "site2", sourceAuth: "XXX" }
Of course, any package will be able to make use of the “Frontity Queries” or “Frontity Options”, not only the Frontity core. Actually, frontity_source_auth would be a source package option.

What do you think?

mmczaplinski · 16 September 2020 22:31

Nice, I think this makes a lot of sense!

I don’t think I have anything more to add at this point

mmczaplinski · 22 September 2020 00:05

Just noting that for now I am not removing all the queries that start with frontity_ but just remove specific queries, in this case both frontity_name and frontity_source_auth and adding their values to state.frontity.options.

luisherranz · 22 September 2020 08:50

In my opinion that defeats a bit the purpose of being used freely by packages. I’ve explained the reasons a bit further in the PR

luisherranz · 29 September 2020 08:42

The PR is finished and ready to be merged.

We finally used camelcase for the auth options. For example, frontity_source_auth will end up as state.frontity.options.sourceAuth.

We finally remove both state.source.auth and state.frontity.options.sourceAuth from the state before sending it to the client to avoid exposing server tokens in the client.

Other than that, I think the rest is exactly as defined in Michal’s implementation proposal.

Awesome work!!

luisherranz · 29 September 2020 08:58

This is a brief summary of how the new Frontity Query Options work:

Any query param in the URL that starts with frontity_ is a “Frontity Query Option”.
Those queries are reserved to “send special information to Frontity”, not related to the URL that needs to be rendered (a post, a page, or whatever), but to the Frontity configuration.
Those parameters are removed from the state.frontity.initialLink and state.router.link because they should not affect the content of what needs to be rendered (a post, a page, or whatever).
Those parameters are added to state.frontity.options. Packages that use them will look for them there.
The name of those parameters is converted to camelcase. The value is not transformed.
Some of these “Frontity Query Options” will be used by the core, like for example:
- frontity_name -> state.frontity.options.name: The name of the site you want to load.
- frontity_public_path -> state.frontity.options.publicPath: The public path that you want to use (not ready yet, but it will exist).
But packages can also make use of them, like for example:
- frontity_source_auth -> state.frontity.options.sourceAuth: A authentication token for the source package.
- frontity_comments_skip -> state.frontity.options.commentsSkip: A boolean to skip rendering the comments (it’s not real, I’ve just made it up).

SantosGuillamot · 2 October 2020 13:05

This feature has been released : Read the full Release Announcement.

juanma · 10 December 2020 16:49

A post was split to a new topic: How can I add a Bearer token to the Auth headers?

mmczaplinski · 13 April 2021 19:58

I think that we should add an explicit note in the documentation about Frontity Query Options that they should be treated as an untrusted user input and never ever passed as parameters to arbitrary functions, etc.

Imagine that someone has the writes the following (very exaggerated) example:

const { foo } = state.frontity.options;
saveToDatabase(foo);              // you're screwed
runCommand(`npx frontity ${foo}`) // you're screwed even more

mburridge · 23 April 2021 13:51

@mmczaplinski Adds warning to frontity-query-options.md by mburridge · Pull Request #287 · frontity/docs · GitHub