Skip to content
This repository was archived by the owner on Sep 24, 2019. It is now read-only.

Conversation

@cjoudrey
Copy link
Contributor

fyi @xuorig this will impact your mutation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably dont want to differentiate invalid user vs password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used to think the same until I read a UX article about this.

It's easy to know if a username is valid or not by attempting to register with that username, so by returning Invalid username or password we're not really making it any more secure, we're just making the UX worse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

why this check exactly? instead of just returning ctx[:user] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should change this code a bit.

When someone is not logged in, I store GuestUser.new in ctx[:user], but in this specific case I think we want to resolve to nil when the person is not logged in.

Maybe a better approach would be to define def logged_in? in GuestUser and User such that we can simply do ctx[:user] if ctx[:user].logged_in?.

We can use the same logic in our mutations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need GuestUser if we just return nil in these cases ? I guess eventually you want to show certain fields under viewer even when youre a guest ?

I think i like the second approach better yep!

@cjoudrey
Copy link
Contributor Author

Going to do what's simplest for now, ctx[:user] will be nil when user isn't logged in.

We'll see if we need GuestUser down the line.

@cjoudrey cjoudrey merged commit 657a86b into master Feb 13, 2017
@cjoudrey cjoudrey deleted the add-authentication branch February 13, 2017 03:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants