Introduce IPFS promo infobar #19588

cypt4 · 2023-08-07T11:06:15Z

Resolves brave/brave-browser#32010
Privacy review: https://github.com/brave/reviews/issues/1371

New infobar is shown when user enters some page which can be loaded via ipfs:// (dnslink pages, gateway-like pages).
IpfsTabHelper creates infobar on expected conditions via BraveIpfsInfobarDelegate.
BraveConfirmInfobar is forked from ConfirmInfobar to allow reordering of buttons.

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Open some ipfs-gateway like resource like "https://ipfs.io/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"
Check that infobar is shown
Press [x], check that infobar appears after re-enter the url
Press "Only this time" check that ipfs:// url is loaded but "Automatically redirect requests for IPFS network resources to the configured gateway."
Check that infobar is shown after re-enter the url
Press "Aways button" check that "Automatically redirect requests for IPFS network resources to the configured gateway." setting is enabled after and infobar is not shown again.
On fresh browser enter "https://ipfs.io/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi" and check that after pressing "No Thanks" button infobar is not shown and page is not redirected to the ipfs://
Check that infobar is not shown on other sites(where purple button is not shown)

Also need to check obsolete system infobar brave/brave-browser#32129

github-actions · 2023-08-08T12:10:19Z

browser/ui/views/infobars/brave_confirm_infobar.cc

