OAuth2

[jgaehring]

I started these notes in a code comment at line 4, but it quickly went out of scope to cover some broader thoughts about the new API for this library. I'm just going to leave it here so we can continue to discuss.

Per https://github.com/farmOS/farmOS-client/pull/350#discussion_r428747370, I think it could be nice to change this function signature to 2 parameters: first, a string for the host (our one required param), and second an options object, which includes the clientId and tokenUpdater, as well as an optional token, if the caller already has one. The simplest implementation might look something like:

function farmOS(host, opts) {
  const {
    clientId = 'farm',
    tokenUpdater = null,
    token: initToken = null,
  } = opts;
  // etc...
}

(Note: I'm renaming the property initToken so it doesn't collide with your other useages of token elsewhere in the code.)

That makes me wonder, also, about providing a special default for the tokenUpdater, so:

function farmOS(host, opts) {
  function defaultUpdater(token) {
    localStorage.setItem('farmos-token', JSON.stringify(token));
  }
  const {
    clientId = 'farm',
    tokenUpdater = defaultUpdater,
    token: initToken = null,
  } = opts;
  // etc...
}

And that in turn makes me wonder if there should be complimentary getter and setter params:

function farmOS(host, opts) {
  function defaultSetToken(token) {
    localStorage.setItem('farmos-token', JSON.stringify(token));
  }
  function defaultGetToken() {
    return JSON.parse(localStorage.getItem('farmos-token')):
  }
  const {
    clientId = 'farm',
    setToken = defaultSetToken,
    getToken = defaultGetToken,
    token: initToken = null,
  } = opts;
  // etc...
}

Although I guess the main problem with that is it hardcodes localStorage as a dependency, which would break in Node.js, but that could sol with a simple check on the environment: ved

function farmOS(host, opts) {
  let defaultSetToken;
  let defaultGetToken;
  if (!!window && 'localStorage' in window) {
    defaultSetToken = (token) => {
      localStorage.setItem('farmos-token', JSON.stringify(token));
    }
    defaultGetToken = () => {
      return JSON.parse(localStorage.getItem('farmos-token')):
    }
  }
  const {
    clientId = 'farm',
    setToken = defaultSetToken, // will default to `undefined` if there's no localStorage
    getToken = defaultGetToken, // will default to `undefined` if there's no localStorage
    token: initToken = null,
  } = opts;
  // etc...
}

I haven't totally worked out what implications this getter/setter combo would have on the rest of the OAuth implementation, b/c again, I'm not 100% on the way tokens are currently being refreshed etc, but it may provide a way around the need for the "lazy initializer" we're using in Field Kit at the moment. I know @paul121 brought up the issue that if different modules (eg, the authModule and http/module) are creating separate instances of the client, no amount of internal state will be able to communicate that across module boundaries, but if each instance is passed its own getter for accessing the global state somehow, either via localStorage or some other persistance medium or state mechanism. I think this is preferable to appending the token to the window object or something like that, b/c it's just not best practices to pollute the global namespace that way, and b/c it will still require a call to .useToken() or something like that before every call to the server.

So I don't know if I have any definite conclusions as of yet, but I guess I'm hoping this could inform the eventual shape of the API, so in FK we could just do something like this at the top of every module that needs to use farmOS.js:

const host = localStorage.getItem('host');
const getToken = () => JSON.parse(localStorage.getItem('token'));
const setToken = token => localStorage.setItem('token', JSON.stringify(token));
const farm = farmOS(host, { getToken, setToken });

And that's it. No "lazy" wrapping function around farm, no calls to .useToken, just the host and a getter/setter combo. Other workflows could be made possible for other applications, either in Node or browser, but right now I don't think we should worry about those, since to my knowledge, FK is still the only application using farmOS.js.

[paul121]

I think it could be nice to change this function signature to 2 parameters: first, a string for the host (our one required param), and second an options object, which includes the clientId and tokenUpdater, as well as an optional token, if the caller already has one.

:+1: to having a second options object. That is much cleaner and better supports some of these other ideas like..

And that in turn makes me wonder if there should be complimentary getter and setter params:

Getters and setters sound like a good solution! I agree this would mostly move all internal state outside of the farmOS.js library. A lot of my thinking thus far is influenced by the Python requests-oauthlib library which provides the token_updater callback, but that only gets us half way to removing token state out of the internals. A getToken method would accomplish that.

