Server: Immediately ask user to set password after Stripe checkout

packages/server/package.json

@@ -3,7 +3,7 @@
   "version": "2.6.2",
   "private": true,
   "scripts": {
-    "start-dev": "npm run build && nodemon --config nodemon.json --ext ts,js,mustache,css,tsx dist/app.js --env dev",
+    "start-dev": "npm run build && JOPLIN_IS_TESTING=1 nodemon --config nodemon.json --ext ts,js,mustache,css,tsx dist/app.js --env dev",
     "start-dev-no-watch": "node dist/app.js --env dev",
     "rebuild": "npm run clean && npm run build && npm run tsc",
     "build": "gulp build",

packages/server/src/middleware/notificationHandler.test.ts

@@ -2,6 +2,13 @@ import { createUserAndSession, beforeAllDb, afterAllTests, beforeEachDb, models,
 import { Notification, UserFlagType } from '../services/database/types';
 import { defaultAdminEmail, defaultAdminPassword } from '../db';
 import notificationHandler from './notificationHandler';
+import { AppContext } from '../utils/types';
+
+const runNotificationHandler = async (sessionId: string): Promise<AppContext> => {
+	const context = await koaAppContext({ sessionId: sessionId });
+	await notificationHandler(context, koaNext);
+	return context;
+};
 
 describe('notificationHandler', function() {
 
@@ -18,22 +25,25 @@ describe('notificationHandler', function() {
 	});
 
 	test('should check admin password', async function() {
-		const { session } = await createUserAndSession(1, true);
+		const r = await createUserAndSession(1, true);
+		const session = r.session;
+		let admin = r.user;
 
 		// The default admin password actually doesn't pass the complexity
 		// check, so we need to skip validation for testing here. Eventually, a
 		// better mechanism to set the initial default admin password should
 		// probably be implemented.
 
-		const admin = await models().user().save({
+		admin = await models().user().save({
+			id: admin.id,
 			email: defaultAdminEmail,
 			password: defaultAdminPassword,
 			is_admin: 1,
+			email_confirmed: 1,
 		}, { skipValidation: true });
 
 		{
-			const ctx = await koaAppContext({ sessionId: session.id });
+			const ctx = await runNotificationHandler(session.id);
-			await notificationHandler(ctx, koaNext);
 
 			const notifications: Notification[] = await models().notification().all();
 			expect(notifications.length).toBe(1);
@@ -49,8 +59,7 @@ describe('notificationHandler', function() {
 				password: 'changed!',
 			}, { skipValidation: true });
 
-			const ctx = await koaAppContext({ sessionId: session.id });
+			const ctx = await runNotificationHandler(session.id);
-			await notificationHandler(ctx, koaNext);
 
 			const notifications: Notification[] = await models().notification().all();
 			expect(notifications.length).toBe(1);
@@ -69,8 +78,7 @@ describe('notificationHandler', function() {
 			password: defaultAdminPassword,
 		});
 
-		const context = await koaAppContext({ sessionId: session.id });
+		await runNotificationHandler(session.id);
-		await notificationHandler(context, koaNext);
 
 		const notifications: Notification[] = await models().notification().all();
 		expect(notifications.length).toBe(0);
@@ -81,10 +89,24 @@ describe('notificationHandler', function() {
 
 		await models().userFlag().add(user.id, UserFlagType.FailedPaymentFinal);
 
-		const ctx = await koaAppContext({ sessionId: session.id });
+		const ctx = await runNotificationHandler(session.id);
-		await notificationHandler(ctx, koaNext);
 
 		expect(ctx.joplin.notifications.find(v => v.id === 'accountDisabled')).toBeTruthy();
 	});
 
+	test('should display a banner if the email is not confirmed', async function() {
+		const { session, user } = await createUserAndSession(1);
+
+		{
+			const ctx = await runNotificationHandler(session.id);
+			expect(ctx.joplin.notifications.find(v => v.id === 'confirmEmail')).toBeTruthy();
+		}
+
+		{
+			await models().user().save({ id: user.id, email_confirmed: 1 });
+			const ctx = await runNotificationHandler(session.id);
+			expect(ctx.joplin.notifications.find(v => v.id === 'confirmEmail')).toBeFalsy();
+		}
+	});
+
 });

packages/server/src/middleware/notificationHandler.ts

@@ -25,7 +25,7 @@ async function handleChangeAdminPasswordNotification(ctx: AppContext) {
 			_('The default admin password is insecure and has not been changed! [Change it now](%s)', profileUrl())
 		);
 	} else {
-		await notificationModel.markAsRead(ctx.joplin.owner.id, NotificationKey.ChangeAdminPassword);
+		await notificationModel.setRead(ctx.joplin.owner.id, NotificationKey.ChangeAdminPassword);
 	}
 }
 
@@ -57,6 +57,22 @@ async function handleUserFlags(ctx: AppContext): Promise<NotificationView> {
 	return null;
 }
 
+async function handleConfirmEmailNotification(ctx: AppContext): Promise<NotificationView> {
+	if (!ctx.joplin.owner) return null;
+
+	if (!ctx.joplin.owner.email_confirmed) {
+		return {
+			id: 'confirmEmail',
+			messageHtml: renderMarkdown('An email has been sent to you containing an activation link to complete your registration.\n\nMake sure you click it to secure your account and keep access to it.'),
+			levelClassName: levelClassName(NotificationLevel.Important),
+			closeUrl: '',
+		};
+	}
+
+	return null;
+}
+
+
 // async function handleSqliteInProdNotification(ctx: AppContext) {
 // 	if (!ctx.joplin.owner.is_admin) return;
 
@@ -104,11 +120,18 @@ export default async function(ctx: AppContext, next: KoaNext): Promise<void> {
 		if (!ctx.joplin.owner) return next();
 
 		await handleChangeAdminPasswordNotification(ctx);
+		await handleConfirmEmailNotification(ctx);
 		// await handleSqliteInProdNotification(ctx);
 		const notificationViews = await makeNotificationViews(ctx);
 
-		const userFlagView = await handleUserFlags(ctx);
+		const nonDismisableViews = [
-		if (userFlagView) notificationViews.push(userFlagView);
+			await handleUserFlags(ctx),
+			await handleConfirmEmailNotification(ctx),
+		];
+
+		for (const nonDismisableView of nonDismisableViews) {
+			if (nonDismisableView) notificationViews.push(nonDismisableView);
+		}
 
 		ctx.joplin.notifications = notificationViews;
 	} catch (error) {

packages/server/src/middleware/routeHandler.ts

@@ -73,6 +73,6 @@ export default async function(ctx: AppContext) {
 		// Technically this is not the total request duration because there are
 		// other middlewares but that should give a good approximation
 		const requestDuration = Date.now() - requestStartTime;
-		ctx.joplin.appLogger().info(`${ctx.request.method} ${ctx.path} (${requestDuration}ms)`);
+		ctx.joplin.appLogger().info(`${ctx.request.method} ${ctx.path} (${ctx.response.status}) (${requestDuration}ms)`);
 	}
 }

packages/server/src/models/NotificationModel.test.ts

@@ -17,15 +17,15 @@ describe('NotificationModel', function() {
 	});
 
 	test('should require a user to create the notification', async function() {
-		await expectThrow(async () => models().notification().add('', NotificationKey.ConfirmEmail, NotificationLevel.Normal, NotificationKey.ConfirmEmail));
+		await expectThrow(async () => models().notification().add('', NotificationKey.EmailConfirmed, NotificationLevel.Normal, NotificationKey.EmailConfirmed));
 	});
 
 	test('should create a notification', async function() {
 		const { user } = await createUserAndSession(1, true);
 		const model = models().notification();
-		await model.add(user.id, NotificationKey.ConfirmEmail, NotificationLevel.Important, 'testing');
+		await model.add(user.id, NotificationKey.EmailConfirmed, NotificationLevel.Important, 'testing');
-		const n: Notification = await model.loadByKey(user.id, NotificationKey.ConfirmEmail);
+		const n: Notification = await model.loadByKey(user.id, NotificationKey.EmailConfirmed);
-		expect(n.key).toBe(NotificationKey.ConfirmEmail);
+		expect(n.key).toBe(NotificationKey.EmailConfirmed);
 		expect(n.message).toBe('testing');
 		expect(n.level).toBe(NotificationLevel.Important);
 	});
@@ -33,18 +33,18 @@ describe('NotificationModel', function() {
 	test('should create only one notification per key', async function() {
 		const { user } = await createUserAndSession(1, true);
 		const model = models().notification();
-		await model.add(user.id, NotificationKey.ConfirmEmail, NotificationLevel.Important, 'testing');
+		await model.add(user.id, NotificationKey.EmailConfirmed, NotificationLevel.Important, 'testing');
-		await model.add(user.id, NotificationKey.ConfirmEmail, NotificationLevel.Important, 'testing');
+		await model.add(user.id, NotificationKey.EmailConfirmed, NotificationLevel.Important, 'testing');
 		expect((await model.all()).length).toBe(1);
 	});
 
 	test('should mark a notification as read', async function() {
 		const { user } = await createUserAndSession(1, true);
 		const model = models().notification();
-		await model.add(user.id, NotificationKey.ConfirmEmail, NotificationLevel.Important, 'testing');
+		await model.add(user.id, NotificationKey.EmailConfirmed, NotificationLevel.Important, 'testing');
-		expect((await model.loadByKey(user.id, NotificationKey.ConfirmEmail)).read).toBe(0);
+		expect((await model.loadByKey(user.id, NotificationKey.EmailConfirmed)).read).toBe(0);
-		await model.markAsRead(user.id, NotificationKey.ConfirmEmail);
+		await model.setRead(user.id, NotificationKey.EmailConfirmed);
-		expect((await model.loadByKey(user.id, NotificationKey.ConfirmEmail)).read).toBe(1);
+		expect((await model.loadByKey(user.id, NotificationKey.EmailConfirmed)).read).toBe(1);
 	});
 
 });

packages/server/src/models/NotificationModel.ts

@@ -5,7 +5,7 @@ import BaseModel, { ValidateOptions } from './BaseModel';
 
 export enum NotificationKey {
 	Any = 'any',
-	ConfirmEmail = 'confirmEmail',
+	// ConfirmEmail = 'confirmEmail',
 	PasswordSet = 'passwordSet',
 	EmailConfirmed = 'emailConfirmed',
 	ChangeAdminPassword = 'change_admin_password',
@@ -31,10 +31,10 @@ export default class NotificationModel extends BaseModel<Notification> {
 
 	public async add(userId: Uuid, key: NotificationKey, level: NotificationLevel = null, message: string = null): Promise<Notification> {
 		const notificationTypes: Record<string, NotificationType> = {
-			[NotificationKey.ConfirmEmail]: {
+			// [NotificationKey.ConfirmEmail]: {
-				level: NotificationLevel.Normal,
+			// 	level: NotificationLevel.Normal,
-				message: `Welcome to ${this.appName}! An email has been sent to you containing an activation link to complete your registration.`,
+			// 	message: `Welcome to ${this.appName}! An email has been sent to you containing an activation link to complete your registration. Make sure you click it to secure your account and keep access to it.`,
-			},
+			// },
 			[NotificationKey.EmailConfirmed]: {
 				level: NotificationLevel.Normal,
 				message: 'Your email has been confirmed',
@@ -83,12 +83,12 @@ export default class NotificationModel extends BaseModel<Notification> {
 		return this.save({ key: actualKey, message, level, owner_id: userId });
 	}
 
-	public async markAsRead(userId: Uuid, key: NotificationKey): Promise<void> {
+	public async setRead(userId: Uuid, key: NotificationKey, read: boolean = true): Promise<void> {
 		const n = await this.loadByKey(userId, key);
 		if (!n) return;
 
 		await this.db(this.tableName)
-			.update({ read: 1 })
+			.update({ read: read ? 1 : 0 })
 			.where('key', '=', key)
 			.andWhere('owner_id', '=', userId);
 	}

packages/server/src/models/SubscriptionModel.ts

@@ -134,7 +134,7 @@ export default class SubscriptionModel extends BaseModel<Subscription> {
 				account_type: accountType,
 				email,
 				full_name: fullName,
-				email_confirmed: 1,
+				email_confirmed: 0, // Email is not confirmed, because Stripe doesn't check this
 				password: uuidgen(),
 				must_set_password: 1,
 			});

packages/server/src/routes/index/notifications.test.ts

@@ -22,9 +22,9 @@ describe('index_notification', function() {
 
 		const model = models().notification();
 
-		await model.add(user.id, NotificationKey.ConfirmEmail, NotificationLevel.Normal, 'testing notification');
+		await model.add(user.id, NotificationKey.EmailConfirmed, NotificationLevel.Normal, 'testing notification');
 
-		const notification = await model.loadByKey(user.id, NotificationKey.ConfirmEmail);
+		const notification = await model.loadByKey(user.id, NotificationKey.EmailConfirmed);
 
 		expect(notification.read).toBe(0);
 
@@ -41,7 +41,7 @@ describe('index_notification', function() {
 
 		await routeHandler(context);
 
-		expect((await model.loadByKey(user.id, NotificationKey.ConfirmEmail)).read).toBe(1);
+		expect((await model.loadByKey(user.id, NotificationKey.EmailConfirmed)).read).toBe(1);
 	});
 
 });

packages/server/src/routes/index/signup.test.ts

@@ -1,5 +1,4 @@
 import config from '../../config';
-import { NotificationKey } from '../../models/NotificationModel';
 import { AccountType } from '../../models/UserModel';
 import { getCanShareFolder, getMaxItemSize } from '../../models/utils/user';
 import { MB } from '../../utils/bytes';
@@ -53,11 +52,6 @@ describe('index_signup', function() {
 		// Check that the user is logged in
 		const session = await models().session().load(cookieGet(context, 'sessionId'));
 		expect(session.user_id).toBe(user.id);
-
-		// Check that the notification has been created
-		const notifications = await models().notification().allUnreadByUserId(user.id);
-		expect(notifications.length).toBe(1);
-		expect(notifications[0].key).toBe(NotificationKey.ConfirmEmail);
 	});
 
 });

packages/server/src/routes/index/signup.ts

@@ -7,7 +7,6 @@ import config from '../../config';
 import defaultView from '../../utils/defaultView';
 import { View } from '../../services/MustacheService';
 import { checkRepeatPassword } from './users';
-import { NotificationKey } from '../../models/NotificationModel';
 import { AccountType } from '../../models/UserModel';
 import { ErrorForbidden } from '../../utils/errors';
 import { cookieSet } from '../../utils/cookies';
@@ -54,8 +53,6 @@ router.post('signup', async (_path: SubPath, ctx: AppContext) => {
 		const session = await ctx.joplin.models.session().createUserSession(user.id);
 		cookieSet(ctx, 'sessionId', session.id);
 
-		await ctx.joplin.models.notification().add(user.id, NotificationKey.ConfirmEmail);
-
 		return redirect(ctx, `${config().baseUrl}/home`);
 	} catch (error) {
 		return makeView(error);

packages/server/src/routes/index/stripe.test.ts

@@ -87,6 +87,7 @@ describe('index/stripe', function() {
 
 		const user = await models().user().loadByEmail('toto@example.com');
 		expect(user.account_type).toBe(AccountType.Pro);
+		expect(user.email_confirmed).toBe(0);
 
 		const sub = await models().subscription().byUserId(user.id);
 		expect(sub.stripe_subscription_id).toBe('sub_123');

packages/server/src/routes/index/stripe.ts

@@ -1,4 +1,4 @@
-import { SubPath } from '../../utils/routeUtils';
+import { redirect, SubPath } from '../../utils/routeUtils';
 import Router from '../../utils/Router';
 import { Env, RouteType } from '../../utils/types';
 import { AppContext } from '../../utils/types';
@@ -10,9 +10,11 @@ import Logger from '@joplin/lib/Logger';
 import getRawBody = require('raw-body');
 import { AccountType } from '../../models/UserModel';
 import { betaUserTrialPeriodDays, cancelSubscription, initStripe, isBetaUser, priceIdToAccountType, stripeConfig } from '../../utils/stripe';
-import { Subscription, UserFlagType } from '../../services/database/types';
+import { Subscription, User, UserFlagType } from '../../services/database/types';
 import { findPrice, PricePeriod } from '@joplin/lib/utils/joplinCloud';
 import { Models } from '../../models/factory';
+import { confirmUrl } from '../../utils/urlUtils';
+import { msleep } from '../../utils/time';
 
 const logger = Logger.create('/stripe');
 
@@ -116,6 +118,24 @@ export const handleSubscriptionCreated = async (stripe: Stripe, models: Models,
 	}
 };
 
+// For some reason, after checkout Stripe redirects to success_url immediately,
+// without waiting for the "checkout.session.completed" event to be completed.
+// It may be because they expect the webhook to immediately return code 200,
+// which is not how it's currently implemented here.
+// https://stripe.com/docs/payments/checkout/fulfill-orders#fulfill
+//
+// It means that by the time success_url is called, the user hasn't been created
+// yet. So here we wait for the user to be available and return it. It shouldn't
+// wait for more than 2-3 seconds.
+const waitForUserCreation = async (models: Models, userEmail: string): Promise<User | null> => {
+	for (let i = 0; i < 10; i++) {
+		const user = await models.user().loadByEmail(userEmail);
+		if (user) return user;
+		await msleep(1000);
+	}
+	return null;
+};
+
 export const postHandlers: PostHandlers = {
 
 	createCheckoutSession: async (stripe: Stripe, __path: SubPath, ctx: AppContext) => {
@@ -365,11 +385,24 @@ export const postHandlers: PostHandlers = {
 
 const getHandlers: Record<string, StripeRouteHandler> = {
 
-	success: async (_stripe: Stripe, _path: SubPath, _ctx: AppContext) => {
+	success: async (stripe: Stripe, _path: SubPath, ctx: AppContext) => {
+		try {
+			const models = ctx.joplin.models;
+			const checkoutSession = await stripe.checkout.sessions.retrieve(ctx.query.session_id);
+			const userEmail = checkoutSession.customer_details.email || checkoutSession.customer_email; // customer_email appears to be always null but fallback to it just in case
+			if (!userEmail) throw new Error(`Could not find email from checkout session: ${JSON.stringify(checkoutSession)}`);
+			const user = await waitForUserCreation(models, userEmail);
+			if (!user) throw new Error(`Could not find user from checkout session: ${JSON.stringify(checkoutSession)}`);
+			const validationToken = await ctx.joplin.models.token().generate(user.id);
+			const redirectUrl = encodeURI(confirmUrl(user.id, validationToken, false));
+			return redirect(ctx, redirectUrl);
+		} catch (error) {
+			logger.error('Could not automatically redirect user to account confirmation page. They will have to follow the link in the confirmation email. Error was:', error);
 			return `
 				<p>Thank you for signing up for ${globalConfig().appName}! You should receive an email shortly with instructions on how to connect to your account.</p>
 				<p><a href="https://joplinapp.org">Go back to JoplinApp.org</a></p>
 			`;
+		}
 	},
 
 	cancel: async (_stripe: Stripe, _path: SubPath, _ctx: AppContext) => {

packages/server/src/routes/index/users.test.ts

@@ -286,6 +286,27 @@ describe('index/users', function() {
 		expect(notification.key).toBe('passwordSet');
 	});
 
+	test('should not confirm email if not requested', async function() {
+		let user1 = await models().user().save({
+			email: 'user1@localhost',
+			must_set_password: 1,
+			email_confirmed: 0,
+			password: uuidgen(),
+		});
+
+		const email = (await models().email().all()).find(e => e.recipient_id === user1.id);
+		const matches = email.body.match(/\/(users\/.*)(\?token=)(.{32})/);
+		const path = matches[1];
+		const token = matches[3];
+
+		await execRequest('', 'GET', path, null, { query: { token, confirm_email: '0' } });
+
+		// In this case, the email should not be confirmed, because
+		// "confirm_email" is set to 0.
+		user1 = await models().user().load(user1.id);
+		expect(user1.email_confirmed).toBe(0);
+	});
+
 	test('should allow user to verify their email', async function() {
 		let user1 = await models().user().save({
 			email: 'user1@localhost',

packages/server/src/routes/index/users.ts

@@ -3,7 +3,7 @@ import Router from '../../utils/Router';
 import { RouteType } from '../../utils/types';
 import { AppContext, HttpMethod } from '../../utils/types';
 import { bodyFields, contextSessionId, formParse } from '../../utils/requestUtils';
-import { ErrorForbidden, ErrorUnprocessableEntity } from '../../utils/errors';
+import { ErrorBadRequest, ErrorForbidden, ErrorNotFound, ErrorUnprocessableEntity } from '../../utils/errors';
 import { User, UserFlag, UserFlagType, Uuid } from '../../services/database/types';
 import config from '../../config';
 import { View } from '../../services/MustacheService';
@@ -214,7 +214,8 @@ router.get('users/:id/confirm', async (path: SubPath, ctx: AppContext, error: Er
 	const userId = path.id;
 	const token = ctx.query.token;
 
-	if (token) {
+	if (!token) throw new ErrorBadRequest('Missing token');
+
 	const beforeChangingEmailHandler = async (newEmail: string) => {
 		if (config().stripe.enabled) {
 			try {
@@ -230,10 +231,10 @@ router.get('users/:id/confirm', async (path: SubPath, ctx: AppContext, error: Er
 		}
 	};
 
-		await models.user().processEmailConfirmation(userId, token, beforeChangingEmailHandler);
+	if (ctx.query.confirm_email !== '0') await models.user().processEmailConfirmation(userId, token, beforeChangingEmailHandler);
-	}
 
 	const user = await models.user().load(userId);
+	if (!user) throw new ErrorNotFound(`No such user: ${userId}`);
 
 	if (user.must_set_password) {
 		const view: View = {

packages/server/src/utils/urlUtils.ts

@@ -30,8 +30,8 @@ export function helpUrl(): string {
 	return `${config().baseUrl}/help`;
 }
 
-export function confirmUrl(userId: Uuid, validationToken: string): string {
+export function confirmUrl(userId: Uuid, validationToken: string, autoConfirmEmail: boolean = true): string {
-	return `${config().baseUrl}/users/${userId}/confirm?token=${validationToken}`;
+	return `${config().baseUrl}/users/${userId}/confirm?token=${validationToken}${autoConfirmEmail ? '' : '&confirm_email=0'}`;
 }
 
 export function stripePortalUrl(): string {

readme/spec/server_sharing.md

@@ -37,7 +37,7 @@ Any resource attached to the note is also shared - so for example images will be
 
 Any linked note will **not** be shared, due to the following reasons:
 
-- Privacy issue - you don't want to accidentally share a note just because it was linked to another note.
+- Privacy issue - you don't want to accidentally share a note just because it was linked from another note.
 
 - Even if the linked note has been shared separately, we still don't give access to it. We don't know who that link has been shared with - it could be a different recipient.
 
@@ -45,4 +45,4 @@ Any linked note will **not** be shared, due to the following reasons:
 
 It should be possible to have multiple share links for a given note. For example: I share a note with one person, then the same note with a different person. I revoke the share for one person, but I sill want the other person to access the note.
 
-So when a share link is created for a note, the API always return a new link.
+So when a share link is created for a note, the API always returns a new link.