Auth state persistence #109
Conversation
|
This is my first PR on this project, so please tell me if I should be doing anything differently. I looked for unit tests to add some, but couldn't find any in the aws-amplify-react-native package; point me to the right place and I can add some. I considered adding a config option to the Auth class that would allow for turning off this feature, thus preserving the old behavior. I actually have a commit that it mostly done, but I decided to not add it to this PR because I couldn't tell if it was necessary, i.e. would anyone not want this feature? I also wasn't sure which the default should be: new feature 'on' by default (which I think is the expected behavior, for someone new to this library), or 'off' by default (which would not break the experience for current users). If there is a desire to be able to toggle this feature on/off, I can pretty easily add it as an option. |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
=======================================
Coverage 92.24% 92.24%
=======================================
Files 52 52
Lines 2424 2424
Branches 486 486
=======================================
Hits 2236 2236
Misses 179 179
Partials 9 9Continue to review full report at Codecov.
|
rlmartin
force-pushed
the
auth-state-persistence
branch
2 times, most recently
from
d548fdf
to
57cc599
Compare
|
Would be great if this was added. |
| return (this._userPoolStorageSync || Promise.resolve()).then(result => { | ||
| if (!that.userPool) { return Promise.reject('No userPool'); } | ||
|
|
||
| return that.userPool.getCurrentUser(); |
There was a problem hiding this comment.
When retrieving the user from storage the signInUserSession field on the CognitoUser is null. Should _getSyncedUser() not set/refresh the session?
e.g.
const user = that.userPool.getCurrentUser();
if (!user) {
return Promise.reject('No user');
}
return new Promise((resolve, reject) => {
user.getSession((err, session) => {
if (err) {
reject(err);
} else {
user.setSignInUserSession(session)
resolve(user);
}
});
});There was a problem hiding this comment.
A couple thoughts.
First, it looks to me like signInUserSession gets assigned as part of the successful user.getSession call here: https://github.com/aws/aws-amplify/blob/923514dc42d1122642773ec5f00a7cd405d51c8b/packages/aws-amplify-react-native/src/lib/aws-cognito-identity/CognitoUser.js#L909. So unless I'm mistaken, I don't think we need the user.setSignInUserSession(session) line above.
Second - and here's where my unfamiliarity with this library's intricacies comes out - the existing code separates the concerns of getting the user from getting the user session. I specifically noticed that there are both currentUser and currentAuthenticatedUser methods - the latter of which loads the session, the former does not (current behavoir). Your suggestion would change this behavior, since I call_getSyncedUser() from both of these methods (plus a couple others). I believe that my commit as-is preserves current behavior - but feel free to correct me if I'm wrong. I think the session should be loaded into user.signInUserSession at the same time that it currently would be without this added feature of caching to AsyncStorage.
Are you seeing a particular example in which you are not seeing a populated user.signInUserSession when you would expect it to be populated? I can look into it if so.
|
@mlabieniec @powerful23 @nidsharm @richardzcode what is the process for getting a review? I don't think I've missed anything in CONTRIBUTING.md, but feel free to tell me if I did. |
|
@rlmartin thanks again for the pr. Just fyi We are still working on this functionality, and just fyi, what we are doing is abstracting a lot of this stuff away from being react native specific, especially for things like auth. We want these things in the aws-amplify core to avoid duplication of code. So once we complete this task the core lib will handle the sessions and RN will naturally inherit the functionality using it's native storage. |
rlmartin
force-pushed
the
auth-state-persistence
branch
from
57cc599
to
1787a0f
Compare
This will maintain the auth state across sessions instead of losing it when the app is refreshed.
This is necessary because Auth.currentUser() returns a Promise. When a user is logged in and the app is reloaded, there is a brief flicker during which the SignIn screen is shown before the authenticated app appears. This Loading screen replaces that flicker and will toggle to either signIn or signedIn, as appropriate.
rlmartin
force-pushed
the
auth-state-persistence
branch
from
1787a0f
to
56d9e72
Compare
|
This logic has been merged into aws-amplify core and is now available in react native. This will be in the next publish to npm. |
|
@mlabieniec next publish means now with Thanks |
|
@Darkade Yes that is correct. So now, unless you are using HOCs / components i.e. Authenticator you can just use aws-amplify within react native, and you need to use aws-amplify to initially configure. Basically rn is the same as aws-amplify-react now. |
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
When using out-of-the-box
withAuthenticator, the user authorization will not persist across sessions (i.e. when you reload the app). This commit adds persistence of the auth state across sessions, by using thesyncmethod added to the react-native-aws-cognito-js library here: https://github.com/AirLabsTeam/react-native-aws-cognito-js/blob/master/src/StorageHelper.js#L71 (it uses the React Native AsyncStorage under the hood).Please see the commit message for commentary on the second commit in this PR.