I'd propose that the default setToken and getToken just modify an internal farm.token property. This is simple enough and that way there is no special configuration or dependency required to get the library up and running. Special use cases in Node or browser could choose otherwise.

This would largely eliminate the need for .useToken and/or an initToken :+1:

so in FK we could just do something like this at the top of every module that needs to use farmOS.js:

const host = localStorage.getItem('host');
const getToken = () => JSON.parse(localStorage.getItem('token'));
const setToken = token => localStorage.setItem('token', JSON.stringify(token));
const farm = farmOS(host, { getToken, setToken });

I think this is mostly true..

• What about the host? This isn't always known (before logging in) and could change if you logged into a different farmOS server (not sure how much we want to support that tho). I think that could have the same problem we have now without the "lazy" wrapper
• Unfortunately this doesn't solve the "batch of requests" issue. The state required for handling this still needs to exist in one client object. I don't think we could simply abstract that out with additional getters/setters.
  • But that said.... the "batch of requests" issue shouldn't be a super common problem. As long as the batch of requests are to the same client this is OK - and it seems like this would be the case if a client was created in each module that needs farmOS.js. Worst case scenario, our super solid error handling catches the any 400 refresh token error in FK and the user just needs to login again. (hopefully this would not happen often)
• I'm curious though, could that chunk of code could live in a @utils/farmClient and export the client object? It could have methods to logout and remove the client, recreate the client if the host changes, and deny attempts to make requests if no client has been created? Would there be much more involved there?

[jgaehring]

(Apologies in advance: this comment got a bit rambly as I figured out some of the implications of what I was writing as I wrote it, but it was easier to preserve the thought process rather than rewrite it.)

---

I'd propose that the default setToken and getToken just modify an internal farm.token property. This is simple enough and that way there is no special configuration or dependency required to get the library up and running. Special use cases in Node or browser could choose otherwise.

Oh brilliant! Yea, hadn't thought of that, but it makes perfect sense: make in-memory storage the default storage, which is perfectly adequate for one-off usage, like the scripting scenario you mentioned in https://github.com/farmOS/farmOS-client/pull/350#discussion_r428845866.

What about the host? This isn't always known (before logging in) and could change if you logged into a different farmOS server (not sure how much we want to support that tho). I think that could have the same problem we have now without the "lazy" wrapper

Ah dammit I think you're right. Hm, yea, seems like having a lazy initializer wrapping this is still the best remedy that I can think of, at least for now. Would really love to eliminate that though. Need to think more about this... :thinking:

The state required for handling this still needs to exist in one client object.

I'm a little confused by this statement. In my example above, w/o the lazy intitializer, how is farm not one client object? No matter how many times you call farm.log.get(), internally that references the same internal client object, the same subscribers array, etc. Any internal state is closed over by the farmOS function, so the farm object it returns (and all its methods) retains access to that state. Or is there some way that that state gets disposed of internally that I'm not aware of?

Obviously, this isn't the case if we wrap farm in a lazy intitializer, which we still have to resolve re: host etc, but if we assume for the moment we can solve that and remove the laziness, what's stopping us?

Oh, or are you referring to the fact that there would still be 2 separate instances of the client in the 2 separate modules that use it? (Sorry, just realized that. :upside_down_face:) I guess I'm not so concerned about that for the case of batching requests. I don't see any scenario where FK will have any requests for data happening in parallel with a request for authorization. I suppose if other apps were consuming this library we couldn't be assured that they'd have the same separation, but I don't feel like we need to accommodate those scenarios right now.

Ohhhh, but I see there actually is a problem where FK's updateUserAndSiteInfo action is included in authModule, and it does get called in parallel with actions from http/module in App.vue. Argh, I should have moved updateUserAndSiteInfo to http/module long ago! :facepalm:

Ok! Thanks for bearing with me, @paul121. I think I'm starting to understand! :slightly_smiling_face:

I'm inclined to say farmOS.js should take an opinionated approach, and only endeavor to support applications that don't try to execute parallel requests from separate modules. I think the onus should be on FK to structure its modules more effectively by instantiating the client in as few modules as possible, and if it has to do so in multiple modules, it takes responsibility for guaranteeing that there won't be any parallelism between those modules. In other words, we need to move updateUserAndSiteInfo to http/module at the very least, and strongly consider merging all of authModule into http/module.

