Add support for API clients

[krschacht]

A request will be authenticated if there is a proper Bearer token in the request and JSON should be returned for all endpoints.

Automatically returning a response for actions that end with redirect_to was a little tricky. The way this was implemented was to assign a special significance to redirect_to with a status: :see_other. When this status is present, we take this as a sign of success and we render a simple success JSON response.

[krschacht]

@matthewbennink Give this one another look. I think I was actually able to solve those good points that you raised (ivars and redirect). Is this a good overall solution now?

[matthewbennink]

Yea, I think this makes sense overall. We can request information and generally get everything that's needed for the HTML view, and if we mutate anything, we'll be given a redirect URL that will give us the information we may need, or another redirect perhaps.

Thinking through creating messages, if we send a POST to /assistants/:assistant_id/messages, we'll get a redirect to /conversations/:conversation_id/messages?version=1 and that will provide the full list of messages. Our message is likely the most recent "user" message or perhaps it's the streaming_message, but maybe that doesn't matter so much since we'll have the full list and can reconcile as needed with some local state.

I did a quick search for where instance variables are set and, apart from the User, I don't see any that seem particularly dangerous if they're leaked. The assumption is that if the user has access to the action, they have access to any data within instance variables we set.

For User, it's providing the OpenAI and Anthropic keys even if they're set via the ENV vars, so that might be something to think through. The @user is only set in a couple places, once in users#update where we redirect afterwards, so it won't be leaked with the current logic, and the other in authentications#create where it doesn't seem necessary to use an instance variable at all; I think we could get away with just user since it's only referenced within the controller method, not the new view template.

More importantly, the @user variable is set even though the user isn't authenticated yet, so it may be possible to send a request to /login and get a response back for the person with a given email even if the wrong password is supplied. It doesn't work right now because we render the new template, so default_render isn't called and instead we get this error:

base-1      | web      | ActionView::MissingTemplate (Missing template authentications/new, application/new with {:locale=>[:en], :formats=>[:json], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby]}.

---

TL;DR - If we just remove the @user instance variable from authentications#create, I think this will be safe. We could also adjust the api_request? method to only return true if the client is authenticated, e.g., request.format.json? && Current.client.authenticated?.

[matthewbennink]

Oh - the only other thing I forgot to note that we'll need to deal with is skipping verify_authenticity_token when it's an API request. I haven't figured that one out yet. It may just be another case of overriding a method and calling super for non-API requests, but it could happen in a separate PR.

[krschacht]

If we just remove the @user instance variable from authentications#create, I think this will be safe.

I'm loving the thoroughness. Great catch on @user. I just removed those two unnecessary setting of them.

However, it really seems wrong that we should need to be concerned with a user seeing the @user object. The fact that we're even having to ask ourselves this suggests that the openai_key implementation is wrong. One some level I already realized this b/c I didn't want to "leak" these keys via a user's account settings, and now looking back I think my suggested fix was wrong. We fixed it at the form level but really we should have fixed it at the model level.

I think the right fix is: we no longer hack the openai_key attribute. Instead, on the user model we add a method on the model like:

def preferred_openai_key
  openai_key || ENV...
end

We could also adjust the api_request? method to only return true if the client is authenticated, e.g., request.format.json? && Current.client.authenticated?.

I don't quite get this because every controller action already has a before_filter concerning authentication. There's a default app-wide in application_controller and some controllers override that for various actions.

If we also add an authentication check in the redirect_to, which is at the bottom, then we could end up with a discrepancy between the auth check at the start of an action and the end. So that doesn't seem right.

@matthewbennink Is there a situation I'm not thinking of where we need redirect_to to check it again?

authenticity_token

I think it's just skip_before_action :verify_authenticity_token, if: :api_request? I added that to the application_controller. I'm not quite sure how to test it but I'm pretty sure it's that simple

[matthewbennink]

I don't quite get this because every controller action already has a before_filter concerning authentication.

I think I had the unauthenticated routes in mind at the time, such as signing in. But we're no longer setting instance variables for any of those now that @user has been changed, so I think we're set. If we ever do set an instance variable prior to authenticating, it could potentially be accessed, but it feels unlikely and also wouldn't be associated with any user.

I'm pretty sure it's that simple

Yep