+      [this](ConfirmInfoBarDelegate::InfoBarButton type,
+             void (BraveConfirmInfoBar::*click_function)()) {
+        auto* button = AddChildView(std::make_unique<views::MdTextButton>(
+            base::BindRepeating(click_function, base::Unretained(this)),


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/c/chromium-uaf.yaml

Cc @thypon @goodov @iefremov

This was copied from ConfirmInfobar, i will use weak ptr here

I think we don't need weak ptr here because button is owned by BraveConfirmInforBar.
Passed |click_function| is valid always till |button| is gone.

mkarolin · 2023-08-08T20:29:02Z

app/brave_generated_resources.grd

@@ -876,6 +876,21 @@ Or change later at <ph name="SETTINGS_EXTENIONS_LINK">$2<ex>brave://settings/ext
      <message name="IDS_BRAVE_IPFS_LEARN_MORE">
        Learn more about IPFS and privacy
      </message>
+      <message name="IDS_BRAVE_IPFS_INFOBAR_TEXT" desc="Text that promotes turning on autoredirect to the configured gateway setting">
+        Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave?


@rmcfadden3 any thoughts on this?

Maybe just move the parents like so:

Would you like to handle content-addressed resources (ipfs:// and ipns://) with native IPFS support built into Brave?

or

Would you like Brave to natively handle content-addressed resources such as ipfs:// and ipns://?

or

Would you like to use native IPFS support built into Brave to handle content-addressed resources (ipfs:// and ipns://)?

Per Slack convo with @mkarolin …

Let's avoid phrasing like "content-addressed resources" as they may not be fully understood by all users. Instead, we landed on the following:

If a site can be opened with an IPFS link (like ipfs:// or ipns://), would you like to open it with Brave's built-in IPFS support?

I am afraid these changed sentences bury the lead – this feature is not about enabling ipfs:// but UPGRADING https:// to ipfs:// and ipns:// when possible.

"IPFS resources" on the web could be https:// URLs of public gateways like:

https://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi.ipfs.w3s.link

https://cloudflare-ipfs.com/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi

Or DNSLink website like:

https://en.wikipedia-on-ipfs.org

Ref. https://docs.ipfs.tech/how-to/address-ipfs-on-web

Brave detects these resources and instead of loading them over HTTP, it uses local IPFS node and presents them as loaded via ipfs:// or ipns:// in the address bar, to indicate native IPFS was used and the hashes were verified.

With that in mind, perhaps below will work better?

Suggested change

Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave?

Would you like to upgrade IPFS resources like this and load them using the native support (ipfs:// and ipns://) built into Brave?

Or even shorter:

Suggested change

Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave?

Would you like to upgrade IPFS resources to native handler (ipfs:// and ipns://) built into Brave?

Last version is good, would just like one change:

"opened with an IPFS link" is not ideal, given how IPFS works. just making a link is not enough.

"If a site is available on IPFS" or "on the IPFS network" or something like that would be better.

Didn't see Lidel's comment before adding this.

He's correct about the overall purpose of the bar.

However, I think upgrade is not correct - it implies a value to the change.

"enable" or something like that would be better.

@lidel @autonome PTAL^

Apologies for spending so much time on this text. It's a major conversion point, so it's important to us to get it right 😄

"such" works for en-uk but not for en-us.

How's this attempt to simplify + clarify:

This page is available on the IPFS network. Load IPFS pages using Brave's built-in support via ipfs:// and ipns:// ? [Always][Only This Time][No Thanks]

Changes

s/content/page/, as it's more descriptive, aligns w/ web terminology, and "content" is producer/publisher language not end-user language

remove parens to be more explicit that the presence of those schemes are what it looks like when the built-in support is being used

highlight the protocol schemes

@autonome — fair points. But I'd suggest we keep the "Would you…" at the start of the sentence to signify this is a question. And the ? after the "ipns://" reads a bit confusing (it appears like it could be part of the scheme). So what if we compromise at:

This page is available on the IPFS network. Would you like to load pages like this using Brave's built-in IPFS support (via ipfs:// or ipns://)? [Always] [Only This Time] [No Thanks]

If not the above, here's an alt version that's a bit closer to what you suggested:

This page is available on the IPFS network. Would you like to load IPFS pages using Brave's built-in support (via ipfs:// or ipns://)? [Always] [Only This Time] [No Thanks]

@rmcfadden3 thanks, either of those two wfm!

mkarolin

chromium_src++

simonhong

++ with mostly trivial comments.

simonhong · 2023-08-16T00:23:50Z

browser/infobars/brave_ipfs_infobar_delegate.cc

+BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() {}
+
+BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() {}


nit:

Suggested change

BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() {}

BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() {}

BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() = default;

BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() = default;

simonhong · 2023-08-16T00:55:31Z

browser/ipfs/ipfs_tab_helper.cc

+    }
+  }
+
+  ~BraveIPFSInfoBarDelegateObserverImpl() override {}


ditto

Suggested change

~BraveIPFSInfoBarDelegateObserverImpl() override {}

~BraveIPFSInfoBarDelegateObserverImpl() override = default;

simonhong · 2023-08-16T01:09:30Z

browser/infobars/brave_ipfs_infobar_delegate.cc

+    PrefService* local_state)
+    : observer_(std::move(observer)), local_state_(local_state) {}
+
+BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() {}


Suggested change

BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() {}

BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() = default;

simonhong · 2023-08-16T01:12:27Z

browser/infobars/brave_ipfs_infobar_delegate.cc

+      NOTREACHED();
+  }
+  return std::u16string();


Suggested change

NOTREACHED();

}

return std::u16string();

NOTREACHED_NORETURN();

}

simonhong · 2023-08-16T01:15:05Z

browser/infobars/brave_ipfs_infobar_delegate.h

+class BraveIPFSInfoBarDelegateObserver {
+ public:
+  BraveIPFSInfoBarDelegateObserver();
+  virtual ~BraveIPFSInfoBarDelegateObserver();


this dtor could be moved to protected section as this'll be destroyed from subclass.

There is std::uniqie_ptr usage in the BraveIPFSInfoBarDelegate

simonhong · 2023-08-16T01:27:57Z

browser/ui/views/infobars/brave_confirm_infobar.cc

    // We don't consider case where ok/cancel button and check box exist
    // together.
-    DCHECK(!ok_button_ && !cancel_button_);


Let's delete above three lines as we could have all together.

simonhong · 2023-08-16T01:34:49Z

browser/ui/views/infobars/brave_confirm_infobar.cc

+  const int button_count =
+      (ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0);


nit: how about simply like below?

Suggested change

const int button_count =

(ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0);

const int button_count = GetDelegate()->GetButtonsOrder().size();

simonhong · 2023-08-16T01:51:48Z

browser/ui/views/infobars/brave_confirm_infobar.h

-  BraveConfirmInfoBarDelegate* GetBraveDelegate();
+  BraveConfirmInfoBarDelegate* GetDelegate();
+
+  views::MdTextButton* ok_button_for_testing() { return ok_button_; }


This method could be deleted.

simonhong · 2023-08-16T02:01:53Z

browser/infobars/brave_ipfs_infobar_delegate.cc

+  NOTREACHED();
+  return std::u16string();


Suggested change

NOTREACHED();

return std::u16string();

NOTREACHED_NORETURN();

simonhong · 2023-08-16T02:05:16Z

browser/ui/views/infobars/brave_confirm_infobar.h


+ protected:
+  // InfoBarView:
+  int GetContentMinimumWidth() const override;


It seems there is no reason to have this in protected section.

simonhong · 2023-08-16T11:30:48Z

browser/ui/views/infobars/brave_confirm_infobar.cc

+      [this](ConfirmInfoBarDelegate::InfoBarButton type,
+             void (BraveConfirmInfoBar::*click_function)()) {
+        auto* button = AddChildView(std::make_unique<views::MdTextButton>(
+            base::BindRepeating(click_function, base::Unretained(this)),


I think we don't need weak ptr here because button is owned by BraveConfirmInforBar.
Passed |click_function| is valid always till |button| is gone.

simonhong · 2023-08-16T11:31:33Z

browser/ui/views/infobars/brave_confirm_infobar.cc

+    if (features::IsChromeRefresh2023()) {
+      extra_button_->SetStyle(ui::ButtonStyle::kTonal);
+    }


Forgot to mention. I think we don't need this as we'll not have different UI for this feature.

Resolves brave/brave-browser#32010

kdenhartog · 2023-08-16T19:37:05Z

I validated that the unretained usage was updated to a week pointer so I remove the needs-sec-review label

kdenhartog

no sec issues concerns popping up from what I'm seeing and the one action has been fixed. Approving for security

rmcfadden3

LGTM!

srirambv · 2023-08-21T07:09:43Z

Verification passed on

Brave	1.59.33 Chromium: 116.0.5845.96 (Official Build) nightly (64-bit)
Revision	`25347f9`
OS	Linux

Verified steps from PR description
Verified new promo info bar is shown for links that can be loaded via IPFS
Verified selecting Always doesn't show for other links opened after it
Verified selecting Only once continues to show the info bar every time the link is visited
Verified selecting No Thanks doesn't show the info bar any more
Verified selecting Always enables Automatically redirect requests for IPFS network resources to the configured gateway. setting

19588.mp4

Uplift of #19588 (squashed) to beta

github-actions bot assigned cypt4 Aug 7, 2023

cypt4 force-pushed the brave_32010 branch 4 times, most recently from ae618e8 to d3b30ea Compare August 8, 2023 11:49

cypt4 marked this pull request as ready for review August 8, 2023 12:06

cypt4 requested review from a team as code owners August 8, 2023 12:06

cypt4 requested review from simonhong and mkarolin August 8, 2023 12:06

github-actions bot reviewed Aug 8, 2023

View reviewed changes

github-actions bot added the needs-security-review label Aug 8, 2023

github-actions bot assigned goodov, iefremov and thypon Aug 8, 2023

mkarolin reviewed Aug 8, 2023

View reviewed changes

mkarolin approved these changes Aug 15, 2023

View reviewed changes

simonhong approved these changes Aug 16, 2023

View reviewed changes

simonhong reviewed Aug 16, 2023

View reviewed changes

Introduce IPFS promo infobar

99cb287

Resolves brave/brave-browser#32010

cypt4 force-pushed the brave_32010 branch from d3b30ea to 852a5ec Compare August 16, 2023 17:11

kdenhartog removed the needs-security-review label Aug 16, 2023

kdenhartog approved these changes Aug 16, 2023

View reviewed changes

cypt4 force-pushed the brave_32010 branch from 852a5ec to 4e4c995 Compare August 18, 2023 12:26

Review fix

90d842c

cypt4 force-pushed the brave_32010 branch from 4e4c995 to 90d842c Compare August 18, 2023 14:27

rmcfadden3 approved these changes Aug 18, 2023

View reviewed changes

cypt4 merged commit 32109b6 into master Aug 18, 2023

cypt4 deleted the brave_32010 branch August 18, 2023 17:47

github-actions bot added this to the 1.59.x - Nightly milestone Aug 18, 2023

brave-builds added a commit that referenced this pull request Aug 18, 2023

Uplift of #19588 (squashed) to beta

cd6512e

brave-builds mentioned this pull request Aug 18, 2023

Introduce IPFS promo infobar (uplift to 1.58.x) #19772

Merged

7 tasks

kjozwiak pushed a commit that referenced this pull request Aug 23, 2023

Introduce IPFS promo infobar (uplift to 1.58.x) (#19772)

08406b2

Uplift of #19588 (squashed) to beta

srirambv mentioned this pull request Sep 1, 2023

Autoredirect IPFS resources promo infobar brave/brave-browser#32010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IPFS promo infobar #19588

Introduce IPFS promo infobar #19588

cypt4 commented Aug 7, 2023 •

edited

Loading

github-actions bot Aug 8, 2023

cypt4 Aug 8, 2023

simonhong Aug 16, 2023

mkarolin Aug 8, 2023

rmcfadden3 Aug 8, 2023

lidel Aug 9, 2023 •

edited

Loading

autonome Aug 9, 2023

autonome Aug 9, 2023

cypt4 Aug 16, 2023

autonome Aug 17, 2023 •

edited

Loading

rmcfadden3 Aug 17, 2023 •

edited

Loading

rmcfadden3 Aug 17, 2023

autonome Aug 18, 2023

mkarolin left a comment

simonhong left a comment

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

cypt4 Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

simonhong Aug 16, 2023

kdenhartog commented Aug 16, 2023

kdenhartog left a comment

rmcfadden3 left a comment

srirambv commented Aug 21, 2023

	Would you like to handle content-addressed resources with native IPFS support (ipfs:// and ipns://) built into Brave?
	Would you like to upgrade IPFS resources like this and load them using the native support (ipfs:// and ipns://) built into Brave?

		BraveIPFSInfoBarDelegateObserver::BraveIPFSInfoBarDelegateObserver() {}

		BraveIPFSInfoBarDelegateObserver::~BraveIPFSInfoBarDelegateObserver() {}

	~BraveIPFSInfoBarDelegateObserverImpl() override {}
	~BraveIPFSInfoBarDelegateObserverImpl() override = default;

	BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() {}
	BraveIPFSInfoBarDelegate::~BraveIPFSInfoBarDelegate() = default;

		const int button_count =
		(ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0);

	const int button_count =
	(ok_button_ ? 1 : 0) + (cancel_button_ ? 1 : 0) + (extra_button_ ? 1 : 0);
	const int button_count = GetDelegate()->GetButtonsOrder().size();

Introduce IPFS promo infobar #19588

Introduce IPFS promo infobar #19588

Conversation

cypt4 commented Aug 7, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autonome Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

rmcfadden3 Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog commented Aug 16, 2023

kdenhartog left a comment

Choose a reason for hiding this comment

rmcfadden3 left a comment

Choose a reason for hiding this comment

srirambv commented Aug 21, 2023

cypt4 commented Aug 7, 2023 •

edited

Loading

lidel Aug 9, 2023 •

edited

Loading

autonome Aug 17, 2023 •

edited

Loading

rmcfadden3 Aug 17, 2023 •

edited

Loading