Merging the 2 modules together might even alleviate the problem we have with host updating, or at least provides a cleaner solution. Analogous to .useToken(), we could just provide a method like .useHost(), which could be called by didSubmitCredentials (ugh, that name needs to change) just before it calls farm.authorize(). As long as all the other requests in http/module were using the same instance of farm as didSubmitCredentials, then problem solved!

That really seems like the direction we want to go. It'll make things a lot easier on ourselves.

I guess the question then remains, for both the host and the token: do we want to support the imperative useToken/useHost pattern in our API, or the functional getter/setter pattern in our API? Or both? I think now that username and password have been relegated to dependencies of the authorize method (rightly so, imo), then the only thing that we need to instantiate the client with is the host and the token. I don't see that list of dependencies growing or shrinking any time soon, so I'm inclined to say if it's only 2 dependencies, why not support both styles? There's also the matter of our function signature; I'm thinking maybe we could support both

const farm = farmOS('https://test.farmos.net', token);

and

const farm = farmOS({
  host: 'host://test.farmos.net',
  token,
  getHost() {/* function body */ },
  setHost() {/* function body */ },
  getToken() {/* function body */ },
  setToken() {/* function body */ },
});

Might be overkill, but wouldn't be too hard to implement, I think. Real question is then, what parameters are required, if any? Especially if we have useToken/useHost methods as well?

I'm curious though, could that chunk of code could live in a @utils/farmClient and export the client object? It could have methods to logout and remove the client, recreate the client if the host changes, and deny attempts to make requests if no client has been created? Would there be much more involved there?

By that "chunk of code", do you mean like the wrapper itself, with some additional logic for recreating the client, etc? That seems like a plausible solution, too, if we didn't want to whole-heartedly merge the 2 modules together. And I think it's compatible with the idea of making FK responsible for how it manages parallel requests. Might be worth discussing further in farmOS/farmOS-client#350.

Let's be careful to update the docs with some sort of warning, like

:warning: WARNING ABOUT PARALLEL REQUESTS :warning:

This library takes certain measures to ensure that a batch of requests, if executed in parallel with the same farm instance, will refresh its OAuth token only once, if needed, and will subsequently process each request using the new token. This cannot be guaranteed, however, for parallel requests made with separate farm instances; such requests may be subject to race conditions, which could cause many of them to fail because they didn't use the newest token. It is the responsibility of your application to manage requests so they either execute parallel requests using the same farm instance, or avoid such parallel requests altogether.

Ok, those are all my thoughts (for now :stuck_out_tongue:). Lemme know if you want to chat about all this in realtime, @paul121. Maybe some time Tues?

[paul121]

After chatting with @jgaehring we've decided to stick with just the getToken and setToken options. I've implemented this with default values that use an internal farm.token param:

function farmOS(host, opts) {
  const {
    clientId = 'farm',
    getToken = () => farm.token,
    setToken = token => farm.token = token,
  } = opts
...

This is working with FK quite elegantly.

@jgaehring are there any other considerations for farmOS.js? I think we've gotten most everything figured out? (Will need to update docs too.)

---

Let's be careful to update the docs with some sort of warning, like

:warning: WARNING ABOUT PARALLEL REQUESTS :warning:

Yes!! We'll need to add a special section to the docs discussing this.

[jgaehring]

I was doing some linting today and these lines caught my eye:

https://github.com/farmOS/farmOS.js/blob/98be3af411c468878647de565a80726ea439b24f/index.js#L5-L9

In particular, how we're storing the token on the farm.token property that the user can access. I can see why this would be useful, but I feel like if we're doing that we're creating the expectation that the token will always be accessible on that property. However, I don't believe that is the case, because we're not updating it there whenever setToken is called, if the user supplies their own setToken callback.

To my mind, we should commit to one of two paths:

1. Keep the token accessible at farm.token, and make sure it's always updated.
2. Remove the farm.token property and just use a closed variable.

I kinda lean towards 2 for now, because it's less to maintain, and I don't feel like documenting it. But what are your thoughts, @paul121? Do you do something similar in farmOS.py?

[jgaehring]

Hmm, I thought I'd tested this before, but I just tried to send a new log to the server and got back:

403 Access Denied: CSRF validation failed

Thought this might have been due to some of the linting I did, but I reset HEAD to 9b2642c and I'm still getting the error.

I'm going to keep investigating, and will report back if I find something, but @paul121, any clues why this could be happening?