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

User keeps receiving Pinpoint push notifications after Amplify.Auth.signOut() #3179

Open
4 of 14 tasks
dJani97 opened this issue Jun 12, 2023 · 14 comments
Open
4 of 14 tasks
Assignees
Labels
Documentation Improvements or fixes to public documentation (docs.amplify.aws, pub.dev, readmes). feature-request A request for a new feature or an enhancement to an existing API or category. good first issue Good for newcomers push notifications

Comments

@dJani97
Copy link

dJani97 commented Jun 12, 2023

Description

We use Pinpoint to send push notifications, and to help with that we use the Amplify.Notifications.Push.identifyUser() method.

But it seems like after a user logs out, the endpoint associated with their current device does not get deleted and they keep receiving push notifications. This is not desired and could be a security concern.

I looked at the docs but couldn't find any information on how and when a pinpoint endpoint gets unregistered. Could you help me with that?

Expected behaviour

  • Amplify.Auth.signOut() is called
  • possibly another method is called, eg. Amplify.Notifications.Push.unassociateUser()
  • the user stops receiving push notifications on this particular device

Additional details

Here is how the client identifies the user with Pinpoint:

Future<void> registerUserWithPinpoint(Profile profile) async {
  final user = await Amplify.Auth.getCurrentUser();
  await Amplify.Notifications.Push.identifyUser(
    userId: user.userId,
    userProfile: AWSPinpointUserProfile(email: profile.email, name: profile.name),
  );
}

And here is how the server targets the client:

async function sendNotification(payload) {
  const sendNotificationInput = {
    ApplicationId: process.env.ANALYTICS_APP_NOTIFICATIONS_ID,
    SendUsersMessageRequest: {
      MessageConfiguration: {
        GCMMessage: {
          Title: payload.title,
          Body: payload.message,
          Data: { data: JSON.stringify(payload) },
          Priority: 'high',
        },
        APNSMessage: {
          Title: payload.title,
          Body: payload.message,
          Data: { data: JSON.stringify(payload) },
          Priority: '10',
        },
      },
      Users: {
        [payload.userId]: {},
      },
    },
  }
  return pinpointClient.send(
    new SendUsersMessagesCommand(sendNotificationInput),
  )
}

Categories

  • Analytics
  • API (REST)
  • API (GraphQL)
  • Auth
  • Authenticator
  • DataStore
  • Notifications (Push)
  • Storage

Steps to Reproduce

  • add plugins: AmplifyAuthCognito, AmplifyAPI, AmplifyAnalyticsPinpoint, AmplifyPushNotificationsPinpoint
  • configure amplify
  • sign in with Amplify.Auth.signIn()
  • associate the endpoint with a user: Amplify.Notifications.Push.identifyUser()
  • sign out with Amplify.Auth.signOut()
  • (backend): send a push notification for the user by userID

Screenshots

No response

Platforms

  • iOS
  • Android
  • Web
  • macOS
  • Windows
  • Linux

Flutter Version

3.10.4

Amplify Flutter Version

1.1.1

Deployment Method

Amplify CLI

Schema

No response

@dnys1 dnys1 added pending-triage This issue is in the backlog of issues to triage push notifications labels Jun 13, 2023
@dnys1 dnys1 self-assigned this Jun 15, 2023
@dnys1 dnys1 added Investigating and removed pending-triage This issue is in the backlog of issues to triage labels Jun 15, 2023
@dJani97
Copy link
Author

dJani97 commented Jul 3, 2023

@dnys1 is there any update on this? Can I assist you with more information?

@abdallahshaban557
Copy link
Contributor

Hi @dJani97 - It is recommended that after you can signOut, to then call the identifyUser API, and then pass the userID as an empty string. That will disassociate the endpoint from that user ID. We have not built in the wiring so that the signOut automatically handles dissociating the endpoint for your signed in user, we left that responsibility to the developer building out the features for their app.

@dJani97
Copy link
Author

dJani97 commented Jul 7, 2023

@abdallahshaban557 thank you for the info! I'm going to try this now:

await Amplify.Notifications.Push.identifyUser(
  userId: '',
  userProfile: AWSPinpointUserProfile(),
);

