Skip to content

Conversation

@Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Jun 12, 2024

Explanation

This adds some core controllers for the notification services that are consumed in both mobile and extension. (Maintained by the notifications team).

Controllers are:

  • ProfileSyncController - which contains 2 controllers (AuthenticationController and UserStorageController)

    • These are used for adding or consuming authenticated endpoints and also make use of the profile syncing feature across platforms and devices.
  • NotificationServicesController - which also contains 2 controllers (NotificationServicesController and NotificationsServicesPushController)

    • Both of these controllers are tightly coupled. The first manages pull based notifications and creation of resource; and the second is responsible for push notifications.

NOTE - these controllers should be treated as V0.x.x, as they are under development on extension and mobile (iron out any issues).

Previous PR: #4320
(closed this as it was getting to unruly to work in.)

References

Here is a loom recording for the new controllers.
https://www.loom.com/share/4e95a8fcc2ae4d81b737265fc75571c5?sid=8247a832-3f2e-4837-ab74-30c45524ccfe

Future Improvements (as separate PRs)

  1. Lets tidy up and improve Push Notifications for Mobile (after dog-fooding) - the implementation may differ on mobile.
  2. I also want to improve imports so we can do paths (e.g. @metamask/profile-sync-controller/sdk) instead of the global named exports have.
  3. Decoupling this from extension has shown some annoying messaging system actions back and forth (leading to some circular dependencies due to importing types). This has been resolved for now, but I want our team to rethink how we are using the messaging system to be more streamlined.
  • I don't want UserStorage controller calling actions to Notifications (we should not do this)
  • I don't really like how tied the notifications and push notifications do communication back and forth. I would rather we orchestrate communication in 1 controller (so communication is 1 way).

Changelog

@metamask/profile-sync-controller

  • ADDED: new AuthenticationController
  • ADDED: new UserStorageController

@metamask/notfication-services-controller

  • ADDED: new NotificationServicesController
  • ADDED: new NotificationServicesPushController

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

JSoufer and others added 30 commits May 24, 2024 18:04
note - we need to ensure types and linting are passing
…:MetaMask/core into feat/integrate-notifications-controllers
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
…chema.ts

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
…chema.ts

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Jonathansoufer and others added 15 commits June 7, 2024 13:51
reorganise logic and controllers into separate folders. Will figure out module exports in a little bit
fix circular deps, tests, types, linting
we pass feature announcement env through the controller config now
fix types, imports, tests, builds, circular deps, and decouple as much as I can from extension.
some stuff was using the old controller name. There are some places that use the older name, but might be a hassle to update
these 2 controllers are heavily tied together and should belong under the same package
for some reason the yarn.lock file got mangled. This mostly reverts it & ensures CI passes
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team June 12, 2024 19:54
@socket-security
Copy link

socket-security bot commented Jun 12, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@contentful/content-source-maps@0.5.0 None +3 252 kB contentful-ecosystem
npm/@contentful/rich-text-html-renderer@16.5.2 None +1 150 kB contentful-ecosystem
npm/@contentful/rich-text-types@16.5.2 None 0 146 kB contentful-ecosystem
npm/@fastify/busboy@2.1.1 None 0 80.2 kB gurgunday
npm/@firebase/analytics-compat@0.2.10 Transitive: environment, network +7 2.77 MB google-wombot
npm/@firebase/app-check-compat@0.3.11 Transitive: environment, network +5 2.13 MB google-wombot
npm/@firebase/app-check-interop-types@0.3.2 None 0 2.9 kB google-wombot
npm/@firebase/app-compat@0.2.35 Transitive: environment +3 1.66 MB google-wombot
npm/@firebase/app-types@0.9.2 None 0 9.56 kB google-wombot
npm/@firebase/app@0.10.5 Transitive: environment +4 2.03 MB google-wombot
npm/@firebase/auth-compat@0.5.9 Transitive: environment, network, unsafe +6 22.8 MB google-wombot
npm/@firebase/auth-interop-types@0.2.3 None 0 1.97 kB google-wombot
npm/@firebase/database-compat@1.0.5 Transitive: environment, network +9 14.8 MB google-wombot
npm/@firebase/firestore-compat@0.3.32 Transitive: environment, filesystem, network, unsafe +21 39.3 MB google-wombot
npm/@firebase/functions-compat@0.3.11 Transitive: environment, network, unsafe +6 3 MB google-wombot
npm/@firebase/installations-compat@0.2.7 Transitive: environment, network +5 1.95 MB google-wombot
npm/@firebase/messaging-compat@0.2.9 Transitive: environment, network +6 2.98 MB google-wombot
npm/@firebase/performance-compat@0.2.7 Transitive: environment, network +7 2.66 MB google-wombot
npm/@firebase/remote-config-compat@0.2.7 Transitive: environment, network +7 2.59 MB google-wombot
npm/@firebase/storage-compat@0.3.8 Transitive: environment, network, unsafe +5 5.32 MB google-wombot
npm/@firebase/vertexai-preview@0.0.2 network Transitive: environment +3 2.22 MB google-wombot
npm/@metamask/notification-services-controller@0.0.0-use.local None 0 0 B
npm/@protobufjs/aspromise@1.1.2 None 0 9.05 kB dcode
npm/axios@1.6.8 network Transitive: environment, filesystem +8 2.22 MB jasonsaayman
npm/bignumber.js@4.1.0 None 0 392 kB mikemcl
npm/contentful-resolve-response@1.8.1 None +1 139 kB whydah-gally
npm/contentful-sdk-core@8.1.4 None +5 451 kB contentful-ecosystem
npm/contentful@10.11.11 None 0 0 B
npm/firebase@10.12.2 Transitive: environment, filesystem, network, unsafe +36 99.5 MB google-wombot
npm/loglevel@1.9.1 None 0 639 kB pimterry
npm/tslib@2.6.3 None 0 84.9 kB typescript-bot

