Open julian-dotcom opened 4 months ago
jimblanc
commented
4 months ago @julian-dotcom Thank you for your report! It seems like the application is having trouble when users are unauthenticated. Could you confirm that you've enabled unauthenticated access when setting up your backend? Reference: https://docs.amplify.aws/gen1/react-native/build-a-backend/push-notifications/set-up-push-notifications/#set-up-backend-resources
julian-dotcom
commented
4 months ago We only want to send notifications to authenticated users. We never want unauthenticated users to receive notifications.
But as far as I understand initializePushNotifications() has to be called in the index.js.
How do I prevent it from registering the device for push notifications until the user logs in?
jimblanc
commented
4 months ago @julian-dotcom Unfortunately this is a use-case that we don't directly support at the moment. However, there is a possible work around you can try: Using identifyUser to mark or unmark a particular device, for example setting optOut: 'ALL' until the user is signed in and resetting it when they sign out.
julian-dotcom
commented
4 months ago This doesn't work. When calling identifyUser like so:
import * as PushNotification from 'aws-amplify/push-notifications';
...
await PushNotification.identifyUser({
userId: '',
userProfile: {},
options: { address: deviceToken.token, optOut: 'ALL' },
});identifyUser never finishes executing.
This is because const { credentials, identityId } = await resolveCredentials(); fails in line 28 of node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/identifyUser.native.ts
julian-dotcom
commented
4 months ago Please see this ticket: 13504
Samaritan1011001
commented
4 months ago @julian-dotcom There are two options here,
initializePushNotifications when called since endpoint creation needs them. You can immediately call identifyUser with optOut as "ALL" as Jim mentioned so that the guest user does not receive any push notifications pushed through Pinpoint's campaigns.
Note: this will not restrict Firebase from sending notifications to the device. When the user authenticates, you may call identifyUser again with optOut as "None".
identifyUser({
userId: 'user-id-01',
userProfile: {name: 'user-test-name'},
options: {optOut: 'ALL'},
})initializePushNotifications only in your authenticated route on the app. This API basically initializes all the listeners needed for the library in addition to registering an endpoint (with credentials & auto generated endpointId for the "Push" channel). It is recommended to call as early as possible so as to not miss listening to some events like notification coming in when the app is in background. So, if your authenticated route in your app is the first page to be rendered, calling this API there will also work for your use-case. In this scenario, you would not need to call identifyUser since you are initializing only in the authenticated route of your app. julian-dotcom
commented
3 months ago I created a patch, because in my estimation this is a bug.
As I commented somewhere else:
I investigated some more and figured out, what goes wrong. To me, it looks like a bug.
When calling initializePushNotifications() in my index.js, the function calls addNativeListeners(), which calls addTokenEventListener(), which calls registerDevice().
The problem is that the register device function fails until a user is authenticated. This is fine and makes sense.
However, the issue is that the exception is not passed on to initializePushNotifications(). This function continues and calls initialize(), setting initialized to true.
This incorrect behavior because the device was never successfully registered, but the code now thinks it's initialized.
Hence, whenever calling initializePushNotifications() after, the code just exits the function early and nothing happens.
In my estimation, this should not happen. We should set initialized to true, ONLY if the device was successfully registered.
export const initializePushNotifications = (): void => {
if (isInitialized()) {
logger.info('Push notifications have already been enabled');
console.log('Push notifications already initialized')
return;
}
addNativeListeners();
addAnalyticsListeners();
initialize();
};Source: node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
enzzoperez
commented
3 months ago I created a patch, because in my estimation this is a bug.
As I commented somewhere else:
I investigated some more and figured out, what goes wrong. To me, it looks like a bug.
When calling initializePushNotifications() in my index.js, the function calls addNativeListeners(), which calls addTokenEventListener(), which calls registerDevice().
The problem is that the register device function fails until a user is authenticated. This is fine and makes sense.
However, the issue is that the exception is not passed on to initializePushNotifications(). This function continues and calls initialize(), setting initialized to true.
This incorrect behavior because the device was never successfully registered, but the code now thinks it's initialized.
Hence, whenever calling initializePushNotifications() after, the code just exits the function early and nothing happens.
In my estimation, this should not happen. We should set initialized to true, ONLY if the device was successfully registered.
export const initializePushNotifications = (): void => { if (isInitialized()) { logger.info('Push notifications have already been enabled'); console.log('Push notifications already initialized') return; } addNativeListeners(); addAnalyticsListeners(); initialize(); };Source:
node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
Hi julian, what is the patch that you created?, I have the same error, I tried with workarounds that passed here but no success 😬
eidson-encora
commented
3 months ago I'm also facing this issue and had some problems patching it. :cry:
I have "aws-amplify": "^6.3.8" on my package.json, but the patch is on folder @aws-amplify so I got No such package @aws-amplify when running patch-package
julian-dotcom
commented
3 months ago I created a patch for this file:
node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
I modify the initializePushNotifications function like so:
let initializedPushNotification = false;
const initializePN = async () => initializedPushNotification = true;
const isInitializedPN = () => initializedPushNotification;
export const initializePushNotifications = (): void => {
if (isInitialized()) {
logger.info('Push notifications have already been enabled');
!isInitializedPN() && registerDevice(getToken());
return;
}
addNativeListeners();
addAnalyticsListeners();
initialize();
};And I add initializePN(); in registerDevice()
const registerDevice = async (address: string): Promise<void> => {
const { credentials, identityId } = await resolveCredentials();
const { appId, region } = resolveConfig();
try {
await updateEndpoint({
address,
appId,
category: 'PushNotification',
credentials,
region,
channelType: getChannelType(),
identityId,
userAgentValue: getPushNotificationUserAgentString(
PushNotificationAction.InitializePushNotifications,
),
});
initializePN();
// always resolve inflight device registration promise here even though the promise is only awaited on by
// `identifyUser` when no endpoint is found in the cache
resolveInflightDeviceRegistration();
} catch (underlyingError) {
rejectInflightDeviceRegistration(underlyingError);
throw underlyingError;
}
};This is the actual patch file @aws-amplify+notifications+2.0.33.patch:
diff --git a/node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts b/node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
index 2582ada..88ae7b6 100644
--- a/node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
+++ b/node_modules/@aws-amplify/notifications/src/pushNotifications/providers/pinpoint/apis/initializePushNotifications.native.ts
@@ -40,10 +40,15 @@ const logger = new ConsoleLogger('Notifications.PushNotification');
const BACKGROUND_TASK_TIMEOUT = 25; // seconds
+let initializedPushNotification = false;
+const initializePN = async () => initializedPushNotification = true;
+const isInitializedPN = () => initializedPushNotification;
+
+
export const initializePushNotifications = (): void => {
if (isInitialized()) {
logger.info('Push notifications have already been enabled');
-
+ !isInitializedPN() && registerDevice(getToken());
return;
}
addNativeListeners();
@@ -159,7 +164,10 @@ const addNativeListeners = (): void => {
try {
await registerDevice(token);
} catch (err) {
- logger.error('Failed to register device for push notifications', err);
+ if (err.name !== 'NoCredentials') {
+ logger.error('Failed to register device for push notifications', err);
+ }
+
throw err;
}
},
@@ -218,6 +226,7 @@ const registerDevice = async (address: string): Promise<void> => {
PushNotificationAction.InitializePushNotifications,
),
});
+ initializePN();
// always resolve inflight device registration promise here even though the promise is only awaited on by
// `identifyUser` when no endpoint is found in the cache
resolveInflightDeviceRegistration();thanks so much @julian-dotcom , it worked
guo-pf
commented
3 months ago I have "aws-amplify": "^6.3.6" on my package.json. you can try:
import {fetchAuthSession} from 'aws-amplify/auth';
import {updateEndpoint} from '@aws-amplify/core/internals/providers/pinpoint';
import {
Category,
PushNotificationAction,
getAmplifyUserAgent,
} from '@aws-amplify/core/internals/utils';
const getPushNotificationUserAgentString = (action: PushNotificationAction) =>
getAmplifyUserAgent({
category: Category.PushNotification,
action,
});
export const registerEndPoint = async (
appId: string,
region: string,
userId: string,
fcmToken: string,
) => {
return new Promise<boolean>(async (resolve, reject) => {
fetchAuthSession()
.then(async session => {
if (session.credentials) {
updateEndpoint({
address: fcmToken,
appId: appId,
category: 'PushNotification',
channelType: 'GCM',
credentials: session.credentials,
identityId: session.identityId,
optOut: 'NONE',
region: region,
userId: userId,
userAgentValue: getPushNotificationUserAgentString(
PushNotificationAction.InitializePushNotifications,
),
})
.then(async () => {
resolve(true);
})
.catch(async err => {
reject(err);
});
} else {
reject(undefined);
}
})
.catch(async err => {
reject(err);
});
});
};@julian-dotcom
keithdmoore
commented
2 months ago @julian-dotcom @cwomack Is there a PR for this?
keithdmoore
commented
2 months ago @julian-dotcom @cwomack I also have the same issue. The patch does fix this issue. However, for some reason, I am not getting device token so the OptOut is set to "ALL". I also noticed that the token in the addTokenEventListener is undefined as well.
keithdmoore
commented
2 months ago I think this issue would be resolved if https://github.com/aws-amplify/amplify-js/issues/13277 was resolved.
cwomack
commented
2 months ago @julian-dotcom, want to confirm something on how you're implementing the patch recommended in this comment above. Are you calling initializePushNotifications() twice? Once at the top level of your code (after Amplify.configure()) and again in the code you provided in the comment?
Currently, our documentation for implementing and initializing push notifications implies that the authenticated & unauthenticated with guest access enabled are supported. So this use case of unauthenticated users with guest access disabled for the identityPool is a bit of an edge case.
georgeplaton7
commented
1 month ago When will the patch be merged and released?
cwomack
commented
3 weeks ago @georgeplaton7, we don't have a PR for this specifically yet I believe. The workaround/patch that's been mentioned was in @julian-dotcom's comment above (here) that others have reported working for them.
We'd welcome anyone to submit a PR tied to this if they're willing, but since this is a bit of an edge case at this point for the use case of unauthenticated users with guest access being disabled for the identityPool.
Before opening, please confirm:
JavaScript Framework
React Native
Amplify APIs
Authentication, Analytics, Push Notifications
Amplify Version
v6
Amplify Categories
auth, analytics, notifications
Backend
Other
Environment information
Describe the bug
When I launch my React Native app in an unauthenticated state, I get the following error:
[ERROR] 57:31.569 Notifications.PushNotification - Failed to register device for push notifications [NoCredentials: Credentials should not be empty.]This happens when I call
initializePushNotifications()in my index.js.When a user is authenticated and I launch the app, the error does NOT show up.
The error also doesn't show up if
initializePushNotifications()is commented out.Expected behavior
Get no error when launching the app in its unauthenticated state.
Reproduction steps
npx react-native run-androidCode Snippet
This is the code for my index.js:
Log output
aws-exports.js
Manual configuration
No response
Additional configuration
No response
Mobile Device
No response
Mobile Operating System
No response
Mobile Browser
No response
Mobile Browser Version
No response
Additional information and screenshots
No response