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

[PAX-11407] [merge after 8/25] removing client_secret_env_key from manifest #83

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

uhunnyslack
Copy link
Contributor

@uhunnyslack uhunnyslack commented Aug 22, 2022

Summary

Jira ticket: https://jira.tinyspeck.com/browse/PAX-11407

  • The environment variable name client_secret_env_key became redundant and the internal 3p auth users are supposed to be using a new subcommand add-secret instead of the .env variable
  • Therefore we are removing the field client_secret_env_key from the SDK object
  • The webapp should be okay to accept the oauth2 object now if this field is optional there (TODO check)
  • There will be a CLI change as well to get rid of this field completely. PR
  • This PR can be merged after the new add-secret subcommand is available to the end user.

How to test:

  • This needs to be tested after the client_secret_env_key field becomes optional in the PR
  • We need to update the SDK to point to this version by creating an app and updating the import_map.json file
  • Try updating the manifest with a provider object that doesnt have the client_secret_env_key field
  • There should be no error when the app is deployed
  • Both client_secret and client_secret_env_key field should be empty in the providers table in db
    import.map file example:
{
  "imports": {
    "deno-slack-sdk/": "https://raw.githubusercontent.com/slackapi/deno-slack-sdk/6527d84/src/",
    "deno-slack-api/": "https://deno.land/x/deno_slack_api@0.0.3/"
  }
}

Manifest.ts file in app file example:


const GoogleProvider = DefineOAuth2Provider({
  provider_key: "google",
  provider_type: Schema.providers.oauth2.CUSTOM,
  options: {
    "provider_name": "Google",
    "authorization_url": "https://accounts.google.com/o/oauth2/auth",
    "token_url": "https://oauth2.googleapis.com/token",
    "client_id":
      "1086877417652-b1o23iogo2dqithh2rrspt967i70kig6.apps.googleusercontent.com",
     //  "client_secret_env_key": "google_client_secret", --> this is deleted
    "scope": [
      "https://www.googleapis.com/auth/spreadsheets",
      "https://www.googleapis.com/auth/userinfo.email",
      "https://www.googleapis.com/auth/userinfo.profile",
    ],
    "authorization_url_extras": {
      "prompt": "consent",
      "access_type": "offline",
    },
    "identity_config": {
      "url": "https://www.googleapis.com/oauth2/v1/userinfo",
      "account_identifier": "$.email",
    },
  },
});

Requirements (place an x in each [ ])

@uhunnyslack uhunnyslack requested a review from a team as a code owner August 22, 2022 17:57
@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #83 (8ddf965) into main (1245b73) will not change coverage.
The diff coverage is n/a.

❗ Current head 8ddf965 differs from pull request most recent head ae6749b. Consider uploading reports for the commit ae6749b to get more accurate results

@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files          41       41           
  Lines        1565     1565           
  Branches       87       87           
=======================================
  Hits         1504     1504           
  Misses         59       59           
  Partials        2        2           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@uhunnyslack uhunnyslack force-pushed the removing_client_secret_env_key branch from d820120 to 6527d84 Compare August 22, 2022 18:01
@uhunnyslack uhunnyslack reopened this Aug 22, 2022
@uhunnyslack uhunnyslack changed the title removing client_secret_env_key from manifest [PAX-11407] [merge after 8/25] removing client_secret_env_key from manifest Aug 22, 2022
@uhunnyslack
Copy link
Contributor Author

@nupurgoyall , can we merge this right now ? What is the SDK release cutoff ? Is it the same as the cli?

Copy link
Contributor

@nupurgoyall nupurgoyall left a comment

Choose a reason for hiding this comment

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

LGTM! Tested this with both the webapp and CLI PRs:
https://slack-github.com/slack/webapp/pull/151759
https://github.com/slackapi/slack-cli/pull/682

We can merge this before the 9/1 release candidate and after the 8/25 release code freeze is lifted.

@uhunnyslack uhunnyslack force-pushed the removing_client_secret_env_key branch from 6527d84 to 75c939c Compare August 25, 2022 16:28
@uhunnyslack uhunnyslack force-pushed the removing_client_secret_env_key branch from 75c939c to ae6749b Compare August 25, 2022 18:18
@uhunnyslack uhunnyslack merged commit 70f29a2 into main Aug 25, 2022
@uhunnyslack uhunnyslack deleted the removing_client_secret_env_key branch August 25, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants