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 brave disable-machine-id and disable-encryption-win switches #795

Merged
merged 1 commit into from Jan 14, 2019

Conversation

@crazy-max
Copy link
Contributor

commented Nov 3, 2018

Fix brave/brave-browser#694

See brave/brave-browser#694 portapps/brave-portable#4

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

disable-machine-id

  • Launch brave with switch --disable-machine-id
  • Install an extension
  • Close brave and copy data to a second computer
  • Launch brave on the second computer
  • The extension must be ok

disable-encryption-win

  • Launch brave with switch --disable-encryption-win
  • Login to a known website with credentials and save them (will add unencrypted cookies and passwords to database)
  • Close brave and copy data to a second computer
  • Launch brave on the second computer
  • You must be still logged in on the website and credentials must be kept intact in the database

use both switches

  • Previous and following settings must be ok too :
    • Default search engine
    • Homepage
    • Startup pages
    • Pinned tabs

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source
@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2018

Hi @bridiver @garrettr @bbondy,

Your opinion on this PR?

Thanks

@bbondy

This comment has been minimized.

Copy link
Member

commented Dec 2, 2018

@riastradh-brave could you maybe look at this one?

@crazy-max crazy-max referenced this pull request Dec 4, 2018

Closed

Dev build way out of date #9

@caspertone2003

This comment has been minimized.

Copy link

commented Dec 20, 2018

@bridiver @garrettr @bbondy @riastradh-brave

It this patch not progressing or withdrawn?
Thanks

@caspertone2003

This comment has been minimized.

Copy link

commented Dec 29, 2018

@bridiver @garrettr @bbondy @riastradh-brave

Dear reviewers,
I am very interested in this patch. Could you please green light it?
I am stranded with muon until this does not resolved.

I know you probably work on this on a voluntary basis but almost two months to approve a patch seems to look that you are to busy and that this impacts in blocking progress.
If you can not take care of this PR, could you assign it to another person?

Thank you very much.

CT.

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Dec 29, 2018

@caspertone2003 we only use patches as a last resort because they add additional workload for every chromium update and this adds a significant amount of patching that doesn't seem to benefit Brave. What is the reason for disabling the machine id? We disable the sending of metrics so I'm not sure what the purpose would be. Also, why do you want to disable encryption? Even if we were to approve these features, the size of the patches would have to be reduced considerably. We just can't justify maintaining large patches that don't appear to be useful to Brave users

@andretiagogr

This comment has been minimized.

Copy link

commented Dec 29, 2018

What is the reason for disabling the machine id? We disable the sending of metrics so I'm not sure what the purpose would be. Also, why do you want to disable

I believe they want a portable version where all extensions and passwords are saved between different computers

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2018

@bridiver As stated brave/brave-browser#694 and portapps/brave-portable#4, this PR add 2 flags to disable encryption and machine-id to be able to use Brave as a portable version.
This modification has already been applied and tested on ungoogled-chromium (see Eloston/ungoogled-chromium#591)

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Dec 29, 2018

either way this patch is far too large for what it does. A chromium_src override for kPlatformSupportsPreferenceTracking might work for the machine-id. Not sure about os_crypt, but we just can't have these large patches so they need to be eliminated with chromium_src overrides or reduced significantly. There's also a dependency issue here because src/components and src/services now have dependencies on src/brave/common.

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2018

@bridiver Yes this PR is quite old now, will check this out. Thanks

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Dec 29, 2018

you could also potentially add in a utility method into the os_crypt files using chromium_src overrides to pull most of the code out of the patch itself. Something like if (DisableEncryption(ciphertext)) return true; as the actual patch with the rest of it handled in the chromium_src override

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

@bridiver I reduced the impact of patches by half. I had implemented some modifications in chromium_src but given the little change in the current patches I think it is no longer necessary. Can you give me your opinion? Thanks.

@caspertone2003

This comment has been minimized.

Copy link

commented Dec 31, 2018

@bridiver Big thanks for taking action on this PR
@crazy-max Big thanks for updating the PR

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 8, 2019

Thanks @crazy-max . We can further reduce these patches by combing them with chromium_src overrides for the same file. For instance for machine_id_provider_win.cc chromium_src override you could define IsMachineIdDisabled before including the original file and then just patch HasId() to call that method. The includes would all be in the chromium_src override so the patch itself would only be one line

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

For instance for machine_id_provider_win.cc chromium_src override you could define IsMachineIdDisabled before including the original file and then just patch HasId() to call that method. The includes would all be in the chromium_src override so the patch itself would only be one line

@bridiver Like 5592af7 ?

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

@crazy-max yes!

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@bridiver Done, let me know if it's ok

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2019

Thanks @crazy-max! Just a few small tweaks and it's good to go

@caspertone2003

This comment has been minimized.

Copy link

commented Jan 11, 2019

@bridiver
@crazy-max
Once again thank you for your dedication. I see a lot of frequent progress.
Should I be knowledgeable I would try to help, in this case I can only enjoy the outcome (and test it as needed). Thanks!

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 14, 2019

@crazy-max can you also squash the commits when you're done. I saw the update to fix the copy so just let me know when it's ready and I think it's good to merge

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@bridiver Ok but you can already squash & merge from GitHub no ? :

image

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 14, 2019

@crazy-max yes, but I don't know what it does with the commit messages (maybe the default of just listing them all) which isn't necessarily what we want. It's actually a checkbox that you have checked off in the template even though it isn't actually squashed :)

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

yes, but I don't know what it does with the commit messages (maybe the default of just listing them all) which isn't necessarily what we want. It's actually a checkbox that you have checked off in the template even though it isn't actually squashed :)

@bridiver Ok commits have been squashed !

@crazy-max crazy-max changed the title Add brave disable-machine-id and disable-encryption switches Add brave disable-machine-id and disable-encryption-win switches Jan 14, 2019

@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 14, 2019

@crazy-max before I merge can you amend the commit msg to have
Fix https://github.com/brave/brave-browser/issues/694 to auto-close the issue and check off Used Github auto-closing keywords in the commit message.

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

I have done this through the PR desc :

image

@bridiver bridiver merged commit d050ee0 into brave:master Jan 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bridiver

This comment has been minimized.

Copy link
Collaborator

commented Jan 14, 2019

Sorry it took so long and so much back and forth. I'm shifting my time to prioritize dealing with PRs over other work so hopefully it won't take so long in the future.

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@bridiver No problem I completely understand! Btw will I get a notification when it's merged into a release branch?

@caspertone2003

This comment has been minimized.

Copy link

commented Jan 14, 2019

@bridiver
@crazy-max

You are my heroes!
Thanks for your time and devotion!

@bsclifton bsclifton added this to the 0.61.x - Nightly milestone Jan 15, 2019

@bsclifton

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

@crazy-max no notification; but you can watch for version 0.61.x :) This should be released by the end of this month on the DEV channel (meaning it would potentially hit release channel 6 weeks after that)

@crazy-max

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

@bsclifton Ok thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.