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

Consolidate oauth2 provider type #194

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Consolidate oauth2 provider type #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2918440
Consolidate types
WilliamBergamin Aug 1, 2023
e6aad6e
improve the PR
WilliamBergamin Aug 2, 2023
e331dbc
Merge branch 'main' into consolidate-Oauth2ProviderType
WilliamBergamin Aug 3, 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
1 change: 1 addition & 0 deletions .github/workflows/samples.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ jobs:
- slack-samples/deno-message-translator
- slack-samples/deno-request-time-off
- slack-samples/deno-simple-survey
- slack-samples/deno-github-functions

steps:
- name: Setup Deno
Expand Down
5 changes: 3 additions & 2 deletions src/providers/oauth2/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ import {
OAuth2ProviderDefinitionArgs,
OAuth2ProviderOptions,
} from "./types.ts";

import { OAuth2ProviderTypeValues } from "../../schema/providers/oauth2/types.ts";
import { ManifestOAuth2ProviderSchema } from "../../manifest/manifest_schema.ts";
import {
ManifestOAuth2ProviderSchema,
} from "../../manifest/manifest_schema.ts";

export const DefineOAuth2Provider = (
definition: OAuth2ProviderDefinitionArgs,
Expand Down
55 changes: 28 additions & 27 deletions src/providers/oauth2/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,36 @@
import { OAuth2ProviderTypeValues } from "../../schema/providers/oauth2/types.ts";
import { ManifestOAuth2ProviderSchema } from "../../manifest/manifest_schema.ts";

export type OAuth2ProviderIdentitySchema = {
"url": string;
"account_identifier": string;
"headers"?: {
[key: string]: string;
};
export type OAuth2ProviderDefinitionArgs = ManifestOAuth2ProviderSchema & {
/** A unique name for the provider */
provider_key: string;
};

/**
* TODO: The type system here could be improved one more then one provider type (CUSTOM) is available
* provider_name, authorization_url, token_url, identity_config and authorization_url_extras
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory these should be required but I fear that making these fields required might break some apps

Copy link
Contributor

Choose a reason for hiding this comment

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

How did you determine that these fields are required? Checked the backend schema or some other way?
Can you confirm that not providing these fields works? I.e. can you deploy an app and pass manifest validation if you remove these fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments for each of those field mentioned that they we required for CUSTOM provider types, I am assuming they are true

But since those fields are currently not required I don't think we can make them required without potentially breaking apps

Copy link
Contributor

Choose a reason for hiding this comment

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

"Breaking" from a static-type-check perspective, that is true, however, if the backend requires these fields anyways, then that is much less a concern, I think, because no real app would be able to define a provider without them.

That's why I think we should test this out on a real app 😄

* are only required for CUSTOM provider types
*/
export type OAuth2ProviderOptions = {
/** Client id for your provider */
"client_id": string;
/** Scopes for your provider */
"scope": string[];
/** Display name for your provider. Required for CUSTOM provider types. */
"provider_name"?: string;
/** Authorization url for your provider. Required for CUSTOM provider types. */
"authorization_url"?: string;
/** Token url for your provider. Required for CUSTOM provider types. */
"token_url"?: string;
/** Identity configuration for your provider. Required for CUSTOM provider types. */
"identity_config"?: OAuth2ProviderIdentitySchema;
/** Client id for the provider */
client_id: string;
/** Scopes for the provider */
scope: string[];
/** Display name for the provider. Required for CUSTOM provider types. */
provider_name?: string;
/** Authorization url for the provider. Required for CUSTOM provider types. */
authorization_url?: string;
/** Token url for the provider. Required for CUSTOM provider types. */
token_url?: string;
/** Identity configuration for the provider. Required for CUSTOM provider types. */
identity_config?: OAuth2ProviderIdentity;
/** Optional extras dict for authorization url for your provider. Required for CUSTOM provider types. */
"authorization_url_extras"?: { [key: string]: string };
authorization_url_extras?: { [key: string]: string };
};

export type OAuth2ProviderDefinitionArgs = {
/** A unique name for your provider */
provider_key: string;
/** Type of your provider */
provider_type: OAuth2ProviderTypeValues;
/** OAuth2 Configuration options for your provider */
options: OAuth2ProviderOptions;
export type OAuth2ProviderIdentity = {
url: string;
account_identifier: string;
headers?: {
[key: string]: string;
};
};