As a feedback, this is really obscure for me, and it's not documented. Since both userId and userProfile are required, I would have never thought that passing empty instances will result in the user being unregistered. Please update the docs, or add an unregisterUser() call that wraps this call, to make it more obvious for developers.

I would go as far as to suggest that whenever a user logs out, this unsubscription should be handled automatically, by default. Not doing so is a security risk that developers won't expect. Alternatively, you can warn developers about this in the docs.

@abdallahshaban557
Copy link
Contributor

Hi @dJani97 - you are right, we can definitely make that experience better. We will at least cover this in our documentation as a bare minimum, and investigate adding the wrapper API you mentioned. Thank you so much for the feedback!

@abdallahshaban557 abdallahshaban557 added feature-request A request for a new feature or an enhancement to an existing API or category. and removed Investigating labels Jul 11, 2023
@dnys1 dnys1 added Documentation Improvements or fixes to public documentation (docs.amplify.aws, pub.dev, readmes). good first issue Good for newcomers and removed feature-request A request for a new feature or an enhancement to an existing API or category. labels Jul 31, 2023
@abdallahshaban557
Copy link
Contributor

Hi @dJani97 - I have just received new information that the best way to make sure you de-associate a user with an endpoint is to pass null to the user_id, rather than passing an empty string. Just wanted to make sure I send this your way.

@kouz75
Copy link

kouz75 commented Sep 6, 2023

Hi @dJani97 - I have just received new information that the best way to make sure you de-associate a user with an endpoint is to pass null to the user_id, rather than passing an empty string. Just wanted to make sure I send this your way.

hi,
the user_id is a string and can't be null. setting an empty string works.

@dJani97
Copy link
Author

dJani97 commented Sep 6, 2023

Yes, it cannot be null. This is my current solution to this problem, and it is working:

await Amplify.Notifications.Push.identifyUser(
  userId: '',
  userProfile: AWSPinpointUserProfile(userAttributes: {}),
);

Is is a lot of boilerplate though, and it's very obscure. Notice how I must specify userAttributes: {} or else it will produce a runtime error.

@abdallahshaban557
Copy link
Contributor

We are currently working on improving the API to enable this behavior. We will provide an update here when we make more progress.

@dJani97 - just to confirm, setting the userId and userProfile as you did stopped the user from receiving new Notifications?

@dJani97
Copy link
Author

dJani97 commented Sep 6, 2023

@abdallahshaban557 yes.

@fjnoyp fjnoyp assigned fjnoyp and unassigned dnys1 Sep 6, 2023
@fjnoyp
Copy link
Contributor

fjnoyp commented Sep 9, 2023

Hey @dJani97 thanks for your insights here. Please note that if you save an attribute you must set its value to '' if you send notifications to a segment created with that attribute.

Ex:

identifyUser( userAttributes: { 'key' : 'value' } )

Pinpoint notifications to Segment with key : value will continue to send even if you called:
identifyUser( userAttributes: {})

Instead you must call:
identifyUser(userAttributes: {'key': ''})

That won't actually delete 'key' though.

We are aware of this behavior and have a separate issue for it here:
#3721

@victoravr
Copy link

How can I unsubscribe user from APNs push notifications from pinpoint upon sign-out with Amplify for react-native v6?

@Jordan-Nelson
Copy link
Member

@victoravr since this question appears to be about react native, can you open an issue here: https://github.com/aws-amplify/amplify-js? Thank you

@loe-lobo
Copy link

loe-lobo commented Jun 5, 2024

Hi @fjnoyp

Is there a better solution than clearing all the properties? Maybe updating the endpoint with OptOut = ALL?

Thanks

@NikaHsn
Copy link
Member

NikaHsn commented Jun 11, 2024

thanks @loe-lobo. there is an open feature request #3721 to improve this behaviour.

@Jordan-Nelson Jordan-Nelson added the feature-request A request for a new feature or an enhancement to an existing API or category. label Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or fixes to public documentation (docs.amplify.aws, pub.dev, readmes). feature-request A request for a new feature or an enhancement to an existing API or category. good first issue Good for newcomers push notifications
Projects
None yet
Development

No branches or pull requests

9 participants