🚮 Removed packages: npm/tslib@2.6.2

View full report↗︎

@socket-security
Copy link

socket-security bot commented Jun 12, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSource
Network access npm/@protobufjs/fetch@1.1.0
Network access npm/faye-websocket@0.11.4
Network access npm/faye-websocket@0.11.4
Network access npm/websocket-driver@0.7.4
New author npm/@fastify/busboy@2.1.1
Network access npm/follow-redirects@1.15.6
Network access npm/follow-redirects@1.15.6
Network access npm/axios@1.6.8
Network access npm/axios@1.6.8
Network access npm/undici@5.28.4
Network access npm/undici@5.28.4
Network access npm/undici@5.28.4
Network access npm/undici@5.28.4
Network access npm/undici@5.28.4
Network access npm/@firebase/app-check@0.8.4
Network access npm/@firebase/functions@0.11.5
Network access npm/@firebase/installations@0.6.7
Network access npm/@firebase/messaging@0.12.9
Network access npm/@firebase/performance@0.6.7
Network access npm/@firebase/remote-config@0.4.7
Network access npm/@firebase/analytics@0.10.4
Network access npm/@firebase/auth@1.7.4
Network access npm/@firebase/auth@1.7.4
Network access npm/@firebase/vertexai-preview@0.0.2
Network access npm/@grpc/grpc-js@1.9.15
Network access npm/@grpc/grpc-js@1.9.15
Network access npm/@grpc/grpc-js@1.9.15
Network access npm/@grpc/grpc-js@1.9.15
Network access npm/@grpc/grpc-js@1.9.15

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

What is new author?

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/@protobufjs/fetch@1.1.0
  • @SocketSecurity ignore npm/faye-websocket@0.11.4
  • @SocketSecurity ignore npm/websocket-driver@0.7.4
  • @SocketSecurity ignore npm/@fastify/busboy@2.1.1
  • @SocketSecurity ignore npm/follow-redirects@1.15.6
  • @SocketSecurity ignore npm/axios@1.6.8
  • @SocketSecurity ignore npm/undici@5.28.4
  • @SocketSecurity ignore npm/@firebase/app-check@0.8.4
  • @SocketSecurity ignore npm/@firebase/functions@0.11.5
  • @SocketSecurity ignore npm/@firebase/installations@0.6.7
  • @SocketSecurity ignore npm/@firebase/messaging@0.12.9
  • @SocketSecurity ignore npm/@firebase/performance@0.6.7
  • @SocketSecurity ignore npm/@firebase/remote-config@0.4.7
  • @SocketSecurity ignore npm/@firebase/analytics@0.10.4
  • @SocketSecurity ignore npm/@firebase/auth@1.7.4
  • @SocketSecurity ignore npm/@firebase/vertexai-preview@0.0.2
  • @SocketSecurity ignore npm/@grpc/grpc-js@1.9.15

matteoscurati
matteoscurati previously approved these changes Jun 13, 2024
Copy link
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

Jonathansoufer
Jonathansoufer previously approved these changes Jun 13, 2024
Copy link
Contributor

@Jonathansoufer Jonathansoufer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

this allows mobile and extension to fetch different notifications
Copy link
Contributor

@Jonathansoufer Jonathansoufer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Prithpal-Sooriya Prithpal-Sooriya merged commit dcc1d92 into main Jun 13, 2024
@Prithpal-Sooriya Prithpal-Sooriya deleted the feat/integrate-notifications-controllers branch June 13, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants