-
Notifications
You must be signed in to change notification settings - Fork 325
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
feat(mv3): Ask for Host Permissions if not exist. #1250
Changes from 5 commits
64185dc
a31b57b
cd00c85
7b2dff1
dbdfb6a
9453464
a676e35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this file is copied from recovery.css, I have a todo item to make these more generic, will look into it after mv3. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
@import url('~tachyons/css/tachyons.css'); | ||
@import url('~ipfs-css/ipfs.css'); | ||
|
||
#left-col { | ||
background-image: url('../../../images/stars.png'), linear-gradient(to bottom, #041727 0%, #043b55 100%); | ||
background-size: 100%; | ||
background-repeat: repeat; | ||
} | ||
|
||
a:hover { | ||
text-decoration: none; | ||
} | ||
|
||
a:visited { | ||
color: inherit; | ||
} | ||
|
||
/* | ||
https://github.com/tachyons-css/tachyons-queries | ||
Tachyons: $point == large | ||
*/ | ||
@media (min-width: 60em) { | ||
#left-col { | ||
position: fixed; | ||
top: 0; | ||
right: 55%; | ||
width: 45%; | ||
background-image: url('../../../images/stars.png'), linear-gradient(to bottom, #041727 0%, #043b55 100%); | ||
background-size: 100%; | ||
background-repeat: repeat; | ||
} | ||
|
||
#right-col { | ||
margin-left: 54%; | ||
margin-right: 6%; | ||
} | ||
} | ||
|
||
@media (max-height: 800px) { | ||
#left-col img { | ||
width: 98px !important; | ||
height: 98px !important; | ||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah! !important usually ends up cascading need for important. A little too self-important of the property if you ask me. to confirm, this is just for the permissions page, and in no way leaks into our other CSS, correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is correct, I do have #1252 which will be a refactor at a later stage. |
||
} | ||
|
||
#left-col svg { | ||
width: 60px; | ||
} | ||
} | ||
|
||
.recovery-root { | ||
width: 100%; | ||
height: 100%; | ||
text-align: left; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<title>IPFS Node is Offline</title> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width"> | ||
<link rel="shortcut icon" href="data:image/x-icon;base64,AAABAAEAEBAAAAEAIABoBAAAFgAAACgAAAAQAAAAIAAAAAEAIAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAlo89/56ZQ/8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACUjDu1lo89/6mhTP+zrVP/nplD/5+aRK8AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHNiIS6Wjz3/ubFY/761W/+vp1D/urRZ/8vDZf/GvmH/nplD/1BNIm8AAAAAAAAAAAAAAAAAAAAAAAAAAJaPPf+knEj/vrVb/761W/++tVv/r6dQ/7q0Wf/Lw2X/y8Nl/8vDZf+tpk7/nplD/wAAAAAAAAAAAAAAAJaPPf+2rVX/vrVb/761W/++tVv/vrVb/6+nUP+6tFn/y8Nl/8vDZf/Lw2X/y8Nl/8G6Xv+emUP/AAAAAAAAAACWjz3/vrVb/761W/++tVv/vrVb/761W/+vp1D/urRZ/8vDZf/Lw2X/y8Nl/8vDZf/Lw2X/nplD/wAAAAAAAAAAlo89/761W/++tVv/vrVb/761W/++tVv/r6dQ/7q0Wf/Lw2X/y8Nl/8vDZf/Lw2X/y8Nl/56ZQ/8AAAAAAAAAAJaPPf++tVv/vrVb/761W/++tVv/vbRa/5aPPf+emUP/y8Nl/8vDZf/Lw2X/y8Nl/8vDZf+emUP/AAAAAAAAAACWjz3/vrVb/761W/++tVv/vrVb/5qTQP+inkb/op5G/6KdRv/Lw2X/y8Nl/8vDZf/Lw2X/nplD/wAAAAAAAAAAlo89/761W/++tVv/sqlS/56ZQ//LxWb/0Mlp/9DJaf/Kw2X/oJtE/7+3XP/Lw2X/y8Nl/56ZQ/8AAAAAAAAAAJaPPf+9tFr/mJE+/7GsUv/Rymr/0cpq/9HKav/Rymr/0cpq/9HKav+xrFL/nplD/8vDZf+emUP/AAAAAAAAAACWjz3/op5G/9HKav/Rymr/0cpq/9HKav/Rymr/0cpq/9HKav/Rymr/0cpq/9HKav+inkb/nplD/wAAAAAAAAAAAAAAAKKeRv+3slb/0cpq/9HKav/Rymr/0cpq/9HKav/Rymr/0cpq/9HKav+1sFX/op5G/wAAAAAAAAAAAAAAAAAAAAAAAAAAop5GUKKeRv/Nxmf/0cpq/9HKav/Rymr/0cpq/83GZ/+inkb/op5GSAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAop5G16KeRv/LxWb/y8Vm/6KeRv+inkaPAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAop5G/6KeRtcAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/n8AAPgfAADwDwAAwAMAAIABAACAAQAAgAEAAIABAACAAQAAgAEAAIABAACAAQAAwAMAAPAPAAD4HwAA/n8AAA==" /> | ||
<link rel="stylesheet" href="/dist/bundles/uiCommons.css"> | ||
<link rel="stylesheet" href="/dist/bundles/requestPermissionsPage.css"> | ||
</head> | ||
<body class="navy bg-white sans-serif"> | ||
<app class="flex flex-column transition-all vh-100"> | ||
<main class="bg-white flex-grow-1"> | ||
<div id="root"></div> | ||
</main> | ||
<script src="/dist/bundles/uiCommons.bundle.js"></script> | ||
<script src="/dist/bundles/requestPermissionsPage.bundle.js"></script> | ||
</app> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
'use strict' | ||
/* eslint-env browser, webextensions */ | ||
|
||
import choo from 'choo' | ||
import html from 'choo/html/index.js' | ||
import { i18n, runtime, permissions } from 'webextension-polyfill' | ||
import { nodeOffSvg } from '../welcome/page.js' | ||
import createWelcomePageStore from '../welcome/store.js' | ||
import { optionsPage } from '../../lib/constants.js' | ||
import './request.css' | ||
|
||
const app = choo() | ||
|
||
const learnMoreLink = html`<a class="navy link underline-under hover-aqua" href="https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/host_permissions#requested_permissions_and_user_prompts" target="_blank" rel="noopener noreferrer">${i18n.getMessage('request_permissions_page_learn_more')}</a>` | ||
|
||
const optionsPageLink = html`<a class="navy link underline-under hover-aqua" id="learn-more" href="${optionsPage}" target="_blank" rel="noopener noreferrer">${i18n.getMessage('recovery_page_update_preferences')}</a>` | ||
|
||
// TODO (whizzzkid): refactor base store to be more generic. | ||
app.use(createWelcomePageStore(i18n, runtime)) | ||
// Register our single route | ||
app.route('*', () => { | ||
runtime.sendMessage({ telemetry: { trackView: 'request-permissions' } }) | ||
const requestPermission = async () => { | ||
await permissions.request({ origins: ['<all_urls>'] }) | ||
runtime.reload() | ||
} | ||
|
||
return html`<div class="flex flex-column flex-row-l"> | ||
<div id="left-col" class="min-vh-100 flex flex-column justify-center items-center bg-navy white"> | ||
<div class="mb4 flex flex-column justify-center items-center"> | ||
${nodeOffSvg(200)} | ||
<p class="mt0 mb0 f3 tc">${i18n.getMessage('request_permissions_page_sub_header')}</p> | ||
</div> | ||
</div> | ||
|
||
<div id="right-col" class="pt7 mt5 w-100 flex flex-column justify-around items-center"> | ||
<p class="f3 fw5">${i18n.getMessage('request_permissions_page_message_p1')}</p> | ||
<p class="f4 fw4">${i18n.getMessage('request_permissions_page_message_p2')}</p> | ||
<button | ||
class="fade-in ba bw1 b--teal bg-teal snow f7 ph2 pv3 br2 ma4 pointer" | ||
onclick=${requestPermission} | ||
> | ||
<span class="f5 fw6">${i18n.getMessage('request_permissions_page_button')}</span> | ||
</button> | ||
<p class="f5 fw2 pt5"> | ||
${learnMoreLink} | ${optionsPageLink} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: clicking on We should check if the permission tab already exists, and if so, switch focus to it, instead of creating a second copy. Not a blocker, can be follow-up PR 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
</span> | ||
</div> | ||
</div>` | ||
}) | ||
|
||
// Start the application and render it to the given querySelector | ||
app.mount('#root') | ||
|
||
// Set page title and header translation | ||
document.title = i18n.getMessage('request_permissions_page_title') |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
|
||
import browser from 'webextension-polyfill' | ||
import debug from 'debug' | ||
import { welcomePage } from './constants.js' | ||
import { requestRequiredPermissionsPage, welcomePage } from './constants.js' | ||
import { brave, braveNodeType } from './ipfs-client/brave.js' | ||
|
||
const { version } = browser.runtime.getManifest() | ||
|
@@ -21,6 +21,15 @@ export async function onInstalled (details) { | |
export async function runPendingOnInstallTasks () { | ||
const { onInstallTasks, displayReleaseNotes } = await browser.storage.local.get(['onInstallTasks', 'displayReleaseNotes']) | ||
await browser.storage.local.remove('onInstallTasks') | ||
// this is needed because `permissions.request` cannot be called from a script. If that happens the browser will | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// throws: Error: permissions.request may only be called from a user input handler | ||
// To avoid this, we open a new tab with the permissions page and ask the user to grant the permissions. | ||
// That makes the request valid and allows us to gain access to the permissions. | ||
if (!(await browser.permissions.contains({ origins: ['<all_urls>'] }))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if a user doesn't grant permissions in firefox, will they be constantly bombarded with the permissions page? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, if we don't have permissions then we cannot proceed. So:
We can as an improvement just show this once, if denied we load the extension in offline mode and show the button in the options screen. That's quite a bit more than just doin this. I can create a follow-up ticket if you'd like. |
||
return browser.tabs.create({ | ||
url: requestRequiredPermissionsPage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that I think about it a day later, I think this can also exist as a menu item on the options page, the only downside is the landing page will be options page before the welcome page. This way we isolate that. Let me know if the reviewers feel this is an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep the same flow: initial landing page should be the welcome page. We could polish the welcome page to check for permissions and have some messaging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but then the welcome page shows that the node is offline, we can either add this as notification on all the views or just keep it simple like this. What I believe might happen in the future is firefox changing their mind and make these permissions required instead of optional like chrome does. |
||
}) | ||
} | ||
switch (onInstallTasks) { | ||
case 'onFirstInstall': | ||
await useNativeNodeIfFeasible(browser) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ const optionsPageLink = html`<a class="navy link underline-under hover-aqua" id= | |
app.use(createWelcomePageStore(i18n, runtime)) | ||
// Register our single route | ||
app.route('*', (state) => { | ||
browser.runtime.sendMessage({ telemetry: { trackView: 'recovery' } }) | ||
runtime.sendMessage({ telemetry: { trackView: 'recovery' } }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this seems like it should be a separate fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're not wrong, that's the source for this change, I think it was working like so, but just to make it consistent. It will go away in #1252 |
||
const { hash } = window.location | ||
const { href: publicURI } = new URL(decodeURIComponent(hash.slice(1))) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should communicate exactly what we're doing here. Can we link to your mv3 wiki / explainer doc?
I want to make sure we keep users from thinking that we're observing site-content or in some way profiting off of their site data. IIUC, the only thing we're doing is noticing that a URL is being loaded, checking if that URL is "an IPFS supporting website", and then adding a redirect rule & refreshing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, and has been covered in the extension description, this screen only shows up if the permissions don't exist, would you like to take a stab at fixing the language which comes across as non-invasive but required permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted the language in 9453464 to make it less scary and more informative (focus on WHY we need it, not about API extravaganza :)).