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.

Sign up for GitHub

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

Jump to bottom

fix: discourage authToken usage and link to updated docs #325

Merged
merged 5 commits into from
Mar 15, 2023
Merged

fix: discourage authToken usage and link to updated docs #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6630cdd
fix: discourage `authToken` usage and link to updated docs
byCedric Mar 15, 2023
0c24e22
chore: add changelog entry
byCedric Mar 15, 2023
4f3bc60
refactor: update the auth.token missing value comment in sentry.prope…
byCedric Mar 15, 2023
e1ce56a
fix: clean up authToken warnings
byCedric Mar 15, 2023
bc69f0d
chore: rebuild plugin
byCedric Mar 15, 2023
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

### 🐛 Bug fixes

- Discourage configuring `authToken` and point to `SENTRY_AUTH_TOKEN` and docs. ([#325](https://github.com/expo/sentry-expo/pull/325) by [@byCedric](https://github.com/byCedric))

### 🧹 Chores

## [6.1.0](https://github.com/expo/sentry-expo/releases/tag/v6.1.0) - 2023-03-04
Expand Down
15 changes: 7 additions & 8 deletions plugin/build/withSentry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const withSentry = (config) => {
}
return config;
};
const missingAuthTokenMessage = `# no auth.token found, falling back to SENTRY_AUTH_TOKEN environment variable`;
const missingAuthTokenMessage = `# auth.token is configured through SENTRY_AUTH_TOKEN environment variable`;
const missingProjectMessage = `# no project found, falling back to SENTRY_PROJECT environment variable`;
const missingOrgMessage = `# no org found, falling back to SENTRY_ORG environment variable`;
function getSentryProperties(config) {
Expand All @@ -39,17 +39,16 @@ function getSentryProperties(config) {
config_plugins_1.WarningAggregator.addWarningIOS('sentry-expo', 'No Sentry config found in app.json, builds will fall back to environment variables. Refer to @sentry/react-native docs for how to configure this.');
return '';
}
if (sentryHook.config?.authToken) {
config_plugins_1.WarningAggregator.addWarningAndroid('sentry-expo', 'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration');
config_plugins_1.WarningAggregator.addWarningIOS('sentry-expo', 'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration');
}
return buildSentryPropertiesString(sentryHook.config);
}
exports.getSentryProperties = getSentryProperties;
function buildSentryPropertiesString(sentryHookConfig) {
const { organization, project, authToken, url = 'https://sentry.io/' } = sentryHookConfig ?? {};
const missingProperties = ['organization', 'project', 'authToken'].filter((each) => {
if (!sentryHookConfig?.hasOwnProperty(each)) {
return true;
}
return false;
});
const missingProperties = ['organization', 'project'].filter((each) => !sentryHookConfig?.hasOwnProperty(each));
if (missingProperties.length) {
const warningMessage = `Missing Sentry configuration properties: ${missingProperties.join(', ')} from app.json. Builds will fall back to environment variables. Refer to @sentry/react-native docs for how to configure this.`;
config_plugins_1.WarningAggregator.addWarningAndroid('sentry-expo', warningMessage);
Expand All @@ -58,7 +57,7 @@ function buildSentryPropertiesString(sentryHookConfig) {
return `defaults.url=${url}
${organization ? `defaults.org=${organization}` : missingOrgMessage}
${project ? `defaults.project=${project}` : missingProjectMessage}
${authToken ? `auth.token=${authToken}` : missingAuthTokenMessage}
${authToken ? `# Configure this value through \`SENTRY_AUTH_TOKEN\` environment variable instead. See:https://docs.expo.dev/guides/using-sentry/#app-configuration\nauth.token=${authToken}` : missingAuthTokenMessage}
`;
}
exports.default = (0, config_plugins_1.createRunOncePlugin)(withSentry, pkg.name, pkg.version);
6 changes: 4 additions & 2 deletions plugin/src/__tests__/getSentryProperties-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ describe('Get Sentry properties from app config', () => {
`defaults.url=https://sentry.io/
defaults.org=test-org
defaults.project=myProjectName
# Configure this value through \`SENTRY_AUTH_TOKEN\` environment variable instead. See:https://docs.expo.dev/guides/using-sentry/#app-configuration
auth.token=123-abc
`
);
Expand All @@ -170,6 +171,7 @@ auth.token=123-abc
`defaults.url=https://sentry.io/
defaults.org=test-org
defaults.project=myProjectName
# Configure this value through \`SENTRY_AUTH_TOKEN\` environment variable instead. See:https://docs.expo.dev/guides/using-sentry/#app-configuration
auth.token=123-abc
`
);
Expand All @@ -180,7 +182,7 @@ auth.token=123-abc
`defaults.url=some-url
# no org found, falling back to SENTRY_ORG environment variable
# no project found, falling back to SENTRY_PROJECT environment variable
# no auth.token found, falling back to SENTRY_AUTH_TOKEN environment variable
# auth.token is configured through SENTRY_AUTH_TOKEN environment variable
`
);
});
Expand All @@ -190,7 +192,7 @@ auth.token=123-abc
`defaults.url=some-url
# no org found, falling back to SENTRY_ORG environment variable
# no project found, falling back to SENTRY_PROJECT environment variable
# no auth.token found, falling back to SENTRY_AUTH_TOKEN environment variable
# auth.token is configured through SENTRY_AUTH_TOKEN environment variable
`
);
});
Expand Down
24 changes: 16 additions & 8 deletions plugin/src/withSentry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const withSentry: ConfigPlugin = (config) => {
return config;
};

const missingAuthTokenMessage = `# no auth.token found, falling back to SENTRY_AUTH_TOKEN environment variable`;
const missingAuthTokenMessage = `# auth.token is configured through SENTRY_AUTH_TOKEN environment variable`;
const missingProjectMessage = `# no project found, falling back to SENTRY_PROJECT environment variable`;
const missingOrgMessage = `# no org found, falling back to SENTRY_ORG environment variable`;

Expand All @@ -55,28 +55,36 @@ export function getSentryProperties(config: ExpoConfig): string | null {
return '';
}

if (sentryHook.config?.authToken) {
WarningAggregator.addWarningAndroid(
'sentry-expo',
'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration'
);
WarningAggregator.addWarningIOS(
'sentry-expo',
'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration'
);
}
Comment on lines +58 to +67
Copy link

Choose a reason for hiding this comment

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

"authToken" also needs to be omitted from the missingProperties array at line 74 so that it doesn't warn the user that the field is missing.

Copy link
Member Author

@byCedric byCedric Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jamsch The missing properties doesn't warn users when not defined. The only thing it's doing is adding a comment within the generated sentry.properties file to mention that it's falling back to the SENTRY_AUTH_TOKEN.

We could modify that comment, but I do think it could be useful when users stumble upon the configuration after running $ npx expo prebuild.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the message, and added another comment when using authToken. Just to cover users ignoring the warning, and opening the sentry.properties file.

Copy link

Choose a reason for hiding this comment

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

@byCedric Could you double check that? I'm currently forced to set authToken to false in the config in order for the WarningAggregator message to disappear.

Copy link
Member Author

@byCedric byCedric Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jamsch, can you try on commit bc69f0d? that should take care of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jamsch, just released this: 6.1.1. Thanks for your help :)


return buildSentryPropertiesString(sentryHook.config);
}

function buildSentryPropertiesString(sentryHookConfig: PublishHook['config']) {
const { organization, project, authToken, url = 'https://sentry.io/' } = sentryHookConfig ?? {};
const missingProperties = ['organization', 'project', 'authToken'].filter((each) => {
if (!sentryHookConfig?.hasOwnProperty(each)) {
return true
}
return false
});
const missingProperties = ['organization', 'project'].filter((each) => !sentryHookConfig?.hasOwnProperty(each));

if (missingProperties.length) {
const warningMessage = `Missing Sentry configuration properties: ${missingProperties.join(
', '
)} from app.json. Builds will fall back to environment variables. Refer to @sentry/react-native docs for how to configure this.`;
WarningAggregator.addWarningAndroid('sentry-expo', warningMessage);
WarningAggregator.addWarningIOS('sentry-expo', warningMessage);
}

return `defaults.url=${url}
${organization ? `defaults.org=${organization}` : missingOrgMessage}
${project ? `defaults.project=${project}` : missingProjectMessage}
${authToken ? `auth.token=${authToken}` : missingAuthTokenMessage}
${authToken ? `# Configure this value through \`SENTRY_AUTH_TOKEN\` environment variable instead. See:https://docs.expo.dev/guides/using-sentry/#app-configuration\nauth.token=${authToken}` : missingAuthTokenMessage}
`;
}

Expand Down