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

Allow API responses in PasswordExpiredController #111

Merged
merged 8 commits into from Apr 26, 2020

Conversation

Slike9
Copy link
Contributor

@Slike9 Slike9 commented Jul 3, 2019

For API sometimes it's desirable to receive 2XX responses instead of
3XX. The same technique is used in devise gem.

More about the problem addressed: #111 (comment)

@coveralls
Copy link
Collaborator

coveralls commented Jul 3, 2019

Pull Request Test Coverage Report for Build 726

  • 33 of 33 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 96.047%

Totals Coverage Status
Change from base Build 723: 0.2%
Covered Lines: 1239
Relevant Lines: 1290

💛 - Coveralls

@olbrich
Copy link
Contributor

olbrich commented Jul 3, 2019

@Slike9 Thanks for your contribution, please add a link to the relevant part of the devise codebase, add some tests, and add some comments describing why this is useful. We also need to consider how this might break code that depends on the existing behavior.

@Slike9
Copy link
Contributor Author

Slike9 commented Jul 3, 2019

@olbrich Could you please consider my answer?

please add a link to the relevant part of the devise codebase

Similar code respond_with({}, location: ...) is used in several places in devise codebase:

add some comments describing why this is useful

Method respond_with from responders gem is applied to be able to respond to different requested formats (html, json, etc.).
For formats associated with browsing, like :html, :iphone, etc, the fix won't change the behavior, the action will respond with a redirect as before.
The behavior will change for formats associated with APIs, such as :xml and :json, the action will respond with 204 No Content instead of redirect.

The problem we are facing is the following. In our SPA-like application, we access the controller's actions via AJAX, requesting json format. To update password, the client side requests PUT /users/password_expired.json via AJAX. The browser gets 302 Location: / and follows the redirect to / using PUT method (it preserves the verb) and gets 404. In the client side nothing happens, as it doesn't see 302 response, just 404. More about this problem:

So, we need API friendly responses to AJAX requests, not 302.

add some tests

Do we need to test behavior of respond_with of responders gem? The behavior for html format is already tested, isn't it enough?

@olbrich olbrich self-assigned this Jul 6, 2019
@olbrich olbrich added #️⃣ Major incompatible API changes 🐛 bug 🚧 WIP Work in progress labels Jul 6, 2019
@olbrich olbrich removed the 🚧 WIP Work in progress label Jul 7, 2019
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
@@ -8,27 +8,39 @@ end

appraise 'rails-5.0-stable' do
gem 'rails', '~> 5.0.0'
group :test do
gem 'rails-controller-testing'
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this gem added for? This is an outdated method of testing and generally meant to support legacy apps, not for new code.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have some tests that need to run against controllers in Rails 4.2. This lets them work against rails 5+ as well.

@olbrich olbrich added this to the 0.15.0 milestone Jul 21, 2019
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
test/controllers/test_password_expired_controller.rb Outdated Show resolved Hide resolved
@billgloff
Copy link

Would love to see this get merged. Let me know if you need any help.

@billgloff
Copy link

In lib/devise-security/controllers/helpers.rb there are two checks (handle_password_change & handle_paranoid_verification) for checking if the format is html, can we remove that and handle both cases (html and json) and just return the proper error message if its json format so the frontend can handle it properly?

@olbrich olbrich added #️⃣.#️⃣ Minor added functionality in a backwards-compatible manner and removed #️⃣ Major incompatible API changes labels Aug 18, 2019
@olbrich
Copy link
Contributor

olbrich commented Sep 21, 2019

@billgloff can you elaborate a bit on how you would like the helpers to respond? Some failing tests would be extremely helpful. As an alternative, you could open a separate ticket / PR for your modifications and then I'll go ahead and merge this branch.

@dillonwelch
Copy link
Contributor

@olbrich how do you feel about merging this and then we can handle the request in a separate PR as desired?

@olbrich olbrich merged commit ec95a71 into devise-security:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug #️⃣.#️⃣ Minor added functionality in a backwards-compatible manner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants