Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions services/api/src/routes/users.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ describe('/1/users', () => {
expect(updatedUser.name).toBe('Other Name');
});

it('should allow submitting an unchanged email', async () => {
const user = await createUser({ email: 'self@bar.com' });
const response = await request('PATCH', '/1/users/me', { firstName: 'New', email: 'self@bar.com' }, { user });
expect(response).toHaveStatus(200);
expect(response.body.data.firstName).toBe('New');
});

it('should still reject an email taken by another user', async () => {
await createUser({ email: 'other@bar.com' });
const user = await createUser({ email: 'self@bar.com' });
const response = await request('PATCH', '/1/users/me', { email: 'other@bar.com' }, { user });
expect(response).toHaveStatus(400);
});

it('should be able to patch the device token', async () => {
let user = await createUser();
const response = await request(
Expand Down Expand Up @@ -322,6 +336,27 @@ describe('/1/users', () => {
expect(dbUser.name).toEqual('New Name');
});

it('should allow submitting an unchanged email', async () => {
const admin = await createAdmin();
const user1 = await createUser({ email: 'old@bar.com' });
const response = await request(
'PATCH',
`/1/users/${user1.id}`,
{ firstName: 'New', email: 'old@bar.com' },
{ user: admin },
);
expect(response).toHaveStatus(200);
expect(response.body.data.firstName).toBe('New');
});

it('should still reject an email taken by another user', async () => {
const admin = await createAdmin();
await createUser({ email: 'taken@bar.com' });
const user1 = await createUser({ email: 'mine@bar.com' });
const response = await request('PATCH', `/1/users/${user1.id}`, { email: 'taken@bar.com' }, { user: admin });
expect(response).toHaveStatus(400);
});

it('should deny access to non-admins', async () => {
const user = await createUser({});
const user1 = await createUser({ firstName: 'New', lastName: 'Name' });
Expand Down
12 changes: 12 additions & 0 deletions services/api/src/utils/middleware/params.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ function fetchByParam(Model, options = {}) {
ctx.throw(403);
}
ctx.state[docName] = doc;
excludeFromUniqueChecks(ctx, doc);
} catch (error) {
ctx.throw(400, error);
}
Expand All @@ -35,10 +36,20 @@ function fetchByParamWithSlug(Model, options) {
ctx.throw(401);
}
ctx.state[docName] = doc;
excludeFromUniqueChecks(ctx, doc);
return next();
};
}

// Unique checks in update validation exclude the target document by an id in
// the body, which clients have no reason to send. Take it from the document the
// route already resolved. Update validation strips it again before assign.
function excludeFromUniqueChecks(ctx, doc) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not sure I understand this

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The copy is bad ?

Or you dont understand the problem ?

if (ctx.method === 'PATCH' || ctx.method === 'PUT') {
ctx.request.body.id = doc.id;
}
}

Comment on lines +44 to +52

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@andrewplummer can you take a look to see if this best solution

The problem is that right, if you try to patch users/me

Image

You get this, this because the patch doesnt set the id in the body.

if there is no id in the body the uniqueness check triggers and fails

async function checkAccess(ctx, doc, options = {}) {
const { hasAccess = () => true } = options;
return await hasAccess(ctx, doc);
Expand All @@ -49,6 +60,7 @@ async function checkAccess(ctx, doc, options = {}) {
// when performing "self" checks for "writeAccess".
function isSelf(ctx, next) {
ctx.state.user = ctx.state.authUser;
excludeFromUniqueChecks(ctx, ctx.state.authUser);
return next();
}

Expand Down
12 changes: 7 additions & 5 deletions services/api/src/utils/templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ const renderer = new TemplateRenderer({

async function renderTemplate(options) {
const { dir, ...rest } = resolveOptions(options);
const template = await resolveTemplateArg(options);
const { template, isFile } = await resolveTemplateArg(options);

return renderer.run({
dir,
// A body loaded from the database is the template source itself, so
// resolving it against "dir" would read it as a (very long) filename.
dir: isFile ? dir : undefined,
template,
params: {
...rest,
Expand Down Expand Up @@ -50,16 +52,16 @@ async function resolveTemplateArg(options) {

if (typeof template === 'string' && path.extname(template) !== '') {
// Return template filename if an extension if found.
return template;
return { template, isFile: true };
} else if (template) {
const templateBody = await resolveTemplateBody(options);

// If the template arg is a name then find the template
// and return the body for the channel. Fall back to the
// template name to find it as a file.
return templateBody || template;
return templateBody ? { template: templateBody } : { template, isFile: true };
} else {
return body;
return { template: body };
}
}

Expand Down
20 changes: 20 additions & 0 deletions services/api/src/utils/templates.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,26 @@ describe('renderTemplate', () => {
expect(result.body).toBe('Hello from doc, Frank');
});

it('should render a long template from document', async () => {
// A body longer than the filesystem name limit must not be resolved as a path.
const email = ['---', 'subject: Welcome', '---', '', 'Hello {{name}},', '', 'x'.repeat(500)].join('\n');
await Template.create({
name: 'long',
email,
});

const result = await renderTemplate({
channel: 'email',
template: 'long',
params: {
name: 'Frank',
},
});

expect(result.meta.subject).toBe('Welcome');
expect(result.body).toContain('Hello Frank,');
});

it('should be able to pass a raw template with channel', async () => {
const result = await renderTemplate({
template: 'Hello',
Expand Down
4 changes: 2 additions & 2 deletions services/web/src/screens/Auth/Login.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ async function loginOtp(body) {
method: 'POST',
path: `/1/auth/otp/send`,
body: {
...body,
email: body.email,
type: AUTH_TYPE,
authChannel: AUTH_CHANNEL,
channel: AUTH_CHANNEL,
},
});
}
Expand Down
Loading