Appkeys feedback

Development
1

Support for applications requesting an API key from OctoPrint has recently landed to the Maintenance branch, which is awesome; no more finding and copy/pasting a long string of characters with the chance of missing one and things not working.

I have implemented an initial version in the Cura OctoPrintPlugin, and this is what it looks like:

[Album] Imgur

I have two suggestions, from the perspective of the "client" (Cura in this case).

As far as I have been able to tell, there is no official way to query OctoPrint if requesting an AppKey is supported by an OctoPrint instance. The version may be too old, or the plugin may have been disabled. It would be nice to be able to query this, so the client can only show the button to request access to an OctoPrint instance directly if it supports this. I could just start the request and see the response, but that already opens up the dialog Access Request dialog in any running OctoPrint interfaces, so I would prefer not to start the request unless the user actively presses a button.

I have found a hacky way, which is to do a GET request to /plugin/appkeys/request (without an app_token). This results in a 405 Method Not Allowed http error if the plugin is available (because it expects a POST request), which does not initiate a dialog. It would be nicer if I could GET something (/plugin/appkeys or /plugin/appkeys/request) that returns a 204 No Content or similar.

The second issue is that it can easily happen that many Access Requested dialogs are spawned that are actually orphans. If there are multiple octoprint pages open (in different tabs or different browsers), they can all get the dialog but if one of those them makes the "Allow" decision, all of the other dialogs become irrelevant (but stick around). Also if the originating application is closed down before the decision is made, the dialog also stays open even though the decision has become irrelevant.

My original implementation already had a way to close orphan dialogs, would you like me to create a PR to backport this mechanism, or was there a specific reason you did not want to handle orphan dialogs the way I did it?

PS: if you would prefer this on github, then I'll move the discussion there. But I figured it was more a discussion than a bug or feature request, and everybody likes to see a cool video, right?

3 Likes
2

I'll add something along the lines of /plugin/appkeys/supported, does that sound like a plan?

That would be great. I want to put out a first RC of 1.3.10 ASAP (in fact I had planned on today, but something came up and kept me from it) though, so I'm not 100% sure we'll still get that improvement in there. Now that you mentioned it I also remembered it, that was the timer that regularly checked with the backend, right? If you don't find the time I can also look into it. Truth be told, I simply forgot about this scenario, no reason to not want to tackle it.

1 Like
3

I'll add something along the lines of /plugin/appkeys/supported , does that sound like a plan?

:+1:

Now that you mentioned it I also remembered it, that was the timer that regularly checked with the backend, right?

I had to look it up, but it used the delay option of PNotify to hide the dialogs and would keep resetting the delay timer on each poll. That means traffic from the backend to the frontend while polling. It could be better to handle the polling timeout in the backend instead.

I'll see what I can do today.

Update: WIP PR started https://github.com/foosel/OctoPrint/pull/2869
Update: PR done

1 Like
4

And merged, thank you!

1 Like
5

One more thing I noticed while integrating appkey requests in Cura is that there is a small discrepancy between the documentation and the implementation.

In the workflow, step 3, it says that the server returns a 201 Created response on the initial post to appkeys/request, but it actually sends a 202 Accepted: line 133.

I think either code is an acceptable choice. Changing the implementation is less work than changing the documentation :wink:

6

Oh, thanks for the heads-up. I did adjust the code, an Created appears to make more sense for me here as not only did something get created (new endpoint), we also get it back in the Location header.