Skip to content
Snippets Groups Projects

Display user email for administrators

Closed Hannes Heine requested to merge pr1808head into pr1808base

Created by: Tirokk

aonomike Authored by aonomike Merged


:cake: 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

Screen Shot 2019-10-05 at 5 29 31 PM

Merge request reports

Approval is optional

Closed by avatar (Jul 5, 2025 12:52am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
128 134 id
129 135 name
130 136 slug
137 email
  • Created by: Tirokk

    roschaefer 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.

  • Created by: Tirokk

    aonomike Authored by aonomike


    Done, thanks @roschaefer

  • Hannes Heine
    Hannes Heine @Elweyn started a thread on commit f8cf975d
  • 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

      roschaefer 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 :heart_eyes:

  • Hannes Heine
    Hannes Heine @Elweyn started a thread on commit f8cf975d
  • 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

      roschaefer 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 :wink:

    • Created by: Tirokk

      mattwr18 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

      aonomike Authored by aonomike


      Thanks @mattwr18 @roschaefer Let me know if I need to update this :D

  • Created by: Mogge

    Review: Approved

    Looks good to me @aonomike! And you even refactored another backend test!! Great job!!

  • Created by: Mogge

    Review: Approved

    hesitant

    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 :wink:

  • Please register or sign in to reply
    Loading