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.

Sign up for GitHub

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

Jump to bottom

Greenfield: Rename API key redirect params; switch to POST body #1898

Merged
merged 17 commits into from Sep 17, 2020
Merged

Greenfield: Rename API key redirect params; switch to POST body #1898

merged 17 commits into from Sep 17, 2020

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented Sep 9, 2020 •
edited

This way it is clearer what to expect and it also make the parameter easier to consume.

Kukks
Kukks requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a post (which I completely forgot), we should actually send these props in the body

@dennisreimann
Copy link
Member Author

Boy, that escalated quickly

@NicolasDorier
Copy link
Member

tests do not pass

@Kukks Kukks added this to In Progress in 0.09 Miles-stone via automation Sep 14, 2020
@dennisreimann
Copy link
Member Author

dennisreimann commented Sep 14, 2020 •
edited

The integration test failures seem unrelated (rate providers), but I'm still unhappy about how the PostRedirect view turned out:

I couldn't find a way to apply attributes conditionally and Razor wouldn't let me use the form action in addition to the asp- attributes. Couldn't just if the opening tag either, so it ended up like this.

@dennisreimann
Rename user param to userId in API key redirect
dc5dc45 
This way it is clearer what to expect and it also make the parameteer easier to consume.
@dennisreimann
Post redirect: Allow form url and prettify page
3b97a50 
- Form URL as alternative to controller/action for external URLs
- Making it look nice and add explanation for non-JS case
@dennisreimann
APIKeys: Minor view updates
5350d8c 
fix
@dennisreimann
APIKeys: Use POST redirect for confirmation
00ff804 
fix
@dennisreimann
UI: Minor update to confirm view
bb8e1b1 
Tidies it up and adapts to the newly added ConfirmAPIKeys view.
@dennisreimann
APIKeys: Update delete view
1da7cc5 
Structures the information in title and description better.
@dennisreimann
APIKeys: Distinguish authorize and confirm (reuse)
164cffc
@dennisreimann
Upgrade ChromeDriver
e48f42c
@dennisreimann
Test fixes
2df6616
@dennisreimann
Clean up PostRedirect view
1387606 
By adding missing forgery token
@dennisreimann
Re-add tests for callback post values
237a56d
@dennisreimann
Rename key param to apiKey in API key redirect
0075c13
@dennisreimann
Copy link
Member Author

Now the PR can be reviewed. The main goal was to update two things regarding the callback the consumer app receives:

  • send parameters as POST body instead of as part of the URL query string
  • renaming the parameters to be more clear: user became userId, key became apiKey.

While working on this I also made some minor UI updates to the confirm view:

confirm

The separate confirm view got introduced to prepare the addition of a third button for the "create a new api key" button use case, which I'd like to tackle in a separate PR.

Also added a non-javascript version for the post redirect view:

nojs

Kukks
Kukks requested changes Sep 16, 2020
View reviewed changes
BTCPayServer.Client/BTCPayServer.Client.csproj Outdated Show resolved Hide resolved
BTCPayServer.Common/BTCPayServer.Common.csproj Outdated Show resolved Hide resolved
BTCPayServer.Data/BTCPayServer.Data.csproj Outdated Show resolved Hide resolved
BTCPayServer.Rating/BTCPayServer.Rating.csproj Outdated Show resolved Hide resolved
BTCPayServer/BTCPayServer.csproj Outdated Show resolved Hide resolved
BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json Outdated Show resolved Hide resolved
0.09 Miles-stone automation moved this from In Progress to Pending Review Sep 16, 2020
@dennisreimann @Kukks
Update BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization…
99aaa6f 
….json

Co-authored-by: Andrew Camilleri <evilkukka@gmail.com>
@dennisreimann dennisreimann changed the title Rename user param to userId in API key redirect Greenfield: Rename API key redirect params; switch to POST body Sep 16, 2020
0.09 Miles-stone automation moved this from Pending Review to Review complete Sep 17, 2020
@Kukks Kukks merged commit 7e60328 into btcpayserver:master Sep 17, 2020
0.09 Miles-stone automation moved this from Review complete to Done Sep 17, 2020
civa added a commit to MyCryptoPay/cryptopayserver that referenced this pull request Sep 17, 2020
@civa
Merge pull request #1 from btcpayserver/master
6e784b9 
Greenfield: Rename API key redirect params; switch to POST body (btcpayserver#1898)
@dennisreimann dennisreimann deleted the user-param-rename branch September 17, 2020 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kukks Kukks Kukks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
0.09 Miles-stone
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dennisreimann @NicolasDorier @Kukks