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

Dialog to allow user to log in to third parties #219

Merged
merged 41 commits into from
Jul 18, 2014

Conversation

cooperq
Copy link
Contributor

@cooperq cooperq commented Jul 7, 2014

Creates the ability to have a dialog pop up when a user wants to log in to a third party to interact: e.g. disqus, youtube, instapaper, etc.

cooperq and others added 22 commits May 9, 2014 18:43
Conflicts:
	src/background.js
	src/popup.js
	src/webrequest.js
Conflicts:
	src/background.js
	src/webrequest.js
@cooperq
Copy link
Contributor Author

cooperq commented Jul 7, 2014

This would fix #137

Conflicts:
	doc/sample_cookieblocklist.txt

var exports = {};

var DomainExceptions = {

Choose a reason for hiding this comment

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

Do you plan to eventually have this as an EFF-hosted list (similar to cookieblocklist.txt) so that this can be updated without an extension release?

It would be nice to modify the syntax of cookieblocklist.txt so this list could be incorporated into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Jul 08, 2014 at 09:08:21AM -0700, Yan Zhu wrote:

Do you plan to eventually have this as an EFF-hosted list (similar to cookieblocklist.txt) so that this can be updated without an extension release?

It would be nice to modify the syntax of cookieblocklist.txt so this list could be incorporated into it.

Cookieblocklist.txt is in a format defined by ABP, so I don't think we
should modify it unless we find a way to stay ABP-compatible when doing
so.

But +1 on hosting it somewhere.

Peter Eckersley pde@eff.org
Technology Projects Director Tel +1 415 436 9333 x131
Electronic Frontier Foundation Fax +1 415 436 9993

Choose a reason for hiding this comment

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

@pde What are the consequences of breaking ABP compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diracdeltas the consequence of breaking ABP compatibility is that the chrome version is still dependent on ABP code for it's blocking engine. I have a long term plan to remove this dependency but for now it's still there. We could break ABP compatibility and write some glue to turn the list into something that is abp compatible, but that seems unnecessary IMO.

I agree that it should be hosted somewhere instead of included inline. For now I think it should be hosted as a separate file. I will work on that.

Conflicts:
	doc/sample_cookieblocklist.txt
@cooperq cooperq self-assigned this Jul 8, 2014
@diracdeltas
Copy link

This is coming along in Firefox. I realized today that the page-mod API either accepts wildcard domains or full URLs for matching, so annoyingly, the list needs to have separate entries for "http://etc" and "https://etc" (or I'll parse it into this form in Firefox after fetching it).

@cooperq
Copy link
Contributor Author

cooperq commented Jul 15, 2014

@diracdeltas I decided to make the format of the domain exception list like so:

"<login url>": {"whitelist_urls": [<url 1>, <url 2>], "english_name": "<English Name>"}

for example:
"disqus.com/next/login": {"whitelist_urls": ["disqus.com"], "english_name":"Disqus"}

I think this provides the most clarity.

@cooperq
Copy link
Contributor Author

cooperq commented Jul 15, 2014

@pde I have also removed protocol info from the domain exception list. At this point I think we are good to go.

var K_ENTER = 13;
var K_TAB = 9;

document.addEventListener('keydown',function(e){
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to remove this eventListener after we're done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we do, fixed.

@pde
Copy link
Contributor

pde commented Jul 16, 2014

I can't get this to fire anymore... perhaps we've broken it with the last few changes? What are you seeing, Cooper?

@pde
Copy link
Contributor

pde commented Jul 16, 2014

OK, this is looking good enough for beta. Merging.

@cooperq
Copy link
Contributor Author

cooperq commented Jul 16, 2014

Did you get it to fire?

@pde
Copy link
Contributor

pde commented Jul 16, 2014

Yes, after removing/reinstalling Privacy Badger... so maybe there is
actually a lurking bug ...

On Wed, Jul 16, 2014 at 04:01:33PM -0700, Cooper Quintin wrote:

Did you get it to fire?


Reply to this email directly or view it on GitHub:
#219 (comment)

Peter Eckersley pde@eff.org
Technology Projects Director Tel +1 415 436 9333 x131
Electronic Frontier Foundation Fax +1 415 436 9993

@pde
Copy link
Contributor

pde commented Jul 17, 2014

I can actually reproduce the problem now.

Step 1: comment on Youtube, whitelist to allow the login

Step 2: (may not be required) uninstall Privacy Badger, reinstall

Step 3: logout from Youtube, try to comment on Youtube again (Youtube will ask you to login)

Step 4: after login, clicking in the compose comment box causes an apparent page rerender/reload event, and does not give you focus.

Each time the page reloads the console logs from within the YouTube DOM are:

clobbering cookie getter VM6019:1
clobbering cookies for 
Location {replace: function, assign: function, ancestorOrigins: DOMStringList, origin: "https://plus.google.com", hash: "#id=I0_1405556862746&parent=https%3A%2F%2Fapis.goo….com&pfname=%2FI0_1405556787752&rpctoken=10363102"…}
 clobbercookie.js:19
clobbering cookies for 
Location {replace: function, assign: function, ancestorOrigins: DOMStringList, origin: "https://apis.google.com", hash: "#_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_r…3A%2F%2Fwww.youtube.com&pfname=&rpctoken=34709718"…}
 clobbercookie.js:19

The console logs from within Privacy Badger are:

blacklisting origin plus.google.com heuristicblocking.js:655
forgetting tab 106 webrequest.js:207
blacklisting origin apis.google.com heuristicblocking.js:655
tab removed! 106 webrequest.js:72
forgetting tab 106 webrequest.js:207
Unchecked runtime.lastError while running browserAction.setBadgeBackgroundColor: No tab with id: 106.
    at updateCount (chrome-extension://fgggbcohhecmabjgjnfmpdfaadapjckp/src/background.js:724:26) extensions::sendRequest:86
Unchecked runtime.lastError while running browserAction.setBadgeText: No tab with id: 106.
    at updateCount (chrome-extension://fgggbcohhecmabjgjnfmpdfaadapjckp/src/background.js:729:24) extensions::sendRequest:86
Unchecked runtime.lastError while running browserAction.setBadgeBackgroundColor: No tab with id: 106.
    at updateCount (chrome-extension://fgggbcohhecmabjgjnfmpdfaadapjckp/src/background.js:724:26) extensions::sendRequest:86
Unchecked runtime.lastError while running browserAction.setBadgeText: No tab with id: 106.
    at updateCount (chrome-extension://fgggbcohhecmabjgjnfmpdfaadapjckp/src/background.js:729:24) extensions::sendRequest:86
4
blacklisting origin api

@cooperq
Copy link
Contributor Author

cooperq commented Jul 17, 2014

I can't reproduce this problem even following the steps you outlined. However I do know what is causing that console error. So I will fix that and see if it does anything.

@cooperq
Copy link
Contributor Author

cooperq commented Jul 18, 2014

These commits should fix the console error that @pde was seeing. I'm still unable to reproduce the problem. Going to merge since it is time to release.

cooperq added a commit that referenced this pull request Jul 18, 2014
Dialog to allow user to log in to third parties
@cooperq cooperq merged commit d8eb923 into master Jul 18, 2014
saveAction('noaction', whitelistDomains[i], getHostForTab(tabId));
reloadTab(tabId);
}
if(msg.action === "not_now"){

Choose a reason for hiding this comment

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

shouldn't this be "never" instead of "not_now"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should. There should be an x in the corner of the popup which
amounts to "not now". Pressing escape, or clicking outside the popup,
should have the "not now" effect too...

On Wed, Jul 23, 2014 at 04:38:32PM -0700, Yan Zhu wrote:

+function _askUserToWhitelist(tabId, whitelistDomains, englishName){

  • console.log('assking user to whitelist');
  • var port = chrome.tabs.connect(tabId);
  • port.postMessage({action: 'attemptWhitelist', whitelistDomain:englishName, currentDomain:getHostForTab(tabId)});
  • port.onMessage.addListener(function(msg){
  • for(var i = 0; i < whitelistDomains.length; i++){
  •  if(msg.action === "allow_all"){
    
  •    saveAction('noaction', whitelistDomains[i]);
    
  •    reloadTab(tabId);
    
  •  } 
    
  •  if(msg.action === "allow_once"){
    
  •    //allow blag on this site only
    
  •    saveAction('noaction', whitelistDomains[i], getHostForTab(tabId));
    
  •    reloadTab(tabId);
    
  •  }
    
  •  if(msg.action === "not_now"){
    

shouldn't this be "never" instead of "not_now"?


Reply to this email directly or view it on GitHub:
https://github.com/EFForg/privacybadgerchrome/pull/219/files#r15322886

Peter Eckersley pde@eff.org
Technology Projects Director Tel +1 415 436 9333 x131
Electronic Frontier Foundation Fax +1 415 436 9993

@ghostwords ghostwords added broken site heuristic Badger's core learning-what-to-block functionality ui User interface modifications; related to but not the same as the "ux" label widgets Click-to-activate placeholders for blocked but potentially useful social buttons/widgets labels Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broken site enhancement heuristic Badger's core learning-what-to-block functionality ui User interface modifications; related to but not the same as the "ux" label widgets Click-to-activate placeholders for blocked but potentially useful social buttons/widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants