Skip to content
This repository has been archived by the owner on Sep 12, 2021. It is now read-only.

Change header in default Fingerprint generator #271

Closed
wants to merge 2 commits into from

Conversation

centaure
Copy link

In default FingerprintGenerator, replace ACCEPT header with ACCEPT-LANGUAGE header (when generating Fingerprint):

Using the ACCEPT header field for the default Fingerprint generator will cause problems for applications that use different media types depending on requests. For example, you may have a controller that processes user sign-in POST request RESTfully (accepting "application/json, text/javascript, /; q=0.01"); therefore the authenticator and its corresponding fingerprint is created using this JSON etc ACCEPT header. On the other hand, the post-sign-in process may include a Redirect action whose GET request uses the usual HTTP request type i.e. "text/html,application/xhtml+xml,application/xml;q=0.9". In this case, silhouette will fail during authenticator retrieval when processing the GET request (because the fingerprints are different).

Although the user can implement his/her own generator in this case, it will be good to avoid the "ACCEPT" header for the default generator (so as to remove a potential complication for new users who, for example, use such a mixture of REST and HTML in their API).

ACCEPT-LANGUAGE, on the other hand, will be least likely to be different across request types.

Follow up on change to DefaultFingerprintGenerator:
- include testing for ACCEPT-LANGUAGE when using DefaultFingerprintGenerator.
In default FingerprintGenerator, replace ACCEPT header with ACCEPT-LANGUAGE header (when generating Fingerprint):
Using the ACCEPT header field for the default Fingerprint generator will cause problems for applications that use different media types depending on requests. For example, you may have a controller that accepts "application/json, text/javascript, /; q=0.01" for RESTful requests, and "text/html,application/xhtml+xml,application/xml;q=0.9" for HTML requests. Although the user can implement his/her own generator in this case, it will be good to avoid the "ACCEPT" header for the default generator (so as to remove a potential complication for new users who, for example, use such a mixture of REST and HTML in their API). ACCEPT-LANGUAGE, on the other hand, will be least likely to be different across request types.
@centaure centaure closed this Jan 26, 2015
@centaure centaure reopened this Jan 26, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+1.63%) to 100.0% when pulling d7b3ddb on centaure:patch-1 into bd437b2 on mohiva:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 98.42% when pulling d7b3ddb on centaure:patch-1 into bd437b2 on mohiva:master.

@akkie
Copy link
Contributor

akkie commented Jan 26, 2015

The problem here is that the Accept header for a JSON request is defined by the JS library and not by the browser. So if you call a JSON endpoint from JS then you send another header as in a GET request initiated from your browser.

Let me think a bit about a solution. I'm not sure if we should change the default fingerprint generator. Maybe a better solution would be to create a new one for such a content negotiation scenario. Anyway, the Accept-Language header wouldn't be enough. The Accept-Charset and Accept-Encoding headers are also good candidates to generate a fingerprint of the client.

@centaure
Copy link
Author

I agree that "Accept-Charset and Accept-Encoding headers are also good candidates".

FYI: I am relatively new to Silhouette, and I migrated to Silhouette v2.0-SNAPSHOT (from v1.0) in the last few days (using your silhouette-seed example as a guide). The above fingerprint issue was the only major issue I faced. Initially I had no clue what was going on (as I had not defined "com.mohiva.play.silhouette.impl.authenticators.CookieAuthenticatorService" in my logger.xml file; CookieAuthenticatorService does print a logger info message when the fingerprints are different). But I figured it out after looking through the source code.

@akkie
Copy link
Contributor

akkie commented Jan 28, 2015

I close this in favor of #276.

Thanks, for reporting the issue.

@akkie akkie closed this Jan 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants