Use Ground Truth for authentication #264

petschekr · 2019-03-20T05:49:03Z

Switches to using HackGT Ground Truth for authentication and removes all auth related code from the codebase.

Ground Truth is implemented as the only login strategy which could make initial setup of registration for organizations that don't already host a Ground Truth instance slightly more complicated. Two possible mitigations:

Users can use the 2.x.x branch of registration
Ground Truth supports applications hosted on any domain for any organization. HackGT admins can authorize applications for other organizations if they are ok having their users authenticate via the HackGT Ground Truth instance.

Fixes #252
Fixes #254
Fixes #262

hackgbot · 2019-03-21T06:29:44Z

Hey y'all! A deployment of this PR can be found here:
https://registration-ground-truth.pr.hack.gt

… auth

ehsanmasdar

I want to do some functional testing once we get it deployed, but the code so far looks good to me

ehsanmasdar · 2019-06-16T18:45:30Z

server/routes/api/user.ts

 		if (anonymous) {
 			let email = request.body["anonymous-registration-email"] as string;
 			let name = request.body["anonymous-registration-name"] as string;
-			if (await User.findOne({email})) {
+			if (await User.findOne({ email })) {


So walk-up registration will be done completely outside the ground truth flow. If they later created an account that matched this email using ground truth, would the accounts properly sync up?

also did registration ever get a chance to allow walk up for people who started an application but didn't finish?

@joel99 No that's not implemented yet (and I don't think it's covered by an existing issue)
@ehsanmasdar I don't remember if that's the case. We should do some investigating but that shouldn't block HackGT 6 initial release because walk-up won't happen until day-of

ehsanmasdar · 2019-06-16T18:47:35Z

server/routes/auth.ts

+	if (request.session) {
+		request.session.loginAction = "render";
+	}
+	response.redirect("/login");


Should we redirect user to registration login or ground truth login

Would GT know to redirect back?

It redirects to /login so that we can tell the user that they've successfully logged out. If we redirect to Ground Truth, the user will be told to login again even if that isn't what they want to do

ehsanmasdar · 2019-06-16T18:48:19Z

server/routes/auth.ts

-			}
-		}
-		user.accountConfirmed = true;
+authRoutes.get("/validatehost/:nonce", (request, response) => {


What is the purpose of this? What does this achieve that's not handled by the oauth client library

This is here because we use the Host header when generating links and callback URLs in the auth code. This verifies that the server behind the domain in the Host header is actually us and not some malicious other instance. Not sure if it's overengineered because Host headers can only be crafted by non-browser HTTP libraries (AJAX requests and JavaScript in browsers cannot override the Host header)

ehsanmasdar · 2019-06-16T18:57:10Z

adding @joel99 would like to get a second set of eyes since it's so critical

joel99

@ehsanmasdar can you help spin up a test deployment so we can QA this?
@petschekr embodying refactor as you go 👍

README.md

joel99 · 2019-06-16T23:40:21Z

server/common.ts

 		}
 		// Email
 		if (process.env.EMAIL_FROM) {
-			this.email.from = process.env.EMAIL_FROM!;


any reason a lot of these became optional?

The types for Node.js were updated sometime after this code was written to make arbitrary properties on the process.env object not throw a possibly undefined compiler error. Therefore the non-null assertion operator, !, is no longer required here (and elsewhere).

joel99 · 2019-06-16T23:49:10Z

server/routes/api/user.ts

 		if (anonymous) {
 			let email = request.body["anonymous-registration-email"] as string;
 			let name = request.body["anonymous-registration-name"] as string;
-			if (await User.findOne({email})) {
+			if (await User.findOne({ email })) {


also did registration ever get a chance to allow walk up for people who started an application but didn't finish?

joel99 · 2019-06-17T00:08:11Z

server/routes/auth.ts

+	if (request.session) {
+		request.session.loginAction = "render";
+	}
+	response.redirect("/login");


Would GT know to redirect back?

joel99 · 2019-06-17T00:13:03Z

server/routes/templates.ts

-// Load and compile Handlebars templates
-let [
-	indexTemplate,
-	loginTemplate,


is there some feature that automatically makes a class variable? How does loadTemplate get this.file?

The constructor of the Template class has constructor(private readonly file: string) {} which makes the first argument when called with new a private readonly property automatically. This is shorthand supported by TypeScript

Dockerfile

README.md

client/login.html

server/config/config.example.json

server/middleware.ts

server/routes/api/graphql.ts

evan10s · 2019-06-19T02:37:10Z

server/routes/api/user.ts

@@ -442,14 +462,14 @@ userRoutes.route("/export").get(isAdmin, async (request, response): Promise<void
 		for (let branchName of await Branches.BranchConfig.getNames()) {
 			// TODO: THIS IS A PRELIMINARY VERSION FOR HACKGT CATALYST
 			// TODO: Change this to { "accepted": true }
-			let attendingUsers: IUserMongoose[] = await User.find({ "applied": true, "applicationBranch": branchName });
+			let attendingUsers = await User.find({ "applied": true, "applicationBranch": branchName });


uhhh should probably fix those TODOs above

@ehsanmasdar @joel99 Do you think I can just get rid of this /export endpoint? It's kind of a hidden feature that we haven't used since Catalyst 1 I think

Oh, didn't realize that. We have graphiql and checkin2 can do CSV exports, so I'm guessing we don't need it.

Export data? Yeah, we can grab what we need from db if it becomes relevant if this is an issue. I'd prefer to leave and mark as legacy code for now if it's not blocking, though.

server/routes/templates.ts

evan10s

lgtm, lots of long-needed stuff!

Use Ground Truth for authentication

petschekr added bug enhancement Adds or suggests a new feature, rewrite, or other enhancement to the codebase high priority pending review This PR is pending review by a maintainer labels Mar 20, 2019

petschekr requested a review from ehsanmasdar March 20, 2019 05:49

hackgt-beekeeper bot had a problem deploying to registration-ground-truth-pr March 21, 2019 04:06 Failure

hackgt-beekeeper bot had a problem deploying to registration-ground-truth-pr March 21, 2019 04:19 Failure

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 21, 2019 04:26 Inactive

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 21, 2019 05:52 Inactive

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 21, 2019 06:01 Inactive

petschekr force-pushed the ground-truth branch 3 times, most recently from 77ac737 to 80c5ee4 Compare March 21, 2019 06:27

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 21, 2019 08:20 Inactive

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 21, 2019 08:31 Inactive

hackgt-beekeeper bot deployed to registration-ground-truth-pr March 21, 2019 16:13 View deployment

hackgt-beekeeper bot deployed to registration-ground-truth-pr March 21, 2019 16:17 View deployment

petschekr force-pushed the ground-truth branch from 03028a0 to 08bfe3f Compare March 22, 2019 06:19

petschekr added 7 commits March 22, 2019 00:35

Completely overhaul auth code to use Ground Truth instead of internal…

5c60e28

… auth

Update README to reflect auth changes

82ac003

Update Node version

2a508bd

Bump version to 3.0.0

0f95571

Implement intermediate login page used for login errors and logouts

a3326ca

Log users out of Ground Truth when logging out

3300b3e

Support adding admins by email domain

add5a4c

petschekr force-pushed the ground-truth branch from 08bfe3f to add5a4c Compare March 22, 2019 06:35

ehsanmasdar reviewed Mar 22, 2019

View reviewed changes

Remove unused password reset expiration setting

3838ff2

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr March 28, 2019 04:37 Inactive

hackgt-beekeeper bot temporarily deployed to registration-ground-truth-pr May 6, 2019 12:29 Inactive

hackgt-beekeeper bot had a problem deploying to registration-dev June 16, 2019 18:25 Failure

Update secrets required

e7ed93d

hackgt-beekeeper bot temporarily deployed to registration-dev June 16, 2019 18:35 Inactive

ehsanmasdar reviewed Jun 16, 2019

View reviewed changes

ehsanmasdar requested a review from joel99 June 16, 2019 18:56

joel99 approved these changes Jun 17, 2019

View reviewed changes

ehsanmasdar added this to the HackGT 6 Launch milestone Jun 17, 2019

evan10s requested changes Jun 19, 2019

View reviewed changes

Bump now out-of-date Node version

4d7af31

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 22:48 Inactive

petschekr added 2 commits June 23, 2019 15:50

Fix inconsistencies and typos in README

1c971a8

Move login styles to separate file

7badd48

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 22:53 Inactive

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 22:57 Inactive

Don't make @hack.gt emails hardcoded admin promotion default

58b215c

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 23:01 Inactive

Add example usage of admins promotion configuration

f7fdd98

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 23:05 Inactive

Minor style fix

71ad39a

hackgt-beekeeper bot temporarily deployed to registration-dev June 23, 2019 23:14 Inactive

evan10s approved these changes Jun 24, 2019

View reviewed changes

Merge branch 'master' into ground-truth

b451c6b

hackgt-beekeeper bot temporarily deployed to registration-dev June 25, 2019 13:14 Inactive

petschekr merged commit 5aacf38 into master Jun 25, 2019

ayush-goyal deleted the ground-truth branch December 29, 2020 03:40

rahulrajus pushed a commit that referenced this pull request Jul 22, 2021

Merge pull request #264 from HackGT/ground-truth

96fc683

Use Ground Truth for authentication

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Ground Truth for authentication #264

Use Ground Truth for authentication #264

petschekr commented Mar 20, 2019 •

edited

Loading

hackgbot commented Mar 21, 2019

ehsanmasdar left a comment

ehsanmasdar Jun 16, 2019

joel99 Jun 16, 2019

petschekr Jun 23, 2019

ehsanmasdar Jun 16, 2019

joel99 Jun 17, 2019

petschekr Jun 24, 2019

ehsanmasdar Jun 16, 2019

petschekr Jun 23, 2019

ehsanmasdar commented Jun 16, 2019 •

edited

Loading

joel99 left a comment

joel99 Jun 16, 2019

petschekr Jun 23, 2019

joel99 Jun 16, 2019

joel99 Jun 17, 2019

joel99 Jun 17, 2019

petschekr Jun 24, 2019

joel99 Jun 25, 2019

evan10s Jun 19, 2019

petschekr Jun 23, 2019

evan10s Jun 24, 2019

joel99 Jun 25, 2019

evan10s left a comment

Use Ground Truth for authentication #264

Use Ground Truth for authentication #264

Conversation

petschekr commented Mar 20, 2019 • edited Loading

hackgbot commented Mar 21, 2019

ehsanmasdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehsanmasdar commented Jun 16, 2019 • edited Loading

joel99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan10s left a comment

Choose a reason for hiding this comment

petschekr commented Mar 20, 2019 •

edited

Loading

ehsanmasdar commented Jun 16, 2019 •

edited

Loading