Call before/after_successful_authorization callbacks on create as well as new

[mdeutsch]

Summary

The before/after_successful_authorization callbacks added in #1064 are only called on the new action. I think they should also be called on create to cover situations where the user must explicitly authorize the client.

Other Information

I also found that the callbacks can be run when authorization is not successful. I added some more tests and modified the existing code a bit.

This PR has some conflicts with my other PR, #1072. I'll resolve them but wanted to put this up for review.

cc @nattfodd

[nbulaj]

So the callback will be invoked twice? Looks strange from my point of view...

Moreover, maybe it will be better to move the hooks under the Authorization class instead of controller? It will DRY the code at least

[nattfodd]

So the callback will be invoked twice? Looks strange from my point of view...

+1. I would rather rename callback in create(or both?) into something different, so it would be 2 different callbacks.

Moreover, maybe it will be better to move the hooks under the Authorization class instead of controller? It will DRY the code at least

As I mentioned in previous PR, we need controller's scope in that callback (i.e. for interacting with cookies/sessions).

[nbulaj]

We can use existing callbacks, but add an extra argument - action ('new' or 'create'). Controller ais already passed with the context argument, so you already has an access to cookies or request.

[mdeutsch]

I think the new vs. create question depends on the purpose of these callbacks. Are they intended to be invoked when an authorization code or access token is returned? If so, that can happen in either the new or create action, but they'll only be invoked once during any given authorization flow:

• If the user is required to explicitly authorize the client, then:
  • new just presents a prompt to the user and doesn't invoke the callbacks
  • create generates the auth code/token and does invoke them
• If the skip_authorization callback returned true or the user previously granted permission, then:
  • new generates the auth code/token and invokes the callbacks
  • create is never hit

The duplicated code (before + authorize + after) could easily be extracted into a new method, perhaps into #authorize_response from my other PR.

I looked at moving the hooks into the request objects like the before/after_successful_response hooks in BaseRequest. That's possible -- we could pass the server into the request, which would provide the necessary controller context -- but it would change the signature of Code/TokenRequest#initialize and impact anyone using those classes. If you're ok with that (I see these PRs are targeting version 5.0), I can try making that change.

[mdeutsch]

I've rebased onto master and extracted some of the duplicated code. Now the callbacks are closer to the Authorization class and I think the intent is clearer: that the callbacks are invoked when the request is successfully authorized.

[nbulaj]

Let's rename it to authorizable to be closer to implementation and the essentials of this method. What do you think?

[mdeutsch]

Good call -- I'll change it.

[nbulaj]

Look better now. Don't forget to add a changelog entry to NEWS.md and squash the commits :) Thanks!

[mdeutsch]

OK, I renamed the variable, added a changelog entry, and squashed/rebased.

[nbulaj]

Got it, thank you for your work! 👍