Skip to content

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.


### 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

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.

2 participants