Display user email for administrators
Created by: Tirokk
Authored by aonomike Merged

Pullrequest

- Display user email to administrators
- Refactor Permission middleware tests
- Add tests to check email of user only revealed to admin and owner
Issues
fixes #1704 (closed)
Images

Merge request reports
Activity
128 134 id 129 135 name 130 136 slug 137 email Created by: Tirokk
Authored by roschaefer
Outdated (history rewrite) - original diff
@@ -128,6 +138,7 @@ export default { id name slug + email
At the moment, the backend does not allow anybody except the owner to see the
email
of the user. You will need to change permissions in the backend.I think I would still feel safe, if only the owner or an admin user can see the email.
42 it("does not expose the owner's email address", async () => { 43 let response = {} 44 try { 45 await action() 46 } catch (error) { 47 response = error.response.data 48 } finally { 49 expect(response).toEqual({ User: [null] }) 50 } 72 it("throws an error and does not expose the owner's email address", async () => { 73 await expect( 74 query({ query: userQuery, variables: { name: 'Owner' } }), 75 ).resolves.toMatchObject({ 76 errors: [{ message: 'Not Authorised!' }], 77 data: { User: [null] }, 78 }) Created by: Tirokk
Authored by roschaefer
Outdated (history rewrite) - original diff
@@ -1,89 +1,141 @@ -import { GraphQLClient } from 'graphql-request' +import { createTestClient } from 'apollo-server-testing' +import createServer from '../server' import Factory from '../seed/factories' -import { host, login } from '../jest/helpers' +import { gql } from '../jest/helpers' +import { getDriver, neode as getNeode } from '../bootstrap/neo4j' const factory = Factory() +const instance = getNeode() +const driver = getDriver() + +let query, authenticatedUser, owner, anotherRegularUser, administrator, variables, moderator + +const userQuery = gql` + query($name: String) { + User(name: $name) { + email + } + } +` describe('authorization', () => { + beforeAll(async () => { + await factory.cleanDatabase() + const { server } = createServer({ + context: () => ({ + driver, + instance, + user: authenticatedUser, + }), + }) + query = createTestClient(server).query + }) + describe('given two existing users', () => { beforeEach(async () => { - await factory.create('User', { - email: 'owner@example.org', - name: 'Owner', - password: 'iamtheowner', - }) - await factory.create('User', { - email: 'someone@example.org', - name: 'Someone else', - password: 'else', - }) + ;[owner, anotherRegularUser, administrator, moderator] = await Promise.all([ + factory.create('User', { + email: 'owner@example.org', + name: 'Owner', + password: 'iamtheowner', + }), + factory.create('User', { + email: 'another.regular.user@example.org', + name: 'Another Regular User', + password: 'else', + }), + factory.create('User', { + email: 'admin@example.org', + name: 'Admin', + password: 'admin', + role: 'admin', + }), + factory.create('User', { + email: 'moderator@example.org', + name: 'Moderator', + password: 'moderator', + role: 'moderator', + }), + ]) + variables = {} }) afterEach(async () => { await factory.cleanDatabase() }) describe('access email address', () => { - let headers = {} - let loginCredentials = null - const action = async () => { - if (loginCredentials) { - headers = await login(loginCredentials) - } - const graphQLClient = new GraphQLClient(host, { headers }) - return graphQLClient.request('{User(name: "Owner") { email } }') - } - - describe('not logged in', () => { - it('rejects', async () => { - await expect(action()).rejects.toThrow('Not Authorised!') + describe('unauthenticated', () => { + beforeEach(() => { + authenticatedUser = null }) - - it("does not expose the owner's email address", async () => { - let response = {} - try { - await action() - } catch (error) { - response = error.response.data - } finally { - expect(response).toEqual({ User: [null] }) - } + it("throws an error and does not expose the owner's email address", async () => { + await expect( + query({ query: userQuery, variables: { name: 'Owner' } }), + ).resolves.toMatchObject({ + errors: [{ message: 'Not Authorised!' }], + data: { User: [null] }, + })
This is so much better
33 33 <b>{{ scope.row.name | truncate(20) }}</b> 34 34 </nuxt-link> 35 35 </template> 36 <template slot="email" slot-scope="scope"> 37 <a :href="`mailto:${scope.row.email}`"> 38 <b>{{ scope.row.email }}</b> Created by: Tirokk
Authored by roschaefer
Outdated (history rewrite) - original diff
@@ -33,6 +33,11 @@ <b>{{ scope.row.name | truncate(20) }}</b> </nuxt-link> </template> + <template slot="email" slot-scope="scope"> + <a :href="`mailto:${scope.row.email}`"> + <b>{{ scope.row.email }}</b>
Some of those emails are quite long. Maybe we should think about how to fit the data into the layout? E.g. at some places in the app, we use
| truncate(20)
filter.Although I'm OK to merge this and wait for our admins to complain about it
Created by: Tirokk
Authored by mattwr18
That's what I thought as well, @datenbrei mentioned that they might be copying and pasting the links, so maybe it's more difficult if it's truncated??
Created by: Tirokk
Authored by aonomike
Thanks @mattwr18 @roschaefer Let me know if I need to update this :D
Created by: Mogge
Review: Approved
To me, this looks good and I can't see why we should not merge it. It's tested and there is nothing that comes to my mind, which we need to do before merging. Great job @aonomike !
But I'm a little hesitant to merge this before our daily standup. We could hear back from the others if we missed anything important here. it's a matter of security/data-privacy after all