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

feat(:unlock:): add support for an OAuth flow that triggers social me… #240

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

jgravois
Copy link
Contributor

…dia login automatically

AFFECTS PACKAGES:
@esri/arcgis-rest-auth

ISSUES CLOSED: #239

…dia login automatically

AFFECTS PACKAGES:
@esri/arcgis-rest-auth

ISSUES CLOSED: #239
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

thanks @jgravois! coupla minor suggestions

@@ -156,6 +163,11 @@ export interface IUserSessionOptions {
*/
portal?: string;

/**
* The authentication provider to use. Defaults to ArcGIS, but Google and Facebook social media logins can also be used.
Copy link
Member

Choose a reason for hiding this comment

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

we may want to say something here like: "Specifying a non-ArcGIS provider will take the user directly to that provider's OAuth page."

redirectUri
)}&state=${state}&locale=${locale}`;
} else {
url = `${portal}/oauth2/social/authorize?client_id=${clientId}&socialLoginProviderName=${provider}&autoAccountCreateForSocial=true&response_type=token&expiration=${duration}&redirect_uri=${encodeURIComponent(
Copy link
Member

Choose a reason for hiding this comment

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

This works for Hub, so you can ignore #239 (comment) - we can add an option for this later if others don't want this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

now I'm leaning back the other way: #239 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

My comments #239 (comment)

@coveralls
Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling a0458f8 on feat/auth-provider into ddd3d57 on master.

@tomwayson
Copy link
Member

LGTM :shipit:!

Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

LGTM.

@tomwayson tomwayson merged commit f1e266e into master Jul 10, 2018
@tomwayson tomwayson deleted the feat/auth-provider branch July 10, 2018 21:26
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.

4 participants