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

feat: use mock authentication for local dev #32935

Merged
merged 3 commits into from Jan 15, 2019

Conversation

tchaffee
Copy link
Contributor

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • My article does not contain shortened URLs or affiliate links.

Closes #17382

@tchaffee tchaffee requested a review from a team October 30, 2018 21:20
@tchaffee
Copy link
Contributor Author

Requires freeCodeCamp/loopback-component-passport#4 to be merged first. This needs to be tested locally and by someone familiar with auth. For sure you need to pay close attention to cookies and clearing them when needed to make sure auth is really happening.

@raisedadead There is only one thing left to do and that's to seed the db with the test user. I will work on that now, but since it is trivial you can start reviewing this code which is ready.

@tchaffee tchaffee force-pushed the fix/local-dev-auth branch 2 times, most recently from 752cd42 to 453f9c7 Compare October 31, 2018 22:24
package.json Outdated
@@ -9,6 +9,7 @@
"lint": "echo 'Warning: TODO - Define Linting.'",
"pretest-ci": "npm-run-all -s lint bootstrap",
"seed": "cross-env DEBUG=fcc:* node ./tools/scripts/seed/seedChallenges",
"seed-auth-user": "cross-env DEBUG=fcc:* node ./tools/scripts/seed/seedAuthUser",

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@@ -0,0 +1,101 @@
const path = require('path');

This comment was marked as resolved.

This comment was marked as resolved.

@tchaffee
Copy link
Contributor Author

tchaffee commented Nov 1, 2018

@raisedadead This is complete now. But I could use your help in organizing parts of this per the previous comments.

@tchaffee
Copy link
Contributor Author

tchaffee commented Nov 1, 2018

@raisedadead I just remembered I forgot to seed the AccessToken collection. But when I tested it without that record, everything worked fine. Maybe we don't need that record using my solution? If there is something I'm missing and we do need it, let's discuss so I can understand why. Easy enough to add it to the seeding process.

@raisedadead
Copy link
Member

Hi @tchaffee, thanks for preparing this PR. I'll pull this down and get back to you.

@raisedadead raisedadead self-assigned this Nov 1, 2018
package.json Outdated
@@ -9,6 +9,7 @@
"lint": "echo 'Warning: TODO - Define Linting.'",
"pretest-ci": "npm-run-all -s lint bootstrap",
"seed": "cross-env DEBUG=fcc:* node ./tools/scripts/seed/seedChallenges",
"seed-auth-user": "cross-env DEBUG=fcc:* node ./tools/scripts/seed/seedAuthUser",

This comment was marked as off-topic.

This comment was marked as off-topic.

package.json Outdated
@@ -9,6 +9,7 @@
"lint": "echo 'Warning: TODO - Define Linting.'",
"pretest-ci": "npm-run-all -s lint bootstrap",
"seed": "cross-env DEBUG=fcc:* node ./tools/scripts/seed/seedChallenges",

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -0,0 +1,101 @@
const path = require('path');

This comment was marked as resolved.

api-server/server/boot/authentication.js Outdated Show resolved Hide resolved
@raisedadead
Copy link
Member

I am quite happy with the logic flow. I am yet to test this out.

Unrelated side notes: (This is not at all a concern of this PR and must be taken later)

  • One this that is killing me is at the minute is the user.insertOne. Its messy because we are replicating the schema in the code. I am not a fan of that.
  • The ideal way would be to spin up loopback and do the seeding with its APIs. i.e User.create. This is so, that we do not worry about the default values in the schema. Loopback will add/update the keys for us and then we just do a User.Update on specific things.
  • This should ideally be done for challenge collection as well.

Again just for discussion, do not worry about it right now. I am pretty sure we abandoned it for some reason that I can't recall.

@raisedadead
Copy link
Member

I just remembered I forgot to seed the AccessToken collection. But when I tested it without that record, everything worked fine. Maybe we don't need that record using my solution?

I'll get back with a update when I am done testing. Thanks for your patience.

@raisedadead raisedadead added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. platform: api Server application that needs familiarity with Express, Loopback, MongoDB etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. labels Nov 1, 2018
Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

I forgot! Could you please update the sample.env with this default value for local env variable.

By default the setup starts in development mode.

@raisedadead
Copy link
Member

raisedadead commented Nov 1, 2018

I just remembered I forgot to seed the AccessToken collection. But when I tested it without that record, everything worked fine. Maybe we don't need that record using my solution?

I'll get back with a update when I am done testing. Thanks for your patience.

So, what I can see from your changes:

            // This is the same format as would be returned by Auth0.
            // The id comes from a record that should already be in the db
            // due to seeding at development time.
            const accessToken = {
              id: 'nyynsTyQrmXNo8APMKiG7SAkSvsBiC1XnZ8QhI2GuhfixAaV6hEpuJ5gFTNeEcJU',
              ttl: 77760000000,
              created: new Date().toISOString(),
              userId: u.id
            };

This is where you are bypassing the AccessToken creation with Loopback and hardcoding it. I think we are probably going to be fine using this hack 😉 !

Since, the real reason behind the local auth is to let collaborators to be able to work with pages that are hidden behind an authenticated user, this works perfectly for our needs.

@tchaffee
Copy link
Contributor Author

tchaffee commented Nov 8, 2018

@raisedadead I added LOCAL_MOCK_AUTH=true to sample.env and changed the test in authentication.js to test against that value. Let me know if there is anything else outstanding. Otherwise I think this is ready for merge once someone else has tested it locally.

@nikrb
Copy link
Member

nikrb commented Nov 27, 2018

@tchaffee this works locally for me.

Welcome page "Goto to next challenge" button seems to be dead, but curriculum menu item works.
Clicking the dev user icon in the menu top right goes to the settings page. "Show me my public portfolio" button shows me a gatsby page missing error. "Sign me out of freecodecamp" button seems to work.

@raisedadead
Copy link
Member

Hi @tchaffee @nikrb

We are still working on a couple things for the platform, and hence the delay on getting this merged.

But just to let you know:

... Welcome page "Goto to next challenge" button seems to be dead ...

This is known and Stuart has already dropped a fix.

"Show me my public portfolio" button shows me a gatsby page missing error...

This is a unfortunate limitation of Gatsby's Dev Server for the moment.

The public profile page is actually available and you should see it just fine if you click the button that says Preview custom 404 page.

image

Please note this will not be an issue when the client site is deployed to the production. You won't have the dev server intercepting the dynamic routes. We would need a fix to the Gatsby core to address this and I think they have something in works.

Until then you can simply click the button to get you to the right page when you are developing.

@tchaffee
Copy link
Contributor Author

tchaffee commented Dec 4, 2018

@nikrb thank you for testing this.

@raisedadead I think I've made all your requested changes. Is there anything else I need to do on this or are we just waiting now for other stuff?

@QuincyLarson
Copy link
Contributor

@raisedadead Could you take another look at this and see whether it's ready to go (aside from the merge conflicts)? This could be a huge improvement to onboarding new contributors.

Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

Needs updated @freeCodeCamp/loopback-component-passport

Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

Nice work @tchaffee !

I have taken the liberty and did some chores. Other than that this was an awesome PR to QA. Thanks for your patience with this and the hard work.

Happy contributing!

@raisedadead raisedadead merged commit 381ebb0 into freeCodeCamp:master Jan 15, 2019
@tchaffee tchaffee deleted the fix/local-dev-auth branch April 8, 2019 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: api Server application that needs familiarity with Express, Loopback, MongoDB etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants