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 eslint warnings and TODOs in OAuth module #1425

Merged
merged 1 commit into from Feb 22, 2022
Merged

Fix eslint warnings and TODOs in OAuth module #1425

merged 1 commit into from Feb 22, 2022

Conversation

seratch
Copy link
Member

@seratch seratch commented Feb 15, 2022

Summary

As a preparation before further development on the OAuth module, this pull request resolves the following issues and TODOs:

  • eslint warnings (the following ones plus disabled ones)
  • TODOs such as user token retrieval in FileInstallationStore
  200:38  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  241:21  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  521:21  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  797:24  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

Requirements (place an x in each [ ])

@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper` labels Feb 15, 2022
@seratch seratch added this to the oauth@2.x milestone Feb 15, 2022
@seratch seratch self-assigned this Feb 15, 2022
seratch
seratch commented Feb 15, 2022
View reviewed changes
packages/oauth/src/index.ts Outdated
...queryResult as Installation<'v2', boolean>,
};
const refreshResponses: OAuthV2TokenRefreshResponse[] = await this.refreshExpiringTokens(tokensToRefresh);
refreshResponses.forEach((refreshResp) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved // eslint-disable-next-line no-restricted-syntax

packages/oauth/src/index.ts Outdated

// TODO: related to the above TODO comment as well
// eslint-disable-next-line no-await-in-loop
await this.installationStore.storeInstallation(updatedInstallation);
Copy link
Member Author

Choose a reason for hiding this comment

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

If both bot and user tokens are refreshed, the current code saves those refreshed token separately (twice in the loop at maximum). However, this can be done at a time. @misscoded Let me know if you see any issues with this.

packages/oauth/src/index.ts Outdated
@@ -197,14 +200,17 @@ export class InstallProvider {
const tokensToRefresh: string[] = detectExpiredOrExpiringTokens(authResult, currentUTCSec);

if (tokensToRefresh.length > 0) {
const installationUpdates: any = { ...queryResult }; // TODO :: TS
Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved the TODO by adding an internal type TokenRefreshInstallationUpdates

packages/oauth/src/index.ts Outdated
// map/reduce? But will the return value be the same? Does order of this array matter?
// eslint-disable-next-line no-restricted-syntax
for (const refreshResp of refreshResponses) {
if (queryResult.authVersion !== 'v2') {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a common situation at all but this can happen if custom InstallationStore#fetchInstallation() method returned an unexpected data.

packages/oauth/src/index.ts Outdated
} catch (error: any) {
throw new AuthorizationError(error.message);
} catch (error) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Member Author

Choose a reason for hiding this comment

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

still disabling eslint here but this revised one should be slightly clearer

packages/oauth/src/index.ts Outdated
@@ -529,12 +538,16 @@ export class InstallProvider {
}

// Call the failure callback
const codedError: CodedError = error as CodedError;
if (codedError.code === undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This never happens as long as bolt-js / @slack/oauth does not have internal bugs, though. This addition is mainly to satisfy the TS compiler.

packages/oauth/src/index.ts Outdated
@@ -794,7 +807,7 @@ function callbackFailure(
async function runAuthTest(token: string, clientOptions: WebClientOptions): Promise<AuthTestResult> {
const client = new WebClient(token, clientOptions);
const authResult = await client.auth.test();
return authResult as any as AuthTestResult;
return authResult as unknown as AuthTestResult;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a recommended way

packages/oauth/src/stores/file-store.ts
@@ -63,11 +63,24 @@ export default class FileInstallationStore implements InstallationStore {
throw new Error('enterpriseId is required to fetch data of an enterprise installation');
}

// TODO :: introduce support for user tokens + querying using userId
Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented

packages/oauth/src/index.ts Outdated Show resolved Hide resolved
filmaj
filmaj approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM!

examples/oauth-v2/app.js Show resolved Hide resolved
@seratch
Copy link
Member Author

seratch commented Feb 15, 2022

@filmaj Thanks for your review!

To the other reviewers:

Still waiting for your responses. Although I've already verified if the change works using the example app, I will add more unit tests to cover more testing patterns before merging this PR (currently we don't have enough tests for this module)

@seratch seratch mentioned this pull request Feb 21, 2022
Merged
2 tasks
@seratch seratch merged commit f1dda9b into slackapi:main Feb 22, 2022
@seratch seratch deleted the oauth-todo-warning branch February 22, 2022 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmaj filmaj filmaj approved these changes

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

@stevengill stevengill Awaiting requested review from stevengill

@misscoded misscoded Awaiting requested review from misscoded

@srajiang srajiang Awaiting requested review from srajiang

Assignees

@seratch seratch

Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper`
Projects
None yet
Milestone
oauth@2.5.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@seratch @filmaj