Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • Sign in / Register
Minds Backend - Engine
Minds Backend - Engine
  • Project overview
  • Repository
  • Issues 263
  • Merge Requests 27
  • CI / CD
  • Security & Compliance
  • Packages
  • Wiki
  • Snippets
  • Members
  • Collapse sidebar
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • Minds
  • Minds Backend - EngineMinds Backend - Engine
  • Merge Requests
  • !436

Open
Opened 4 days ago by Ben Hayward@benhayward.ben
Report abuse

Surge token deletion on logout #1270

  • Overview 12
  • Commits 2
  • Pipelines 2
  • Changes 6
2/7 threads resolved

Summary

Closes #1270

Currently on the mobile app if you log into one account, then log out and into a different account, you will still receive push notifications for the first account.

This is because the Surge tokens (used for push notifications) that we use on the backend are not deleted on logout.

Steps

To hammer out with @brianhatchet and @msantang78. This is going to likely be tricky to test as we do not digest push notifications on the front-end, just the mobile app. This may require us using a manual build of the app and plugging it into the associated sandbox.

I would recommend testing sessions and account swapping/triggering notifications.

Regression Scope

Changes v Impact

  • api/v1/notifications - Simple change, setting the token on notif POST (from mobile).
  • Session\Manager - limited to the destroy functionality, so would affect anything that destroys a session (e.g. logging out).
  • Entities\User - added functions and an exported value, I don't foresee any issues here.
