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

Resolve Authentication Issues #169

Merged
merged 19 commits into from
Dec 18, 2014
Merged

Resolve Authentication Issues #169

merged 19 commits into from
Dec 18, 2014

Conversation

thisandagain
Copy link
Contributor

This PR makes some major changes to the way authentication and sessions are managed by the application. In general, the goal was to simplify the codebase to work exclusively with Github authentication and reduce dependencies. This was acomplished by removing boilerplate, moving to client-side session storage, and eliminating the need for a backing datastore.

Revisions

  • Reduce the number of unused dependencies both within package.json and require-ed in the app itself
  • Remove the need for a backing database (Mongo) by eliminating the need for a session store (using client-sessions) and eliminating the storage of user accounts
  • Migrate from passport to github-oauth
  • Add AirBNB style check via JSCS as per the engineering handbook
  • Add script for npm test that includes style and basic route checks

Resolves

GH-156
GH-149

@thisandagain
Copy link
Contributor Author

R? @simonwex @secretrobotron

@davidascher
Copy link

Good riddance. Thanks!
On Dec 18, 2014 1:35 PM, "Andrew Sliwinski" notifications@github.com
wrote:

R? @simonwex https://github.com/simonwex @secretrobotron
https://github.com/secretrobotron


Reply to this email directly or view it on GitHub
#169 (comment)
.

@thisandagain
Copy link
Contributor Author

NP!


mongoose.connect(secrets.db);
mongoose.connection.on('error', function() {
console.error('MongoDB Connection Error. Please make sure that MongoDB is running.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this entire patch was prompted in order to remove this >80-char line. ;) 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😉 That .. and not a fan of storing OAuth tokens and user email addresses if we don't have to.

@simonwex
Copy link
Contributor

Tested sign-in sign-out. Works for me. I left a couple comments in code, nothing critical. Yay!

simonwex added a commit that referenced this pull request Dec 18, 2014
Resolve Authentication Issues
@simonwex simonwex merged commit 4c09c86 into MozillaFoundation:master Dec 18, 2014
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.

3 participants