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

Add method to check the status of the token #34

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

META-DREAMER
Copy link
Contributor

No description provided.

@nicolaslopezj
Copy link
Owner

In what case one would login with token? The token is passed in every call

@META-DREAMER
Copy link
Contributor Author

It's used on the client side to persist the login between app launches. It automatically runs as soon and the client is initialized and if it logs in successfully it will run the onLogin callback on the client.

@dbrrt
Copy link
Contributor

dbrrt commented Jan 15, 2017

loginWithToken could be really useful to implement a SSO authentication

@META-DREAMER
Copy link
Contributor Author

@nicolaslopezj Any update on how soon this can be merged and pushed to atmosphere? Having to use a dirty hack in my app right now to get this working...

@nicolaslopezj
Copy link
Owner

But I don't understand... If you have the token you are already logged in, why making this necessary?

@dbrrt
Copy link
Contributor

dbrrt commented Jan 16, 2017

@HammadJ Can you share the context of your application using this feature? It looks really interesting, but for now except a Single Sign On, I don't see why it should be used by most meteor-apollo-accounts users.

@META-DREAMER
Copy link
Contributor Author

In the cases where the stored token is no longer valid, we want to notify the client of that. If the token isn't valid, the onLoginFailure callback will be called as soon as the app starts and the client can deal with it appropriately.

For example, in my react-native app, the app starts on a splash screen when I initialize meteor-apollos-accounts, which causes it to automatically send the loginWithToken query. I set the onLogin callback to change the root/route of my app to the main screen. I also set the onLoginFailure callback to change the root of my app to the Login screen. As soon as meteor-apollos-accounts is able to successfully login with the saved token, it calls the onLogin callback. If the login fails, it deletes the stored token and calls the onLoginFailure callback.

Example:

export function initApp() {
  return (dispatch, getState, client) => {
    dispatch({ type: types.INIT_APP });

    Accounts.setTokenStore(tokenStore);

    Accounts.initWithClient(client);

    Accounts.onLogin(() => {
      const userId = Accounts.userId();
      if (userId) {
        errorLogger.setUserId(userId);
        dispatch(changeAppRoot(constants.MAIN));
      }
    });

    Accounts.onLoginFailure(() => {
      dispatch(changeAppRoot(constants.LOGIN));
    });

    return dispatch(initAppComplete());
  };
}

This is the same pattern used in react-native-meteor

@META-DREAMER
Copy link
Contributor Author

@nicolaslopezj check out #37 to see where it would be used in the client. This would be called on initialization: https://github.com/nicolaslopezj/meteor-apollo-accounts/pull/37/files#diff-36c47fb33022d2183169ad7c05522fadR75

@nicolaslopezj
Copy link
Owner

Ok, I understand what you are trying to achieve, but I think is not the best way to "double log in".

We should not make mutation to check the login state. We could create a query that returns the login status of the user, which is called when the app starts.

It could be passed to the client package as "checkToken()". If the token has expired or is deleted, the package will log out locally (remove token from store).

This method will not be required, it can be used if you need it on your app.

@META-DREAMER
Copy link
Contributor Author

@nicolaslopezj alright I've changed it to using a query now, will update the other pull requests also, lemme know if this is good

@nicolaslopezj
Copy link
Owner

We need to go slow, one pr at the time. This are too many changes

@@ -8,6 +8,9 @@ export default function (options) {
type Mutation {
# Log the user in with a password.
loginWithPassword (username: String, email: String, password: HashedPassword, plainPassword: String): LoginMethodResponse

# Log the user in with a token
loginWithToken (token: String!): LoginMethodResponse
Copy link
Owner

Choose a reason for hiding this comment

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

We still need to delete this mutation

export default async function (root, { token }, {userId}) {
const user = Meteor.users.findOne({
_id: userId,
'services.resume.loginTokens.hashedToken' : Accounts._hashLoginToken(token)
Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to check the date

import {Meteor} from 'meteor/meteor'

export default async function (root, { token }, {userId}) {
const user = Meteor.users.findOne({
Copy link
Owner

Choose a reason for hiding this comment

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

It's faster to do find().count()? I don't know

Copy link
Contributor

Choose a reason for hiding this comment

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

Both might be appropriate (IMO),
also find().limit(1)

Copy link
Owner

@nicolaslopezj nicolaslopezj Jan 17, 2017

Choose a reason for hiding this comment

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

But count() would only bring a Int from the db and find() would return the whole document

Copy link
Contributor

Choose a reason for hiding this comment

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

So in this case it'll be faster, you're right.

@nicolaslopezj nicolaslopezj changed the title Add loginWithToken mutation to server Add method to check the status of the token Jan 17, 2017
@nicolaslopezj
Copy link
Owner

Now we need to do the checkToken method clientside

@META-DREAMER
Copy link
Contributor Author

META-DREAMER commented Jan 17, 2017

@nicolaslopezj Alright I pushed the changes

As for the checkToken method on the server, I think there's something we can do that will be a lot easier. Since apollo's meteor integration automatically checks the token and injects the userId and user into the context if the token is valid, we can simply check if we have a userId in the context or not. That way, we can change checkToken on the server to:

export default async function (root, variables, { userId }) {
  return {
    success: !!userId,
    userId
  };
}

We won't even have to pass the token in the query, just make sure we load it from LocalStorage/AsyncStorage before we send the query (assuming the user has configured their apollo client correctly).

You can see that the check their doing in the meteor integration is very similar to what we have:
https://github.com/apollostack/meteor-integration/blob/master/main-server.js#L66

If you're cool with this I'll update the pull requests to do this instead.

@nicolaslopezj
Copy link
Owner

Yes! that's a better solution

@META-DREAMER
Copy link
Contributor Author

@nicolaslopezj Alright pushed the changes, lemme know if its good!

@META-DREAMER
Copy link
Contributor Author

@nicolaslopezj How soon can we see this merged?

@ujwal-setlur
Copy link

Did this get merged? Status still shows open...

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