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

adding manifest property for token url configs #198

Closed
wants to merge 2 commits into from

Conversation

uhunnyslack
Copy link
Contributor

@uhunnyslack uhunnyslack commented Aug 16, 2023

Summary

To support more connectors, 3p auth manifest config requires some more new SDK fields

This PR is to add two new fields that helps the connector team unblock creating some more connectors.

  1. use_pkce : boolean -> this is true for providers that uses PKCE:

  2. token_url_config : { use_basic_authentication_scheme_scheme : boolean} to support providers that use this.

More discussions can be found in our slack instance:

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #198 (a50ac0f) into main (738d367) will increase coverage by 0.00%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   99.24%   99.24%           
=======================================
  Files          56       56           
  Lines        2125     2128    +3     
  Branches      136      136           
=======================================
+ Hits         2109     2112    +3     
  Misses         15       15           
  Partials        1        1           

see 1 file with indirect coverage changes

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

@uhunnyslack uhunnyslack marked this pull request as ready for review August 16, 2023 02:03
@uhunnyslack uhunnyslack requested a review from a team as a code owner August 16, 2023 02:03
@WilliamBergamin
Copy link
Contributor

@uhunnyslack I was working on #194 that aims to consolidate the provider args options type and the type found in the manifest we decided to put this PR on hold waiting to see if your changes could conflict with mine

Do you know if there will be a new provider other changes coming in the near future and would it be worth it to create a typescript type dedicated to PKCE providers?

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.

Overall LGTM, left some comments on removing the apps.json file and adding more docs.

Let's coordinate in Slack re: releasing this.

@@ -8,6 +8,10 @@ export type OAuth2ProviderIdentitySchema = {
};
};

export type tokenUrlConfigSchema = {
"use_basic_authentication_scheme"?: boolean;
Copy link
Contributor

@filmaj filmaj Aug 17, 2023

Choose a reason for hiding this comment

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

Let's add a JSdoc here to explain what this is used for. The tech spec mentioned a default value - let's make sure to document that default value here too.

@@ -0,0 +1 @@
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file.

/** Identity configuration for your provider. Required for CUSTOM provider types. */
"identity_config"?: OAuth2ProviderIdentitySchema;
/** Optional extras dict for authorization url for your provider. Required for CUSTOM provider types. */
"authorization_url_extras"?: { [key: string]: string };
/** Optional boolean flag to specify if the provider requires PKCE. Required for CUSTOM provider types. */
"use_pkce"?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also document the default value of this field if not specified.

@filmaj filmaj added feature request New feature or request do not merge semver:minor requires a minor version number bump labels Aug 17, 2023
@uhunnyslack uhunnyslack reopened this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge feature request New feature or request semver:minor requires a minor version number bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants