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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add logout #131

Merged
merged 7 commits into from
Jul 21, 2017
Merged

add logout #131

merged 7 commits into from
Jul 21, 2017

Conversation

housseindjirdeh
Copy link
Member

Logging out is a little tricky, I can't thank you enough @RolfKoenders for getting it started and apologize you didn't have the easiest thing to start working on.

We use the Oauth web flow in the application and not Basic Authentication. In the API, revoking access tokens and deleting authorizations only work for Basic Authentication.

This PR does the following:

  1. Some minor styling in the AuthProfileScreen (Check for updates tap target is a bit bigger now, centered and darkened note on requesting access on repos, made sure fontSizes were normalized.

  1. When the user logs out, the auth reducer portion of the Redux store is cleared. That's where we store the access token and isAuthenticated logic
  2. AsyncStorage is also cleared because the Redux store is persisted there
  3. User is navigated to the Login Screen

All this works just fine. The tricky part is when the user clicks login once again. This is how it would work:

justnavigate

Notice how I didn't have to type in my username and password again. This is because I'm still logged in to Safari hence why I'm just redirected right back with a new access token.

So here's what I did: open github.com/logout using the Safari View to allow the user to log out through the browser. Unfortunately they have to click Done to be able to navigated back to the app (I don't know if it's possible to redirect automatically after a Safari event)

completelogout

Let me know what you folks think, and please let me know if there's anything I should change in this PR 馃檶

} else {
Linking.openURL(url);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

I feel like openLink is a fairly complex method that will (and should) likely be used in other places. Not sure where it should be moved to, though, maybe utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed it's already being used elsewhere, will update it in a bit 馃檶

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - moved it to a helper method 馃憤

RolfKoenders and others added 6 commits July 20, 2017 20:08
* Show logout option on profile page. Show alert when clicking logout

* WIP clear cache and navigate back to login screen

* Use absolute imports

* Processing feedback of the PR and wip code to signout

* Signing out is working, navigating back to login screen.
@housseindjirdeh housseindjirdeh merged commit a3744e5 into gitpoint:master Jul 21, 2017
@andrewda
Copy link
Member

andrewda commented Jul 21, 2017

I don't really understand the inner-workings of how the login session is maintained, but would something like https://github.com/reactnativecn/react-native-http-cache/ work?

@housseindjirdeh
Copy link
Member Author

Hmm thanks will take a look at that tomorrow for sure. The logout flow is relatively straightforward but it's just that the password/username persists in Safari hence it logs you in automatically when a user logs out and tries to sign in again (as opposed to having you type your details once more)

@andrewda
Copy link
Member

Ah ok. Yea clearing the cache seems like it'd probably work (right?) but not sure if the package I linked above will do that.

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

3 participants