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

FEATURE: Make auth_redirect param options on user_api_keys #6840

Merged
merged 3 commits into from Jan 4, 2019

Conversation

4 participants
@cfitz
Copy link
Contributor

commented Jan 1, 2019

This is a possible solution for https://meta.discourse.org/t/user-api-keys-specification/48536/19
This allows for user-api-key requests to not require a redirect url.
Instead, the encypted payload will just be displayed after creation ( which can be copied
pasted into an env for a CLI, for example )

FEATURE: Make auth_redirect param options on user_api_keys
This is a possible solution for https://meta.discourse.org/t/user-api-keys-specification/48536/19
This allows for user-api-key requests to not require a redirect url.
Instead, the encypted payload will just be displayed after creation  ( which can be copied
pasted into an env for a CLI, for example  )
@discoursebot

This comment has been minimized.

Copy link

commented Jan 1, 2019

You've signed the CLA, cfitz. Thank you! This pull request is ready for review.

@discoursebot

This comment has been minimized.

Copy link

commented Jan 1, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/adding-command-line-tools-support-for-user-api-keys/103803/22

@vsoch

This comment has been minimized.

Copy link

commented Jan 1, 2019

Huge +1 to this addition! The first working example (also to be merged soon!) is via the helpme client 👉 vsoch/helpme#34 when you write:

helpme discourse https://stage.neurostars.org

(or some other board) it will walk you through posting an issue, meaning a title, text, optional asciinema and environment, and then post without leaving the command line! This PR here allows for the generation of the user token on the first try (when the user doesn't have one yet).

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Just rendering the payload seems somewhat surprising, I would expect some sort of instructions on the final page, eg:

We just generated a new user API key for you, please paste the following key into your application:

abc12312ab12312312321

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

The generation of the token happens by way of the client, and the user needs to copy paste the payload into the terminal. This PR redirects the user to the page with the payload, and the token generation needs to happen by the client so we can use their credentials files. The instructions are also clear when the interaction is happening. It also only needs to happen once, so it's not too much of a hassle.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

I follow, but a real page here is very beneficial. It is then clear you are on site cause the header is still there, and there are a more instructions to go with cause we don't simply render a white page with a code.

It is a very simple page rendering this with some canned message and the no-ember layout.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

The whole interaction is here https://github.com/vsoch/helpme/pull/34/files#diff-eb72c9906e2158e128328e39f96df25aR40 if you'd like to see it. The user is going through using the tool, and asked to open the URL and copy paste the result in the terminal. I think the web UI is simple because it's perhaps up to the client to decide how to interact with it? I'd, if anything, suggest that the page remains just the code in case a client wants to otherwise interact (beyond copy paste) so they don't have to parse out the string.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Oh but you are suggesting an actual page with the token (with headers, etc.) @cfitz what do you think?

@cfitz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

Adding a view makes sense to me. But also, if the request is expecting a JSON response, should we return JSON? I added both, but let me know if I should just return the html version.

Also I added a new i18n entry to the server.en.yml. Is there a process I should follow to add these to the other translations?

Lastly, should I be squashing my commits?

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

If the user needs to physically click to approve, how can we expect Json? Would they approve, leave the window open, then press enter (and then the client does a GET to the page with json?) Is that a better interaction than copy paste?

@cfitz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

I think the login and the approval will still need to be done via the browser. The only change is if a user post's to user-api-key.json they'll get a JSON response with the payload and the instructions message instead of the html page ( after the approval bit has been done ).
Biut, yeah, that might be unnecessary.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Is this change live with stage.neurostars.org? If so I'd like to run it through testing again to see the new interaction (and try out the json).

FEATURE: Show instructions when creating user-api-key w/out redirect
This adds a view to show instructions when requesting a user-api-key
without a redirect. It adds a erb template and json format.
Also adds a i18n user_api_key.instructions for server.en.yml

@cfitz cfitz force-pushed the cfitz:user-api-keys branch from 48b4099 to 35a4609 Jan 2, 2019

@cfitz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

@vsoch just updated it. try it out and see if that works for you.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

one minor comment and we are good to merge 🎊

Replace && return with if / else block
Replace the && return with an easier to read if / else block.
@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Testing now.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

hey @cfitz the page is totally blank
image

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Here is the actual page, looks like some kind of blank 404?
notfound.html.txt

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Does the flow need to change at all?

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

I'll try the .json version and see if that shows anything.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

It's not clear to me if I'm supposed to do user-api-key.json/new, or user-api-key/new.json - both trigger an error. Here is doing:

    url = (board + "/user-api-key.json/new?scopes=write&application_name=HelpMe&public_key=" + 
           self.public_key.replace("'", "") + 
           "&client_id=" + self.client_id +
           "&nonce=" + nonce )

image

And then doing the other way:

    url = (board + "/user-api-key/new.json?scopes=write&application_name=HelpMe&public_key=" + 
           self.public_key.replace("'", "") + 
           "&client_id=" + self.client_id +
           "&nonce=" + nonce )

image

And if I leave as it was before (not asking for json) I get an empty page. Let me know what to try!

@cfitz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

@vsoch Sorry, that's my bad! I realized the last git pull on the stage server actually errored because the git history diverged. I just reset it and pulled. I think it should be working now.

Sorry, I think I confused things about the JSON handler...the test suite POSTs directly to the user_api_keys controller, so I just added something in the controller. The /user-api-key/new ( without the .json ) action is the form that the user needs to approve.

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

okay, I will try again (with the original flow)

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Oh it's lovely! So much better!

image

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

Still testing though :) Hang tight!

@vsoch

This comment has been minimized.

Copy link

commented Jan 2, 2019

okay it looks good! Here is the post that I just made:

https://stage.neurostars.org/t/why-are-jelly-beans-so-funky-looking/29

And it has it's own asciinema, but I also recorded a second one to see the client interaction (so @SamSaffron can see too!)

https://asciinema.org/a/219484?speed=2

@vsoch

This comment has been minimized.

Copy link

commented Jan 3, 2019

@SamSaffron did you have any other issues we need to work on?

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Looks good to me! thanks heaps!

@SamSaffron SamSaffron merged commit 19d7545 into discourse:master Jan 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.