Request to merge fix/logout-surge-token-deletion-1270 into master
The source branch is 6 commits behind the target branch
Open in Web IDE
Pipeline #108524651 passed for 2b31256f on fix/logout-surge-token-deletion-1270
          Requires 3 more approvals from Devs, Deployers, and QA.
          Rami Albatal
          Rami Albatal
          Marcelo Rivera
          Marcelo Rivera
          Guy Thouret
          Guy Thouret
          Olivia Madrid
          Olivia Madrid
          Emiliano Balbuena
          Emiliano Balbuena
          Ready to be merged automatically. Ask someone with write access to this repository to merge this request

          Deletes source branch

          • Ben Hayward @benhayward.ben added Squad::Yellow Status::Blocked scoped labels 4 days ago

            added scoped labels

          • Ben Hayward
            Ben Hayward @benhayward.ben · 4 days ago
            Developer

            Set to blocked pending chat with Brian and Martin.

          • Ben Hayward @benhayward.ben assigned to @brianhatchet, @msantang78, and @benhayward.ben 4 days ago

            assigned to @brianhatchet, @msantang78, and @benhayward.ben

          • Brian Hatchet
            Brian Hatchet :speech_balloon: @brianhatchet · 4 days ago
            Developer

            Testing on the app requires two things that we don't have.

            1. An ability to change the base url in the app for testing purposes.
            2. The ability to push notifications from the review sites.

            So, let's add some logging to show the push notifications going out so we can verify the body of the notification. Once we're sure the logging works, we'll verify it on staging

          • Brian Hatchet :speech_balloon: @brianhatchet added Status::BuddyReview scoped label and automatically removed Status::Blocked label 4 days ago

            added scoped label and automatically removed label

          • Ben Hayward
            Ben Hayward @benhayward.ben started a thread on the diff 3 days ago
            Controllers/api/v1/notifications.php
            146 146 'service' => $service,
            147 147 'token' => $passed_token
            148 148 ]);
            149
            150 Core\Session::getLoggedInUser()
            151 ->setSurgeToken($token)
            152 ->save();
            • Ben Hayward
              Ben Hayward @benhayward.ben · 3 days ago
              Developer

              We need to dump notification JSON do it in notifications file, but behind developer mode or feature flag.

            • Please register or sign in to reply
          • Emiliano Balbuena
            Emiliano Balbuena @edgebal started a thread on an old version of the diff 2 days ago
            Resolved by Ben Hayward 2 days ago
          • Emiliano Balbuena
            Emiliano Balbuena @edgebal started a thread on an old version of the diff 2 days ago
            Resolved by Ben Hayward 2 days ago
          • Emiliano Balbuena
            Emiliano Balbuena @edgebal · 2 days ago
            Developer

            Added a couple of feedback items, @benhayward.ben

          • Emiliano Balbuena @edgebal added Status::Requires Changes scoped label and automatically removed Status::BuddyReview label 2 days ago

            added scoped label and automatically removed label

          • Ben Hayward @benhayward.ben added 1 commit 2 days ago

            added 1 commit

            • 2b31256f - Updated for feedback

            Compare with previous version

          • Ben Hayward @benhayward.ben resolved all threads 2 days ago

            resolved all threads

          • Mark Harding
            Mark Harding @markeharding started a thread on the diff 6 hours ago
            Controllers/api/v1/notifications.php
            146 146 'service' => $service,
            147 147 'token' => $passed_token
            148 148 ]);
            149
            150 Core\Session::getLoggedInUser()
            • Mark Harding
              Mark Harding @markeharding · 6 hours ago
              Owner

              Not sure why we need to make this change but if we are then Core\Entities\Actions\Save should be used.

            • Please register or sign in to reply
          • Mark Harding
            Mark Harding @markeharding started a thread on the diff 6 hours ago
            Last updated by Ben Hayward 1 hour ago
            Core/Notification/Extensions/Push.php
            106 106 $push['big_picture'] = static::getNotificationBigPicture($notification, $from_user, $entity);
            107 107 $push['group'] = static::getNotificaitonGroup($notification, $from_user, $entity);
            108 108
            109 // temporary logging
            110 error_log(var_export($push, true));
            • Mark Harding
              Mark Harding @markeharding · 6 hours ago
              Owner

              We shouldn't be putting this on production

            • Ben Hayward
              Ben Hayward @benhayward.ben · 1 hour ago
              Developer

              Definitely not, It's only there to test something on sandboxes, I'll leave this unresolved until its gone so that it doesn't get missed.

            • Please register or sign in to reply
          • Mark Harding
            Mark Harding @markeharding started a thread on the diff 6 hours ago
            Core/Sessions/Manager.php
            246 247 public function destroy($all = false)
            247 248 {
            248 249 if ($this->session) {
            249 $this->repository->delete($this->session, $all);
            250 if (!$this->user) {
            251 // If no user set grab from session.
            252 $this->user = new User($this->session->getLoggedInUser(), false);
            • Mark Harding
              Mark Harding @markeharding · 6 hours ago
              Owner

              Dangerous building of user entity. EntitiesBuilder->single should be used

            • Please register or sign in to reply
          • Mark Harding
            Mark Harding @markeharding started a thread on the diff 6 hours ago
            Core/Sessions/Manager.php
            246 247 public function destroy($all = false)
            247 248 {
            248 249 if ($this->session) {
            249 $this->repository->delete($this->session, $all);
            250 if (!$this->user) {
            251 // If no user set grab from session.
            252 $this->user = new User($this->session->getLoggedInUser(), false);
            253 }
            254
            255 $this->user->setSurgeToken('') // Delete push notification token.
            • Mark Harding
              Mark Harding @markeharding · 6 hours ago
              Owner

              Why are you deleting surge tokens when a session is deleted? Mobile apps use OAuth and not sessions. If I log out on desktop, I'm going to stop receiving notifications everywhere.

            • Please register or sign in to reply
          Please register or sign in to reply
          3 Assignees
          Brian Hatchet's avatar
          Martin Santangelo's avatar
          Ben Hayward's avatar
          None
          Milestone
          None
          Time tracking
          No estimate or time spent
          2
          Labels
          Squad::Yellow Status::Requires Changes
          Lock merge request
          Unlocked
          10
          10 participants
          user avatar
          Rami Albatal
          user avatar
          Marcelo Rivera
          user avatar
          Guy Thouret
          user avatar
          Olivia Madrid
          user avatar
          Emiliano Balbuena
          user avatar
          Martin Santangelo
          user avatar
          Brian Hatchet
          Reference: minds/engine!436

          Now you can access the merge request navigation tabs at the top, where they’re easier to find.

          More information and share feedback