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

Inject metamask on a per-page basis #537

Closed
kumavis opened this issue Aug 12, 2016 · 10 comments
Closed

Inject metamask on a per-page basis #537

kumavis opened this issue Aug 12, 2016 · 10 comments
Assignees

Comments

@kumavis
Copy link
Member

kumavis commented Aug 12, 2016

allow users to not inject metamask on a page

@kumavis kumavis added this to the Future Release milestone Aug 12, 2016
@danfinlay
Copy link
Contributor

The request was specifically to allow "disable for this domain", which we could probably add to the contextual menu as well as the settings.

  • Dan

On Aug 12, 2016, at 6:32 AM, kumavis notifications@github.com wrote:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@danfinlay
Copy link
Contributor

I think we now have come around to want to allow sign-in per page, not just opt-out.

This has privacy and performance benefits.

One question, is should we require an additional password verification per additional site? Probably reasonable when initially signing in, and then it could auto-sign in to those sites once unlocked in the future by default.

I do think we would need some UX for what it's like to sign in to some sites but not others, and how to maybe manage that configuration. This would be a nice task for @VladTod.

@danfinlay danfinlay modified the milestones: Public Release, Future Release Dec 2, 2016
@danfinlay
Copy link
Contributor

Moving up to Public Release for the privacy concerns.

@danfinlay
Copy link
Contributor

To differentiate this issue from #714:

That issue is about signing in with an account before injecting that account into the web3 object.

This issue is about not injecting the web3 object.

My earlier comment here belongs in that other issue.

@Tectract
Copy link

Here's my code which loads metamask, if available. Otherwise it defaults to localhost:8545:

function loadWeb3() {
   let web3Injected = window.web3;
   if(typeof web3Injected !== 'undefined'){
     console.log("saw injected web3!");
     web3 = new Web3(web3Injected.currentProvider);
   } else {
     console.log("did not see web3 injected!");
     web3 = new Web3(new Web3.providers.HttpProvider("http://localhost:8545"));
     //console.debug(web3.eth.accounts);
   }
}

@lukehedger
Copy link

@FlySwatter @kumavis Wondering what the status of this proposal is? I arrived here out of a desire to be able to switch MetaMask Web3 injection on/off easily - current practice is to enable/disable the Chrome extension manually. Happy to help out if this feature is still in development/not started.

@danfinlay
Copy link
Contributor

This is a tricky feature, because it fundamentally changes both a UX flow and an architecture (potentially multiple blockchain connections). We're currently doing a lot of UX work right now, and so there's a high chance of work on this conflicting with that work, until it's settled a bit more.

We still value this feature, and I personally consider it a basic privacy feature that needs to be added, but it just needs to be added at the right time, with the right strategy. If you were seriously open to taking on a project that large, I would ask you to first familiarize yourself with the codebase with some smaller issues, ideally one involving the provider system (app/scripts/controllers/network.js), as well as something involving our UI & routing, which is under more flux at the moment, mostly rooted in ui/app/app.js.

@danfinlay
Copy link
Contributor

We have a github tag for good-first-contributon.

Looking there, issues that might satisfy those two domains I referred to are:

Provider:

UI/Routing:

Other relevant issues aren't listed with that tag at the moment.

@rumkin
Copy link

rumkin commented Jan 31, 2018

This is too serious vulnerability to be opened more than a year. Set proper importance labels, please.

@danfinlay
Copy link
Contributor

Closing as duplicate #714.

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

No branches or pull requests

6 participants