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

Add server signed response #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fwininger
Copy link
Collaborator

@kjg @adamlwgriffiths I try to do a PR for #112, can you take a look ?

If the approch seems to be good I implement the unit tests.

@fwininger
Copy link
Collaborator Author

Hi @kjg @mgomes can you take a quick look of this ?

@fwininger
Copy link
Collaborator Author

Hi @kjg @mgomes have you some time to take a quick look of this ? I need this feature in production before the end of the month and I prefers to not manage a hard fork of this gem.

Copy link
Collaborator

@kjg kjg left a comment

Choose a reason for hiding this comment

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

Comments on individual lines, can you please add some tests as well.

Thanks so much for taking this on!

if api_auth_options
response.headers['CONTENT-MD5'] ||= Digest::MD5.base64digest(json)
response.headers['Authorization'] ||= ApiAuth.sign!(
request,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'd want to pass in the response object here right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know that is strange, but we need to pass the request object because we need to access to the request.fullpath.
Secondly, we can use the already defined ActionDispatchRequest without any changed.

@@ -226,6 +226,30 @@ Rails app:
end
```

### Server signing response

The server can perform a validation of the response.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're trying to say the server can sign the content of the response, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -13,7 +13,40 @@ def api_authenticated?(secret_key)
end
end

module ClassMethods
def validation_with_api_auth(api_auth_options = nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we call this something like api_auth_sign_response

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


# API AUTH addition headers
if api_auth_options
response.headers['CONTENT-MD5'] ||= Digest::MD5.base64digest(json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed because the response object won't have a body for ApiAuth to inspect yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact, the body is the json variable. I keep the same code as the classic JSON renderer. It's just a .to_json of the variable passed in the controller.

The source code is here : https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/renderers.rb#L156

I add a comment in the code for memory.

Signed-off-by: Florian Wininger <fw.centrale@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants