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

Add support for Private / Incognito Mode #2121

Closed
wants to merge 1 commit into from

Conversation

jbis9051
Copy link

@jbis9051 jbis9051 commented Oct 15, 2021

Please see comments below for context

#136 (comment)

#136 (comment)

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2021

CLA assistant check
All committers have signed the CLA.

@eliykat eliykat changed the title Fixes #136 Add support for Private / Incognito Mode Oct 18, 2021
@cscharf cscharf added discussion hold do not merge, do not approve yet labels Oct 27, 2021
@cscharf
Copy link
Contributor

cscharf commented Oct 27, 2021

Thanks @jbis9051 , we have some internal discussions in regards to this and will get back to you with any questions or concerns. This would also require a bit of the team's resources for testing and a product decision depending on any regression issues or broken functionality we would need to account for while in private mode.

@fa7ad
Copy link

fa7ad commented Nov 27, 2021

I realize this comment is probably not helpful, but given that this has been an issue for quite some time are there any plans to merge this PR anytime soon?

@eliykat
Copy link
Member

eliykat commented Nov 29, 2021

Hey @fa7ad, I'll set aside some time to look into it this week. That doesn't mean merging this week, we need to figure out what features do and don't work using this workaround, and whether that's something that we can accommodate. I'll post any updates here.

EDIT: I tested and debugged some issues with this today and have passed my feedback onto the team for internal discussion about where to go next.

@eliykat eliykat self-assigned this Dec 3, 2021
@jbis9051
Copy link
Author

jbis9051 commented Dec 3, 2021

@eliykat Did you find any major issues?

@eliykat
Copy link
Member

eliykat commented Dec 7, 2021

There are some tweaks that need to be made to the messaging services, because they don't work now that everything is running in a single frame. However, I hacked together a fix for that in the course of my testing, so I'm happy to fix that issue if you don't mind me branching off your work.

The bigger issue is that some features won't work due to the lack of state:

  • PIN or biometric unlock
  • SSO login
  • right-click context menu
  • plus having to unlock your vault each time you open the popup

I personally think these are acceptable limitations for now (for about ~12 months until we upgrade to the v3 manifest), but we need to decide how to flag these limitations to the user so we don't end up with a bunch of support tickets. Plus get QA to do some full regression testing to make sure there's nothing I've missed.

@jbis9051
Copy link
Author

jbis9051 commented Dec 9, 2021

@eliykat

so I'm happy to fix that issue if you don't mind me branching off your work

Feel free to. This was mostly a proof of concept to get the ball rolling. You should also be able to push directly to https://github.com/jbis9051/browser (I think).

The bigger issue is that some features won't work due to the lack of state:

I suspect that we could leverage the messaging APIs listed here, namely runtime.sendMessage or runtime.connect, to store minimal state in the background script. We would just store things like the session key needed to access the vault.

@fa7ad
Copy link

fa7ad commented Dec 10, 2021

Thank you guys for all the hard work, I took a crack at trying to solve the stateless-ness issue, and the messaging APIs were promising, but the problem that frustrated me most was trying to build a firefox extension. Because this change adds some imports, the bundle size goes up significantly in some files and goes over the per-file limit of what mozilla allows, I played around a bit with the webpack config to get it working but webpack being webpack, there are some issues; specifically not being able to rename the initial chunk reliably on webpack 4 (my approach was to limit the chunk sizes and see if that would work). I don't know if webpack 5 is any better though. I'll try again tonight and hopefully report back how it goes (possibly an addition to this PR or a new one).
Once again, thank you all for all the great work that you're doing.

(Sidenote: Sorry I couldn't be of more help, I work with react mostly at work and Angular is very foreign to me, so I'm mostly relying on standard/normal/non-angular JS/TS)

@djsmith85
Copy link
Contributor

@fa7ad Just as an FYI, we just bumped Angular and with it also webpack to 5

@jbis9051
Copy link
Author

FYI: I've rebased to include all the changes

cc @fa7ad

@jbis9051
Copy link
Author

jbis9051 commented Dec 14, 2021

@eliykat Not sure if it has to do with the rebase but I am not seeing an issue with the right click context menu.

@jbis9051
Copy link
Author

Another issue I am seeing is that once you unlock the vault, the lock button doesn't work.

@eliykat
Copy link
Member

eliykat commented Dec 14, 2021

Thanks @jbis9051, I'll branch off my work and spend some time on it, probably in January.

@jbis9051 and @fa7ad, we are probably looking at an MVP here - i.e. what are the minimum changes we can make to have some basic solution for the community. We will need to completely change our messaging APIs when we upgrade to manifest v3 in ~12 months, so we don't want to overhaul them now only for private mode. If some functionality is limited in private mode in the meantime, that's okay, it's the cost of having private mode as an option at all.

@fa7ad please feel free to continue and share your work if it's something you're interested in, but it may not be merged if it involves significant changes. I might be overstating the code footprint though, if it's a small fix then we can at least look at it.

@jbis9051
Copy link
Author

jbis9051 commented Dec 14, 2021

@eliykat After looking at auth.service.ts, it seems that we could serialize the post-authentication state. Unfortunately, it's not a single key. .

I can probably create some minimal hacky solution but it would require changes to jslib. I don't think a hacky solution is too big of an issue if most of jslib is going to be remade anyway in ~12 months.

Another idea is to just call say something like "Private browsing in Firefox is in beta. Please don't submit any issues." and accept these as "acceptable limitations" as you said. I don't think most people will be logging into many websites while in private browsing (or they probably wouldn't be in private browsing) so UX isn't significantly degraded.

@eliykat
Copy link
Member

eliykat commented Dec 15, 2021

Yes, a callout element on the login and unlock pages is exactly what I was thinking. ("Private Mode detected. Some features will not work. Learn more." with a link to a help site)

At least for a first iteration of having more than zero functionality in private mode.

@Hinton
Copy link
Member

Hinton commented Dec 15, 2021

I don't believe we will get any functionality outside the popup working since they all rely on the background page which won't work without a fair bit of refactor. On the bright side (or bad) the manifest v3 will require major work towards re-organizing that logic. Manifests v3 are not fully supported by all browsers though, but the Chrome cut-off date for Manifest V2 is January 2023.

@eliykat eliykat mentioned this pull request Jan 27, 2022
8 tasks
@jbis9051
Copy link
Author

Closed in favor of #2294

@jbis9051 jbis9051 closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion hold do not merge, do not approve yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants