Skip to content

Use new AuthenticatePusher NewDot Optimized API commands #9659

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

Merged
merged 4 commits into from
Jul 11, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libs/Middleware/Logging.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,11 @@ function Logging(response, request) {
// This error seems to only throw on dev when localhost:8080 tries to access the production web server. It's unclear whether this can happen on production or if
// it's a sign that the web server is down.
Log.hmmm('[Network] Error: Gateway Timeout error', {message: error.message, status: error.status});
} else if (request.command === 'Push_Authenticate') {
// Push_Authenticate requests can return with fetch errors and no message. It happens because we return a non 200 header like 403 Forbidden.
} else if (request.command === 'AuthenticatePusher') {
// AuthenticatePusher requests can return with fetch errors and no message. It happens because we return a non 200 header like 403 Forbidden.
// This is common to see if we are subscribing to a bad channel related to something the user shouldn't be able to access. There's no additional information
// we can get about these requests.
Log.hmmm('[Network] Error: Push_Authenticate', {message: error.message, status: error.status});
Log.hmmm('[Network] Error: AuthenticatePusher', {message: error.message, status: error.status});
} else if (error.message === CONST.ERROR.EXPENSIFY_SERVICE_INTERRUPTED) {
// Expensify site is down completely OR
// Auth (database connection) is down / bedrock has timed out while making a request. We currently can't tell the difference between Auth down and bedrock timing out.
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class AuthScreens extends React.Component {
Pusher.init({
appKey: CONFIG.PUSHER.APP_KEY,
cluster: CONFIG.PUSHER.CLUSTER,
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=Push_Authenticate`,
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=AuthenticatePusher`,
}).then(() => {
Report.subscribeToUserEvents();
User.subscribeToUserEvents();
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Pusher/pusher.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function init(args, params) {
// If we want to pass params in our requests to api.php we'll need to add it to socket.config.auth.params
// as per the documentation
// (https://pusher.com/docs/channels/using_channels/connection#channels-options-parameter).
// Any param mentioned here will show up in $_REQUEST when we call "Push_Authenticate". Params passed here need
// Any param mentioned here will show up in $_REQUEST when we call "AuthenticatePusher". Params passed here need
// to pass our inputRules to show up in the request.
if (params) {
socket.config.auth = {};
Expand Down
55 changes: 28 additions & 27 deletions src/libs/actions/Session/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import * as ValidationUtils from '../../ValidationUtils';
import * as Authentication from '../../Authentication';
import * as ErrorUtils from '../../ErrorUtils';
import * as Welcome from '../Welcome';
import * as API from '../../API';

let credentials = {};
Onyx.connect({
Expand Down Expand Up @@ -434,7 +435,7 @@ function validateEmail(accountID, validateCode) {
// subscribe to a bunch of channels at once we will only reauthenticate and force reconnect Pusher once.
const reauthenticatePusher = _.throttle(() => {
Log.info('[Pusher] Re-authenticating and then reconnecting');
Authentication.reauthenticate('Push_Authenticate')
Authentication.reauthenticate('AuthenticatePusher')
.then(Pusher.reconnect)
.catch(() => {
console.debug(
Expand All @@ -452,39 +453,39 @@ const reauthenticatePusher = _.throttle(() => {
function authenticatePusher(socketID, channelName, callback) {
Log.info('[PusherAuthorizer] Attempting to authorize Pusher', false, {channelName});

DeprecatedAPI.Push_Authenticate({
// We use makeRequestWithSideEffects here because we need to authorize to Pusher (an external service) each time a user connects to any channel.
// eslint-disable-next-line rulesdir/no-api-side-effects-method
API.makeRequestWithSideEffects('AuthenticatePusher', {
socket_id: socketID,
channel_name: channelName,
shouldRetry: false,
forceNetworkRequest: true,
})
.then((response) => {
if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) {
Log.hmmm('[PusherAuthorizer] Unable to authenticate Pusher because authToken is expired');
callback(new Error('Pusher failed to authenticate because authToken is expired'), {auth: ''});
}).then((response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nonblocking fussy observation: if we just leave this indent alone the diff will leave less of an impact

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the new way is better for style but yeah

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer .then() on a new line, but it is a personal preference and not something we have in the style guide.

if (response.jsonCode === CONST.JSON_CODE.NOT_AUTHENTICATED) {
Log.hmmm('[PusherAuthorizer] Unable to authenticate Pusher because authToken is expired');
callback(new Error('Pusher failed to authenticate because authToken is expired'), {auth: ''});

// Attempt to refresh the authToken then reconnect to Pusher
reauthenticatePusher();
return;
}
// Attempt to refresh the authToken then reconnect to Pusher
reauthenticatePusher();
return;
}

if (response.jsonCode !== CONST.JSON_CODE.SUCCESS) {
Log.hmmm('[PusherAuthorizer] Unable to authenticate Pusher for reason other than expired session');
callback(new Error(`Pusher failed to authenticate because code: ${response.jsonCode} message: ${response.message}`), {auth: ''});
return;
}
if (response.jsonCode !== CONST.JSON_CODE.SUCCESS) {
Log.hmmm('[PusherAuthorizer] Unable to authenticate Pusher for reason other than expired session');
callback(new Error(`Pusher failed to authenticate because code: ${response.jsonCode} message: ${response.message}`), {auth: ''});
return;
}

Log.info(
'[PusherAuthorizer] Pusher authenticated successfully',
false,
{channelName},
);
callback(null, response);
})
.catch((error) => {
Log.hmmm('[PusherAuthorizer] Unhandled error: ', {channelName, error});
callback(new Error('Push_Authenticate request failed'), {auth: ''});
});
Log.info(
'[PusherAuthorizer] Pusher authenticated successfully',
false,
{channelName},
);
callback(null, response);
}).catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Change was not really needed.

Log.hmmm('[PusherAuthorizer] Unhandled error: ', {channelName, error});
callback(new Error('AuthenticatePusher request failed'), {auth: ''});
});
}

/**
Expand Down
14 changes: 0 additions & 14 deletions src/libs/deprecatedAPI.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,19 +278,6 @@ function PreferredLocale_Update(parameters) {
return Network.post(commandName, parameters);
}

/**
* @param {Object} parameters
* @param {String} parameters.socket_id
* @param {String} parameters.channel_name
* @returns {Promise}
*/
function Push_Authenticate(parameters) {
const commandName = 'Push_Authenticate';
requireParameters(['socket_id', 'channel_name'],
parameters, commandName);
return Network.post(commandName, parameters);
}

/**
* @param {Object} parameters
* @param {Number} parameters.reportID
Expand Down Expand Up @@ -949,7 +936,6 @@ export {
PersonalDetails_Update,
Plaid_GetLinkToken,
Policy_Employees_Merge,
Push_Authenticate,
RejectTransaction,
Report_AddComment,
Report_GetHistory,
Expand Down
2 changes: 1 addition & 1 deletion tests/actions/ReportTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('actions/Report', () => {
Pusher.init({
appKey: CONFIG.PUSHER.APP_KEY,
cluster: CONFIG.PUSHER.CLUSTER,
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=Push_Authenticate`,
authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=AuthenticatePusher`,
});

Onyx.init({
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/NetworkTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ test('Sequential queue will succeed if triggered while reauthentication via main
})
.then(() => {
// When we queue both non-persistable and persistable commands that will trigger reauthentication and go offline at the same time
Network.post('Push_Authenticate', {content: 'value1'});
Network.post('AuthenticatePusher', {content: 'value1'});
Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
expect(NetworkStore.isOffline()).toBe(false);
expect(NetworkStore.isAuthenticating()).toBe(false);
Expand Down Expand Up @@ -678,9 +678,9 @@ test('Sequential queue will succeed if triggered while reauthentication via main
// We are not offline anymore
expect(NetworkStore.isOffline()).toBe(false);

// First call to xhr is the Push_Authenticate request that could not call Authenticate because we went offline
// First call to xhr is the AuthenticatePusher request that could not call Authenticate because we went offline
const [firstCommand] = xhr.mock.calls[0];
expect(firstCommand).toBe('Push_Authenticate');
expect(firstCommand).toBe('AuthenticatePusher');

// Second call to xhr is the MockCommand that also failed with a 407
const [secondCommand] = xhr.mock.calls[1];
Expand Down