Add #{before,after}_successful_authorization hooks in Doorkeeper::TokensController

[fotos]

Summary

Added #{before,after}_successful_authorization hooks in Doorkeeper::TokensController#create, similar to #1064, as amended in #1075).

Other Information

For the #before_successful_authorization I am not sure how to (pre) authorize like Doorkeeper::AuthorizationsController#pre_auth.

The pre_auth.authorizable? seems to contradict the hook name. If the #pre_auth is #authorizable? does it mean that the strategy is going to be successful? 🤔

In any case I decided to keep the names the same. I would appreciate some feedback or a pointer towards the right direction.

Additionally I added specs for Doorkeeper::TokensController#create. For some reason they have been disabled in 7f54ac2.

[nbulaj]

Actually I'm thinking about how valid is this hook here. Initially before_successful_authorization hook was added for Authorization flow, and tokens controller used for other flows..

[fotos]

I guess I need to explain the use case: similar to Doorkeeper::AuthorizationsController we need to be able to monitor and emit token creation events.

The password flow (resource_owner_from_credentials) goes straight into the Doorkeeper::TokensController. The existing strategy hooks ({before,after}_successful_strategy_response) do not yield the controller or the params.

Would adding different hooks (e.g. {before,after}_successful_token) be a better approach?

[nbulaj]

Hi @fotos . Need to think a little bit more about it. Seems like any other grant flow also must have such hooks and it's also an authorization process, but something confuses me ... Maybe we can find an author of the original PR introduced this hooks and asked him why this hooks works as they works?

I don't remember, maybe it was @f3ndot (sorry if I'm wrong, I remember you refactored or introduced this configuration options).

[fotos]

👋 @nbulaj. No worries, although I'd love to get this merged there is no rush.

I linked to the PRs that introduced the feature in the description (summary). The authors that introduced the callbacks were @nattfodd and @mdeutsch (both PRs have been merged by you 🙌).

Seems like any other grant flow also must have such hooks and it's also an authorization process, but something confuses me […]

I lost you here. You mean other flows should have such hooks and they are currently missing them or other flows have such hooks already (except the password flow in TokensController)?

In any case let me know if I can help, in any way, to get this landed! 🛬

[nbulaj]

. You mean other flows should have such hooks and they are currently missing them

this one 😃

[nbulaj]

LGTM 🚝

I thought about how valid is to reuse this callbacks for tokens controller (that controls password / client credentials / etc flows) and seems like it's OK for me. We are doing an authorization for any grant flow, so we must support all of them

[fotos]

Thanks @nbulaj!

Is there anything else that needs to be changed or shall I squash the commits and then you can merge? 🚀

[nbulaj]

Yep, squash them and I think it's ready for merging. Sorry, I'm very busy right now, can't find much time for open-source

[fotos]

@nbulaj commits squashed. 🗜

No worries, I'm used to waiting weeks or sometimes even months. 😁

A single day turnaround is pretty cool in my book. 😎

[nbulaj]

Thanks, @fotos , for your work! ❤️