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

Added :after_successful_authorization callback #1064

Merged

Conversation

nattfodd
Copy link
Contributor

Summary

This PR adds an ability to run some code after successful authorization. TLDR: #1062

Example:

Doorkeeper.configure do
  after_successful_authorization do |controller|
    controller.session[:logout_urls] << 
      Doorkeeper::Application
      .find_by(controller.request.params.slice(:redirect_uri))
      .logout_uri
  end
end

it 'should call :after_successful_authorization callback' do
expect(Doorkeeper.configuration)
.to receive_message_chain(:after_successful_authorization, :call)
.with(instance_of(described_class))

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.


it 'should call :after_successful_authorization callback' do
expect(Doorkeeper.configuration)
.to receive_message_chain(:after_successful_authorization, :call)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

after do
allow(Doorkeeper.configuration)
.to receive(:skip_authorization)
.and_return(proc { true })

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

describe 'GET #new with callbacks' do
after do
allow(Doorkeeper.configuration)
.to receive(:skip_authorization)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

it 'should call :after_successful_authorization callback' do
expect(Doorkeeper.configuration)
.to receive_message_chain(:after_successful_authorization, :call)
.with(instance_of(described_class))

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.


it 'should call :after_successful_authorization callback' do
expect(Doorkeeper.configuration)
.to receive_message_chain(:after_successful_authorization, :call)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

after do
allow(Doorkeeper.configuration)
.to receive(:skip_authorization)
.and_return(proc { true })

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

describe 'GET #new with callbacks' do
after do
allow(Doorkeeper.configuration)
.to receive(:skip_authorization)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

@nattfodd nattfodd force-pushed the after_successful_authorization branch 3 times, most recently from a16de71 to 1885305 Compare March 26, 2018 14:09
@@ -217,6 +217,7 @@ def option(name, options = {})
::Rails.logger.warn(I18n.t('doorkeeper.errors.messages.credential_flow_not_configured'))
nil
end)
option :after_successful_authorization, default: ->(_controller) {}

Choose a reason for hiding this comment

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

Unnecessary spacing detected.

@@ -24,6 +24,7 @@ def destroy
def render_success
Copy link
Member

Choose a reason for hiding this comment

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

Can we add before hook? I think it can be useful for some purposes, moreover it will comply with other Doorkeeper hooks

@@ -217,6 +217,7 @@ def option(name, options = {})
::Rails.logger.warn(I18n.t('doorkeeper.errors.messages.credential_flow_not_configured'))
nil
end)
option :after_successful_authorization, default: ->(_controller) {}
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename _controller to _context

@nbulaj
Copy link
Member

nbulaj commented Mar 27, 2018

Just small fixes for comments above and it would be great 👍

Also after finishing the work - could you please add a changelog entry to NEWS.md and squash all commits to a single one? Thanks

@nattfodd nattfodd force-pushed the after_successful_authorization branch from 3f36e1f to 42126a7 Compare March 27, 2018 07:49
They expose AuthorizationController context in the :new method
Use cases: for example, when you need to set some cookies after successful authorization
@nattfodd nattfodd force-pushed the after_successful_authorization branch from 42126a7 to 842f450 Compare March 27, 2018 07:50
@nattfodd
Copy link
Contributor Author

@nbulaj it's all done, thank you for your points :)

@nbulaj nbulaj merged commit 37f4e51 into doorkeeper-gem:master Mar 28, 2018
@nbulaj
Copy link
Member

nbulaj commented Mar 28, 2018

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants