Export Via Key Functionality #155

mjkaufer · 2017-09-19T00:59:52Z

To prepare for the integration between checkin and registration, I've added a configurable exportKey parameter and a route at /api/user/all/export/:exportKey. Since we don't have CAS hooked up right now, we need some alternate way to tell registration that checkin is allowed to fetch data from it. You can configure the key from the registration admin settings.

After this is merged, I'll add functionality in checkin to request this endpoint with a configurable key, so we can programmatically import the zip file into checkin.

hackgbot · 2017-09-19T01:02:27Z

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

petschekr

Overall, I like this. There are just 1 or 2 minor code issues. Also, I think it would make more sense for the key to be set randomly using crypto.randomBytes() by default or always rather than being blank or set by the user.

petschekr · 2017-09-20T04:18:39Z

server/common.ts

+		});
+	} else {
+		let userKey = request.params.exportKey;
+		console.log("Yasss", userKey, exportKey);


Can you please remove this console.log?

petschekr · 2017-09-20T04:20:49Z

server/routes/templates.ts

@@ -133,7 +133,8 @@ templateRoutes.route("/").get(authenticateWithRedirect, async (request, response
 		user: request.user,
 		settings: {
 			teamsEnabled: await getSetting<boolean>("teamsEnabled"),
-			qrEnabled: await getSetting<boolean>("qrEnabled")
+			qrEnabled: await getSetting<boolean>("qrEnabled"),
+			exportKey: await getSetting<string>("exportKey")


Why is the export key being sent to the template engine for routes that don't need it?

mjkaufer · 2017-09-20T04:53:33Z

@petschekr implemented your changes, lmk if I need anything else. There are some conflicts with master; I'll resolve those once everything looks good to you

petschekr · 2017-09-20T15:35:59Z

server/common.ts

@@ -306,7 +306,7 @@ export async function setDefaultSettings() {
 		"confirmationClose": new Date(),
 		"teamsEnabled": true,
 		"qrEnabled": true,
-		"exportKey": "",
+		"exportKey": crypto.randomBytes(16).toString('hex'),


double quotes plz

petschekr · 2017-09-20T15:37:12Z

Other than that this looks ready to merge once Andrew's dumpster fire is extinguished and you can fix any merge conflicts that probably cropped up

mjkaufer · 2017-09-20T15:57:43Z

@petschekr just added double quotes

@illegalprime can you take a look over this when you're free

Lmk when Andrew's stuff gets sorted out and I'll fix merge conflicts.

illegalprime

Andrew's stuff should be done, you can rebase. Looks good save for just two comments.

illegalprime · 2017-09-20T18:40:21Z

client/js/admin.ts

@@ -473,6 +473,9 @@ settingsUpdateButton.addEventListener("click", e => {
 	let qrEnabledData = new FormData();
 	qrEnabledData.append("enabled", (document.getElementById("qr-enabled") as HTMLInputElement).checked ? "true" : "false");

+	let exportKeyData = new FormData();
+	exportKeyData.append("key", (document.getElementById("export-key") as HTMLInputElement).value);


trim whitespace

illegalprime · 2017-09-20T18:43:29Z

server/common.ts

@@ -306,6 +306,7 @@ export async function setDefaultSettings() {
 		"confirmationClose": new Date(),
 		"teamsEnabled": true,
 		"qrEnabled": true,
+		"exportKey": crypto.randomBytes(16).toString("hex"),


best expressed as a configuration option (so you can set an env var or file with this key on multiple instances).
I think we should eventually merge settings + configuration, what do you think @petschekr ?

@illegalprime I figured this might be better as a setting, since we wouldn't have to restart the server to change the key

mjkaufer · 2017-09-20T20:44:24Z

@illegalprime @petschekr just merged with Andrew's code

illegalprime · 2017-09-22T02:59:18Z

@mjkaufer didn't read the summary of your PR, but we have cas auth hooked up right now! We can communicate in a better way than pushing CSVs back n' forth.

illegalprime · 2017-09-22T03:07:39Z

I also think that the right way to do this is to provide APIs that check-in can use to:

Text search the name and email of the user
Find a user by his or her ID
Pull up some registration question data (like t-shirt size) for the user

illegalprime · 2017-09-22T03:28:52Z

BTW: We're thinking of doing it through GraphQL for all our services, but now these patent issues have made me uneasy: graphql/graphql-spec#351 ...

illegalprime · 2017-09-27T02:45:10Z

I changed it to just be a config.

petschekr · 2017-09-27T15:49:44Z

server/middleware.ts

@@ -73,7 +73,10 @@ export function isUserOrAdmin(request: express.Request, response: express.Respon
 export function isAdmin(request: express.Request, response: express.Response, next: express.NextFunction) {
 	response.setHeader("Cache-Control", "private");
 	let user = request.user as IUser;
-	if (!request.isAuthenticated()) {
+	if (request.query.adminKey === config.secrets.adminKey) {


Are you sure you want this admin-level access information in a query string? It may be behind HTTPS but will probably appear in server logs / history logs. A cookie or Authorization header might be better.

illegalprime · 2017-09-29T07:06:27Z

@petschekr changed it to a header

petschekr · 2017-09-29T17:55:07Z

server/middleware.ts

+	const auth = request.headers.authorization;
+
+	if (auth && typeof auth === "string") {
+		const key = new Buffer(auth.split(" ")[1], "base64").toString();


This will crash the server if the Authorization header doesn't have a space in it due to it indexing an array of length 0 or 1

… random exportKey by default

mjkaufer requested review from petschekr and illegalprime September 19, 2017 00:59

bunsenmcdubbs assigned bunsenmcdubbs and mjkaufer and unassigned bunsenmcdubbs Sep 20, 2017

petschekr added this to the HackGT 4 milestone Sep 20, 2017

petschekr requested changes Sep 20, 2017

View reviewed changes

petschekr added enhancement Adds or suggests a new feature, rewrite, or other enhancement to the codebase medium priority labels Sep 20, 2017

petschekr reviewed Sep 20, 2017

View reviewed changes

illegalprime requested changes Sep 20, 2017

View reviewed changes

illegalprime force-pushed the export-to-checkin branch 2 times, most recently from b02f404 to 8f6175c Compare September 26, 2017 05:29

illegalprime mentioned this pull request Sep 26, 2017

GraphQL API #159

Merged

petschekr requested changes Sep 27, 2017

View reviewed changes

illegalprime approved these changes Sep 29, 2017

View reviewed changes

petschekr requested changes Sep 29, 2017

View reviewed changes

petschekr approved these changes Sep 30, 2017

View reviewed changes

mjkaufer added 2 commits September 30, 2017 14:04

Track export key

b0949e1

Hide export key from non-admins

35e1e73

mjkaufer and others added 8 commits September 30, 2017 14:04

Route to fetch csv zip with export key

f37218d

Don't send exportKey unnecessarily, removed some silly logs, generate…

5d73671

… random exportKey by default

Export key is now a configuration and not a setting.

20a45d5

Added secret in deployment.

88afa41

Made adminKey a request param.

71bf2f4

Changed auth to header.

9f67fdc

Fixed potential auth token bug

96d16d6

Bump version to 1.10.8

79bf69b

petschekr force-pushed the export-to-checkin branch from 8b3d038 to 79bf69b Compare September 30, 2017 18:24

petschekr merged commit 83ebc11 into master Sep 30, 2017

petschekr deleted the export-to-checkin branch October 7, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export Via Key Functionality #155

Export Via Key Functionality #155

mjkaufer commented Sep 19, 2017

hackgbot commented Sep 19, 2017

petschekr left a comment

petschekr Sep 20, 2017

mjkaufer Sep 20, 2017

petschekr Sep 20, 2017

mjkaufer commented Sep 20, 2017

petschekr Sep 20, 2017

petschekr commented Sep 20, 2017

mjkaufer commented Sep 20, 2017

illegalprime left a comment

illegalprime Sep 20, 2017

illegalprime Sep 20, 2017

mjkaufer Sep 20, 2017

mjkaufer commented Sep 20, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 27, 2017

petschekr Sep 27, 2017 •

edited

Loading

illegalprime Sep 28, 2017

illegalprime commented Sep 29, 2017

petschekr Sep 29, 2017

Export Via Key Functionality #155

Export Via Key Functionality #155

Conversation

mjkaufer commented Sep 19, 2017

hackgbot commented Sep 19, 2017

petschekr 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

mjkaufer commented Sep 20, 2017

Choose a reason for hiding this comment

petschekr commented Sep 20, 2017

mjkaufer commented Sep 20, 2017

illegalprime 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

mjkaufer commented Sep 20, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 22, 2017

illegalprime commented Sep 27, 2017

petschekr Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illegalprime commented Sep 29, 2017

Choose a reason for hiding this comment

petschekr Sep 27, 2017 •

edited

Loading