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

Only invalidate session if authenticated #722

Merged
merged 1 commit into from
Oct 19, 2015

Conversation

wireframe
Copy link
Contributor

Fix #721

if (this.get('session.isAuthenticated')) {
this.get('session').invalidate();
}
return new DS.InvalidError(['Session is not authenticated']);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should return an InvalidError as a 401 response doesn't indicate that the record is invalid but that the request wasn't allowed due to missing authorization or insufficient rights. When returning an InvalidError Ember Data would actually try to get errors for the record from the error which isn't really applicable in this case as the error doesn't apply to the record but to the request as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, ok. I can revert that then, and can be addressed as a separate branch if needed.

The error in my case (#721) is related to session invalidation anyways.

@marcoow
Copy link
Member

marcoow commented Oct 19, 2015

Would be good to have a test for this. Also this breaks existing tests.

@marcoow marcoow added the bug label Oct 19, 2015
@marcoow marcoow added this to the 1.x milestone Oct 19, 2015
@wireframe
Copy link
Contributor Author

thanks for the feedback @marcoow.

I've added unit tests and rebased against latest master to cleanup the history. let me know if there's anything else you'd like to see done.

marcoow added a commit that referenced this pull request Oct 19, 2015
Only invalidate session if authenticated
@marcoow marcoow merged commit f0e939e into mainmatter:master Oct 19, 2015
@marcoow
Copy link
Member

marcoow commented Oct 19, 2015

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants