Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: prevent non work emails #3993

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
20 changes: 11 additions & 9 deletions packages/frontend-2/components/auth/RegisterWithEmailBlock.vue
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
v-model="email"
type="email"
name="email"
label="Email"
label="Work email"
placeholder="Email"
size="lg"
color="foundation"
Expand Down Expand Up @@ -69,17 +69,14 @@ import { ToastNotificationType, useGlobalToast } from '~~/lib/common/composables
import { ensureError } from '@speckle/shared'
import { useAuthManager } from '~~/lib/auth/composables/auth'
import { loginRoute } from '~~/lib/common/helpers/route'
import { passwordRules } from '~~/lib/auth/helpers/validation'
import {
passwordRules,
doesNotContainBlockedDomain
} from '~~/lib/auth/helpers/validation'
import { graphql } from '~~/lib/common/generated/gql'
import type { ServerTermsOfServicePrivacyPolicyFragmentFragment } from '~~/lib/common/generated/gql/graphql'
import { useMounted } from '@vueuse/core'

/**
* TODO:
* - (BE) Password strength check? Do we want to use it anymore?
* - Dim's answer: no, `passwordRules` are legit enough for now.
*/

graphql(`
fragment ServerTermsOfServicePrivacyPolicyFragment on ServerInfo {
termsOfService
Expand All @@ -99,13 +96,18 @@ const router = useRouter()
const { signUpWithEmail, inviteToken } = useAuthManager()
const { triggerNotification } = useGlobalToast()
const isMounted = useMounted()
const isNoPersonalEmailsEnabled = useIsNoPersonalEmailsEnabled()

const newsletterConsent = defineModel<boolean>('newsletterConsent', { required: true })
const loading = ref(false)
const password = ref('')
const email = ref('')

const emailRules = [isEmail]
const emailRules = computed(() =>
inviteToken.value || !isNoPersonalEmailsEnabled.value
? [isEmail]
: [isEmail, doesNotContainBlockedDomain]
)
const nameRules = [isRequired]

const isEmailDisabled = computed(() => !!props.inviteEmail?.length || loading.value)
Expand Down
8 changes: 8 additions & 0 deletions packages/frontend-2/composables/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,12 @@ export const useIsBillingIntegrationEnabled = () => {
return ref(FF_BILLING_INTEGRATION_ENABLED)
}

export const useIsNoPersonalEmailsEnabled = () => {
const {
public: { FF_NO_PERSONAL_EMAILS_ENABLED }
} = useRuntimeConfig()

return ref(FF_NO_PERSONAL_EMAILS_ENABLED)
}

export { useGlobalToast, useActiveUser, usePageQueryStandardFetchPolicy }
8 changes: 8 additions & 0 deletions packages/frontend-2/lib/auth/helpers/validation.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isStringOfLength, stringContains } from '~~/lib/common/helpers/validation'
import { blockedDomains } from '@speckle/shared'

export const passwordLongEnough = isStringOfLength({ minLength: 8 })
export const passwordHasAtLeastOneNumber = stringContains({
Expand All @@ -20,3 +21,10 @@ export const passwordRules = [
passwordHasAtLeastOneLowercaseLetter,
passwordHasAtLeastOneUppercaseLetter
]

export const doesNotContainBlockedDomain = (val: string) => {
const domain = val.split('@')[1]?.toLowerCase()
return domain && blockedDomains.includes(domain)
? 'Please use your work email instead of a personal email address'
: true
}
10 changes: 7 additions & 3 deletions packages/server/modules/auth/strategies/local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import {
} from '@/modules/core/services/ratelimiter'
import { getIpFromRequest } from '@/modules/shared/utils/ip'
import { InviteNotFoundError } from '@/modules/serverinvites/errors'
import { UserInputError, PasswordTooShortError } from '@/modules/core/errors/userinput'

import {
UserInputError,
PasswordTooShortError,
BlockedEmailDomainError
} from '@/modules/core/errors/userinput'
import { ServerInviteResourceType } from '@/modules/serverinvites/domain/constants'
import { getResourceTypeRole } from '@/modules/serverinvites/helpers/core'
import { AuthStrategyMetadata, AuthStrategyBuilder } from '@/modules/auth/helpers/types'
Expand Down Expand Up @@ -117,7 +120,7 @@ const localStrategyBuilderFactory =
invite = await deps.validateServerInvite(user.email, req.session.token)
}

// 3. at this point we know, that we have one of these cases:
// 3.. at this point we know, that we have one of these cases:
// * the server is invite only and the user has a valid invite
// * the server public and the user has a valid invite
// * the server public and the user doesn't have an invite
Expand Down Expand Up @@ -155,6 +158,7 @@ const localStrategyBuilderFactory =
case PasswordTooShortError:
case UserInputError:
case InviteNotFoundError:
case BlockedEmailDomainError:
req.log.info({ err }, 'Error while registering.')
return res.status(400).send({ err: e.message })
default:
Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/auth/tests/apps.graphql.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('GraphQL @apps-api', () => {
;({ sendRequest } = await initializeTestServer(ctx))
testUser = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie@gmail.com',
email: 'didimitrie@example.org',
password: 'wtfwtfwtf'
}

Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/auth/tests/apps.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ const validateToken = validateTokenFactory({
describe('Services @apps-services', () => {
const actor = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie@gmail.com',
email: 'didimitrie@example.org',
password: 'wtfwtfwtf'
}

Expand Down Expand Up @@ -495,7 +495,7 @@ describe('Services @apps-services', () => {
})
const secondUser = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie.wow@gmail.com',
email: 'didimitrie.wow@example.org',
password: 'wtfwtfwtf'
}

Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/auth/tests/helpers/registration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export type LocalAuthRestApiHelpers = ReturnType<typeof localAuthRestApi>
export const generateRegistrationParams = (): RegisterParams => ({
challenge: faker.string.uuid(),
user: {
email: (random(0, 1000) + faker.internet.email()).toLowerCase(),
email: `${random(0, 1000)}@example.org`.toLowerCase(),
password: faker.internet.password(),
name: faker.person.fullName()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { AllScopes } from '@/modules/core/helpers/mainConstants'
import { updateServerInfoFactory } from '@/modules/core/repositories/server'
import { findInviteFactory } from '@/modules/serverinvites/repositories/serverInvites'
import { getFeatureFlags } from '@/modules/shared/helpers/envHelper'
import { expectToThrow, itEach } from '@/test/assertionHelper'
import { BasicTestUser, createTestUsers } from '@/test/authHelper'
import {
Expand All @@ -33,6 +34,8 @@ import {
import { Roles } from '@speckle/shared'
import { expect } from 'chai'

const { FF_NO_PERSONAL_EMAILS_ENABLED } = getFeatureFlags()

const updateServerInfo = updateServerInfoFactory({ db })

describe('Server registration', () => {
Expand Down Expand Up @@ -96,6 +99,18 @@ describe('Server registration', () => {
expect(user.emails.every((e) => !e.verified)).to.be.true
})

FF_NO_PERSONAL_EMAILS_ENABLED
? it('rejects registration with blocked email domain', async () => {
const params = generateRegistrationParams()
params.user.email = 'test@gmail.com'

const error = await expectToThrow(() => restApi.register(params))
expect(error.message).to.contain(
'Please use your work email instead of a personal email address'
)
})
: null

it('fails without challenge', async () => {
const params = generateRegistrationParams()
params.challenge = ''
Expand Down
6 changes: 6 additions & 0 deletions packages/server/modules/core/errors/userinput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,9 @@ export class UnverifiedEmailSSOLoginError extends UserInputError<UnverifiedEmail
'Email already in use by a user with unverified email. Verify the email on the existing user to be able to log in with this method.'
static code = 'UNVERIFIED_EMAIL_SSO_LOGIN_ERROR'
}

export class BlockedEmailDomainError extends UserInputError {
static defaultMessage =
'Please use your work email instead of a personal email address'
static code = 'BLOCKED_EMAIL_DOMAIN_ERROR'
}
19 changes: 18 additions & 1 deletion packages/server/modules/core/services/users/management.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ import {
UserUpdateError,
UserValidationError
} from '@/modules/core/errors/user'
import { PasswordTooShortError, UserInputError } from '@/modules/core/errors/userinput'
import {
BlockedEmailDomainError,
PasswordTooShortError,
UserInputError
} from '@/modules/core/errors/userinput'
import { UserUpdateInput } from '@/modules/core/graph/generated/graphql'
import type { UserRecord } from '@/modules/core/helpers/userHelper'
import { sanitizeImageUrl } from '@/modules/shared/helpers/sanitization'
import {
blockedDomains,
isNullOrUndefined,
NullableKeysToOptional,
Roles,
Expand All @@ -48,6 +53,9 @@ import { DeleteAllUserInvites } from '@/modules/serverinvites/domain/operations'
import { GetServerInfo } from '@/modules/core/domain/server/operations'
import { EventBusEmit } from '@/modules/shared/services/eventBus'
import { UserEvents } from '@/modules/core/domain/users/events'
import { getFeatureFlags } from '@/modules/shared/helpers/envHelper'

const { FF_NO_PERSONAL_EMAILS_ENABLED } = getFeatureFlags()

export const MINIMUM_PASSWORD_LENGTH = 8

Expand Down Expand Up @@ -163,6 +171,15 @@ export const createUserFactory =

if (!finalUser.email?.length) throw new UserInputError('E-mail address is required')

// Temporary experiment: require work emails for all new users
const isBlockedDomain = blockedDomains.includes(
finalUser.email.split('@')[1]?.toLowerCase()
)
const requireWorkDomain =
!user?.signUpContext?.isInvite && FF_NO_PERSONAL_EMAILS_ENABLED

if (requireWorkDomain && isBlockedDomain) throw new BlockedEmailDomainError()

let expectedRole = null
if (finalUser.role) {
const isValidRole = Object.values(Roles.Server).includes(finalUser.role)
Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/core/tests/apitokens.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const createAppToken = createAppTokenFactory({
describe('API Tokens', () => {
const user1: BasicTestUser = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie@gmail.com',
email: 'didimitrie@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down Expand Up @@ -321,7 +321,7 @@ describe('API Tokens', () => {
describe('with limited resource access', () => {
const user2: BasicTestUser = {
name: 'Some other guy',
email: 'bababooey@gmail.com',
email: 'bababooey@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/core/tests/batchCommits.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,13 @@ describe('Batch commits', () => {

const me: BasicTestUser = {
name: 'batch commit dude',
email: 'batchcommitguy@gmail.com',
email: 'batchcommitguy@example.org',
id: ''
}

const otherGuy: BasicTestUser = {
name: 'other batch commit guy',
email: 'otherbatchcommitguy@gmail.com',
email: 'otherbatchcommitguy@example.org',
id: ''
}

Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/core/tests/branches.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ const createObject = createObjectFactory({
describe('Branches @core-branches', () => {
const user = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie4342@gmail.com',
email: 'didimitrie4342@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/core/tests/commits.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ const createObject = createObjectFactory({
describe('Commits @core-commits', () => {
const user = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie4342@gmail.com',
email: 'didimitrie4342@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/core/tests/favoriteStreams.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,12 +221,12 @@ describe('Favorite streams', () => {
}
const me = {
name: 'Itsa Me',
email: 'me@gmail.com',
email: 'me@example.org',
password: 'sn3aky-1337-b1m'
}
const otherGuy = {
name: 'Some Other DUde',
email: 'otherguy@gmail.com',
email: 'otherguy@example.org',
password: 'sn3aky-1337-b1m'
}

Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/core/tests/generic.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,12 @@ describe('Generic AuthN & AuthZ controller tests', () => {
}
const serverOwner = {
name: 'Itsa Me',
email: 'me@gmail.com',
email: 'me@example.org',
password: 'sn3aky-1337-b1m'
}
const otherGuy = {
name: 'Some Other DUde',
email: 'otherguy@gmail.com',
email: 'otherguy@example.org',
password: 'sn3aky-1337-b1m'
}

Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/core/tests/objects.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ const getObjects = getStreamObjectsFactory({ db })
describe('Objects @core-objects', () => {
const userOne = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie43@gmail.com',
email: 'didimitrie43@example.org',
password: 'sn3aky-1337-b1m'
}

Expand Down
4 changes: 2 additions & 2 deletions packages/server/modules/core/tests/streams.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,14 @@ const createObject = createObjectFactory({
describe('Streams @core-streams', () => {
const userOne: BasicTestUser = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie@gmail.com',
email: 'didimitrie@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}

const userTwo: BasicTestUser = {
name: 'Dimitrie Stefanescu 2',
email: 'didimitrie2@gmail.com',
email: 'didimitrie2@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down
2 changes: 1 addition & 1 deletion packages/server/modules/core/tests/users.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ const createObject = createObjectFactory({
describe('Actors & Tokens @user-services', () => {
const myTestActor = {
name: 'Dimitrie Stefanescu',
email: 'didimitrie@gmail.com',
email: 'didimitrie@example.org',
password: 'sn3aky-1337-b1m',
id: ''
}
Expand Down
8 changes: 4 additions & 4 deletions packages/server/modules/core/tests/usersAdminList.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ async function getOrderedUserIds() {
describe('[Admin users list]', () => {
const me = {
name: 'Mr Server Admin Dude',
email: 'adminuserguy@gmail.com',
email: 'adminuserguy@example.org',
password: 'sn3aky-1337-b1m',
id: undefined as Optional<string>,
verified: false
Expand Down Expand Up @@ -197,7 +197,7 @@ describe('[Admin users list]', () => {
name: `User #${i} - ${
remainingSearchQueryUserCount-- >= 1 ? SEARCH_QUERY : ''
}`,
email: `speckleuser${i}@gmail.com`,
email: `speckleuser${i}@example.org`,
password: 'sn3aky-1337-b1m',
verified: false
})
Expand Down Expand Up @@ -225,7 +225,7 @@ describe('[Admin users list]', () => {
{
email: `randominvitee${i}.${
remainingSearchQueryInviteCount-- >= 1 ? SEARCH_QUERY : ''
}@gmail.com`
}@example.org`
},
randomEl(userIds)
)
Expand All @@ -237,7 +237,7 @@ describe('[Admin users list]', () => {
const { id: streamId, ownerId } = randomEl(streamData)
const email = `streamrandominvitee${i}.${
remainingSearchQueryInviteCount-- >= 1 ? SEARCH_QUERY : ''
}@gmail.com`
}@example.org`

await createInviteDirectly(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('FileUploads @fileuploads', () => {

const userOne = {
name: 'User',
email: 'user@gmail.com',
email: 'user@example.org',
password: 'jdsadjsadasfdsa'
}

Expand Down
Loading
Loading