-
Notifications
You must be signed in to change notification settings - Fork 350
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
Manually add URLs to a container #2114
Changes from 20 commits
0294fc5
d2f1ee9
826ecf0
40adf45
4881f64
b93879b
b011457
ea528ff
5f56aa8
a3f3413
e5d157e
1ac0fb2
66e21b5
c9b9a1d
e9dfb09
30a5111
fb0216f
1abc8b8
f9281ab
70cbaf8
6c94e13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1818,6 +1818,12 @@ Logic.registerPanel(P_CONTAINER_EDIT, { | |
Utils.addEnterHandler(document.querySelector("#create-container-ok-link"), () => { | ||
this._submitForm(); | ||
}); | ||
|
||
// Add new site to current container | ||
const siteLink = document.querySelector("#edit-container-site-link"); | ||
Utils.addEnterHandler(siteLink, () => { | ||
this._addSite(); | ||
}); | ||
}, | ||
|
||
async _submitForm() { | ||
|
@@ -1860,6 +1866,113 @@ Logic.registerPanel(P_CONTAINER_EDIT, { | |
return editedIdentity; | ||
}, | ||
|
||
async _addSite() { | ||
// Get URL and container ID from form | ||
const formValues = new FormData(this._editForm); | ||
const url = formValues.get("site-name"); | ||
const userContextId = formValues.get("container-id"); | ||
const currentTab = await Logic.currentTab(); | ||
const tabId = currentTab.id; | ||
const baseURL = this.normalizeUrl(url); | ||
|
||
if (baseURL !== null) { | ||
// Assign URL to container | ||
await Utils.setOrRemoveAssignment(tabId, baseURL, userContextId, false); | ||
|
||
// Clear form | ||
document.querySelector("#edit-container-panel-site-input").value = ""; | ||
|
||
// Show new assignments | ||
const assignments = await Logic.getAssignmentObjectByContainer(userContextId); | ||
this.showAssignedContainers(assignments); | ||
} | ||
}, | ||
|
||
normalizeUrl(url){ | ||
/* | ||
* Important: the rules that automatically open a site in a container only | ||
* look at the URL up to but excluding the / after the domainname. | ||
* | ||
* Furthermore, containers are only useful for http & https URLs because | ||
* those are the only protocols that transmit cookies to maintain | ||
* sessions. | ||
*/ | ||
|
||
// Preface with "https://" if no protocol present | ||
const startsWithProtocol = /^\w+:\/\/|^mailto:/; // all protocols are followed by :// (except mailto) | ||
if (!url.match(startsWithProtocol)) { | ||
url = "https://" + url; | ||
} | ||
|
||
/* | ||
* Dual-purpose match: (1) check that url start with http or https proto, | ||
* and (2) exclude everything from the / after the domain (any path, any | ||
* query string, and any anchor) | ||
*/ | ||
const basePart = /^(https?:\/\/)([^/?#]*)/; | ||
const r = url.match(basePart); | ||
if (!r) return null; | ||
const urlProto = r[1]; // includes :// if any | ||
const urlConnection = r[2]; // [user[:passwd]@]domain[:port] | ||
|
||
// Extract domain from [user[:passwd]@]domain[:port] | ||
const domainPart = /^(?:.*@)?([^:]+)/; | ||
const d = urlConnection.match(domainPart); | ||
if (!d) return null; | ||
const urlDomain = d[1]; | ||
|
||
// Check that the domain is valid (RFC-1034) | ||
const validDomain = /^(?:\w(?:[\w-]*\w)?)(?:\.\w(?:[\w-]*\w)?)+$/; | ||
if (!urlDomain.match(validDomain)) return null; | ||
|
||
return urlProto+urlDomain; | ||
}, | ||
|
||
showAssignedContainers(assignments) { | ||
const assignmentPanel = document.getElementById("edit-sites-assigned"); | ||
const assignmentKeys = Object.keys(assignments); | ||
assignmentPanel.hidden = !(assignmentKeys.length > 0); | ||
if (assignments) { | ||
const tableElement = assignmentPanel.querySelector(".assigned-sites-list"); | ||
/* Remove previous assignment list, | ||
after removing one we rerender the list */ | ||
while (tableElement.firstChild) { | ||
tableElement.firstChild.remove(); | ||
} | ||
|
||
assignmentKeys.forEach((siteKey) => { | ||
const site = assignments[siteKey]; | ||
const trElement = document.createElement("div"); | ||
/* As we don't have the full or correct path the best we can assume is the path is HTTPS and then replace with a broken icon later if it doesn't load. | ||
This is pending a better solution for favicons from web extensions */ | ||
const assumedUrl = `https://${site.hostname}/favicon.ico`; | ||
trElement.innerHTML = Utils.escaped` | ||
<div class="favicon"></div> | ||
<div title="${site.hostname}" class="truncate-text hostname"> | ||
${site.hostname} | ||
</div> | ||
<img | ||
class="pop-button-image delete-assignment" | ||
src="/img/container-delete.svg" | ||
/>`; | ||
Comment on lines
+1949
to
+1957
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 know we used 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. Oh man THAT looks very prehistoric indeed... 🤦♀️ *edit: |
||
trElement.getElementsByClassName("favicon")[0].appendChild(Utils.createFavIconElement(assumedUrl)); | ||
const deleteButton = trElement.querySelector(".delete-assignment"); | ||
const that = this; | ||
Utils.addEnterHandler(deleteButton, async () => { | ||
const userContextId = Logic.currentUserContextId(); | ||
// Lets show the message to the current tab | ||
// TODO remove then when firefox supports arrow fn async | ||
const currentTab = await Logic.currentTab(); | ||
Utils.setOrRemoveAssignment(currentTab.id, assumedUrl, userContextId, true); | ||
delete assignments[siteKey]; | ||
that.showAssignedContainers(assignments); | ||
}); | ||
trElement.classList.add("container-info-tab-row", "clickable"); | ||
tableElement.appendChild(trElement); | ||
}); | ||
} | ||
}, | ||
|
||
initializeRadioButtons() { | ||
const colorRadioTemplate = (containerColor) => { | ||
return Utils.escaped`<input type="radio" value="${containerColor}" name="container-color" id="edit-container-panel-choose-color-${containerColor}" /> | ||
|
@@ -1910,7 +2023,10 @@ Logic.registerPanel(P_CONTAINER_EDIT, { | |
Utils.addEnterHandler(document.querySelector("#manage-assigned-sites-list"), () => { | ||
Logic.showPanel(P_CONTAINER_ASSIGNMENTS, this.getEditInProgressIdentity(), false, false); | ||
}); | ||
|
||
// Only show ability to add site if it's an existing container | ||
document.querySelector("#edit-container-panel-add-site").hidden = !userContextId; | ||
// Make site input empty | ||
document.querySelector("#edit-container-panel-site-input").value = ""; | ||
document.querySelector("#edit-container-panel-name-input").value = identity.name || ""; | ||
document.querySelector("#edit-container-panel-usercontext-input").value = userContextId || NEW_CONTAINER_ID; | ||
const containerName = document.querySelector("#edit-container-panel-name-input"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,137 @@ | ||
const {initializeWithTab} = require("../common"); | ||
|
||
console.log("TRACE START"); | ||
describe("#1670", function () { | ||
console.log("TRACE 0"); | ||
beforeEach(async function () { | ||
console.log("TRACE 1.0"); | ||
this.webExt = await initializeWithTab(); | ||
console.log("TRACE 1.1"); | ||
}); | ||
console.log("TRACE 2"); | ||
|
||
afterEach(function () { | ||
console.log("TRACE 3.0"); | ||
this.webExt.destroy(); | ||
console.log("TRACE 3.1"); | ||
}); | ||
console.log("TRACE 4"); | ||
|
||
describe("creating a new container", function () { | ||
console.log("TRACE 5.0"); | ||
beforeEach(async function () { | ||
console.log("TRACE 5.1.0"); | ||
await this.webExt.popup.helper.clickElementById("manage-containers-link"); | ||
console.log("TRACE 5.1.1"); | ||
await this.webExt.popup.helper.clickElementById("new-container"); | ||
console.log("TRACE 5.1.2"); | ||
await this.webExt.popup.helper.clickElementById("create-container-ok-link"); | ||
console.log("TRACE 5.1.3"); | ||
}); | ||
console.log("TRACE 5.2"); | ||
|
||
it("should create it in the browser as well", function () { | ||
console.log("TRACE 5.3.0"); | ||
this.webExt.background.browser.contextualIdentities.create.should.have.been.calledOnce; | ||
}); | ||
console.log("TRACE 5.4"); | ||
|
||
describe("manually assign a valid URL to a container", function () { | ||
console.log("TRACE 5.5.0"); | ||
const exampleUrl = "https://github.com/mozilla/multi-account-containers"; | ||
beforeEach(async function () { | ||
console.log("TRACE 5.5.1.0"); | ||
await this.webExt.popup.helper.clickElementById("manage-containers-link"); | ||
console.log("TRACE 5.5.1.1"); | ||
await this.webExt.popup.helper.clickElementByQuerySelectorAll(".edit-container-icon", "last"); | ||
console.log("TRACE 5.5.1.2"); | ||
this.webExt.popup.window.document.getElementById("edit-container-panel-site-input").value = exampleUrl; | ||
console.log("TRACE 5.5.1.3"); | ||
await this.webExt.popup.helper.clickElementById("edit-container-site-link"); | ||
console.log("TRACE 5.5.1.4"); | ||
}); | ||
console.log("TRACE 5.5.2"); | ||
|
||
it("should assign the URL to a container", function () { | ||
console.log("TRACE 5.5.3.0"); | ||
this.webExt.background.browser.contextualIdentities.create.should.have.been.calledOnce; | ||
}); | ||
console.log("TRACE 5.5.4"); | ||
}); | ||
console.log("TRACE 5.6"); | ||
|
||
describe("manually assign valid URL without protocol to a container", function () { | ||
console.log("TRACE 5.7.0"); | ||
const exampleUrl = "github.com/mozilla/multi-account-containers"; | ||
beforeEach(async function () { | ||
console.log("TRACE 5.7.1.0"); | ||
await this.webExt.popup.helper.clickElementById("manage-containers-link"); | ||
console.log("TRACE 5.7.1.1"); | ||
await this.webExt.popup.helper.clickElementByQuerySelectorAll(".edit-container-icon", "last"); | ||
console.log("TRACE 5.7.1.2"); | ||
this.webExt.popup.window.document.getElementById("edit-container-panel-site-input").value = exampleUrl; | ||
console.log("TRACE 5.7.1.3"); | ||
await this.webExt.popup.helper.clickElementById("edit-container-site-link"); | ||
console.log("TRACE 5.7.1.4"); | ||
}); | ||
console.log("TRACE 5.7.2"); | ||
|
||
it("should assign the URL without protocol to a container", function () { | ||
console.log("TRACE 5.7.3.0"); | ||
this.webExt.background.browser.contextualIdentities.create.should.have.been.calledOnce; | ||
}); | ||
console.log("TRACE 5.7.4"); | ||
}); | ||
console.log("TRACE 5.8"); | ||
|
||
describe("manually assign an invalid URL to a container", function () { | ||
console.log("TRACE 5.9.0"); | ||
const exampleUrl = "github"; | ||
beforeEach(async function () { | ||
console.log("TRACE 5.9.1.0"); | ||
await this.webExt.popup.helper.clickElementById("manage-containers-link"); | ||
console.log("TRACE 5.9.1.1"); | ||
await this.webExt.popup.helper.clickElementByQuerySelectorAll(".edit-container-icon", "last"); | ||
console.log("TRACE 5.9.1.2"); | ||
this.webExt.popup.window.document.getElementById("edit-container-panel-site-input").value = exampleUrl; | ||
console.log("TRACE 5.9.1.3"); | ||
await this.webExt.popup.helper.clickElementById("edit-container-site-link"); | ||
console.log("TRACE 5.9.1.4"); | ||
}); | ||
console.log("TRACE 5.9.2"); | ||
|
||
it("should not assign the URL to a container", function () { | ||
console.log("TRACE 5.9.3.0"); | ||
this.webExt.background.browser.contextualIdentities.update.should.not.have.been.called; | ||
}); | ||
console.log("TRACE 5.9.4"); | ||
}); | ||
console.log("TRACE 5.10"); | ||
|
||
describe("manually assign empty URL to a container", function () { | ||
console.log("TRACE 5.11.0"); | ||
const exampleUrl = ""; | ||
beforeEach(async function () { | ||
console.log("TRACE 5.11.1.0"); | ||
await this.webExt.popup.helper.clickElementById("manage-containers-link"); | ||
console.log("TRACE 5.11.1.1"); | ||
await this.webExt.popup.helper.clickElementByQuerySelectorAll(".edit-container-icon", "last"); | ||
console.log("TRACE 5.11.1.2"); | ||
this.webExt.popup.window.document.getElementById("edit-container-panel-site-input").value = exampleUrl; | ||
console.log("TRACE 5.11.1.3"); | ||
await this.webExt.popup.helper.clickElementById("edit-container-site-link"); | ||
console.log("TRACE 5.11.1.4"); | ||
}); | ||
console.log("TRACE 5.11.2"); | ||
|
||
it("should not assign the URL to a container", function () { | ||
console.log("TRACE 5.11.3.0"); | ||
this.webExt.background.browser.contextualIdentities.update.should.not.have.been.called; | ||
}); | ||
console.log("TRACE 5.11.4"); | ||
}); | ||
console.log("TRACE 5.12"); | ||
}); | ||
console.log("TRACE 6"); | ||
}); | ||
console.log("TRACE END"); |
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'm pretty sure normalization isn't necessary. Assuming the below call to
Logic.setOrRemoveAssignment
should beUtils.setOrRemoveAssignment
,Utils.setOrRemoveAssignment
send a message whichmulti-account-containers/src/js/utils.js
Lines 93 to 99 in 50f5ebf
messageHandler.init
handles by callingmulti-account-containers/src/js/background/messageHandler.js
Line 49 in 50f5ebf
assignManager._setOrRemoveAssignment
which callsmulti-account-containers/src/js/background/assignManager.js
Lines 560 to 563 in 50f5ebf
storageArea.set
which filters the URL throughmulti-account-containers/src/js/background/assignManager.js
Line 73 in 50f5ebf
storageArea.getSiteStoreKey
which extracts the hostname and possibly the port from the URLmulti-account-containers/src/js/background/assignManager.js
Lines 14 to 20 in 50f5ebf
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'm fairly new to this, so any help you can give - perhaps a PR into my patch - would be very much appreciated.
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.
Isn't the point of
normalizeUrl
to trim the path off? Given that it's random user input, this seems like a fairly sensible idea.It looks like you're right about the
Utils.setOrRemoveAssignment
since it's only defined in src/js/utils.js, so I've committed that change too.