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

Implement full signOut flow #8

Closed
wants to merge 2 commits into from
Closed

Implement full signOut flow #8

wants to merge 2 commits into from

Conversation

thchia
Copy link
Owner

@thchia thchia commented Jan 11, 2019

For #7

  • Refactored Callback component to be generic and call whatever redirect function it was given.
  • Added a SignoutCallback component, to be rendered at the appropriate signout url.
  • Call signoutRedirect in the signOut method.

This will be released in the next 0.x release, since the implementation of signOut has changed.

- Refactored Callback component to be generic and call whatever redirect function it was given.
- Added a SignoutCallback component, to be rendered at the appropriate signout url.
- Call signoutRedirect in the signOut method.
@thchia thchia added the invalid This doesn't seem right label Jan 14, 2019
<Callback
onSuccess={props.onSuccess}
onError={props.onError}
redirectCallback={props.userManager.signinRedirectCallback}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Need to props.userManager.signinRedirectCallback.bind(props.userManager)

<Callback
onSuccess={props.onSuccess}
onError={props.onError}
redirectCallback={props.userManager.signoutRedirectCallback}
Copy link
Owner Author

Choose a reason for hiding this comment

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

See above.

@thchia thchia closed this Jan 22, 2019
@dankremniov
Copy link

dankremniov commented Mar 7, 2019

@thchia, is there any reason why you closed this PR? Full sign out changes made to src/makeAuth/index.tsx makes total sense for me - exactly what I was looking for. I think to extend Authentificator with these changes locally. Just wondering if you have identified any blocking issue with that?

@thchia
Copy link
Owner Author

thchia commented Mar 7, 2019

Hey, you can look through #7 for an explanation, but basically I determined that the functionality that I was trying to add was not compatible with all Identity Providers.

Since this is supposed to be IdP agnostic I closed the PR. However there are 2 ways for you to handle this:

  • If your IdP supports this flow you can make any changes you want in your own project to support it. I don’t think there’s anything blocking it.

  • If you can implement a safe way to add this functionality to the library, feel free to make a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants