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.

Sign up for GitHub

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

Jump to bottom

[server] Stop signup for blocklisted email domains #9385

Merged
merged 3 commits into from
Apr 19, 2022
Merged

[server] Stop signup for blocklisted email domains #9385

merged 3 commits into from
Apr 19, 2022

Conversation

geropl
Copy link
Member

@geropl geropl commented Apr 18, 2022 •
edited by jankeromnes
Loading

Description

Related Issue(s)

Fixes #6285

How to test

Release Notes

re-enables blocklisting of email domains

Documentation

@geropl geropl requested a review from a team April 18, 2022 20:02
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 18, 2022
@geropl geropl force-pushed the gpl/6285-edomains branch from 06f3210 to 2a11329 Compare April 19, 2022 07:04
geropl
geropl commented Apr 19, 2022
View reviewed changes
components/server/src/analytics.ts
@@ -34,6 +34,7 @@ export async function trackSignup(user: User, request: Request, analytics: IAnal
properties: {
auth_provider: user.identities[0].authProviderId,
qualified: !!request.cookies["gitpod-marketing-website-visited"],
blocked: user.blocked,
Copy link
Member Author

Choose a reason for hiding this comment

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

@jakobhero I added this property. Does that require any work from your side?

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @geropl, thanks for looping me in! this does not cause any issues downstream, i will update the tracking plan with the additional property.

@jankeromnes jankeromnes self-assigned this Apr 19, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Stealing this review as Alex is on holidays 😁

@jankeromnes jankeromnes changed the title [server] Stop signup for blacklisted email domains [server] Stop signup for blocklisted email domains Apr 19, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

I did not get blocked 😬 (even though my /account shows a @gitpod.io primary email address)

Did this accidentally get redeployed, or am I doing something wrong?

@csweichel
Copy link
Contributor

@jankeromnes please check the DB to see if the branch is properly set up. @geropl, @aledbf and I are currently busy blocking miners. This would really really help.

Can you try and push this change forward as quickly as possible?

@jankeromnes
Copy link
Contributor

Okay, given the urgency, will push this forward.

@roboquat roboquat added size/S and removed size/XS labels Apr 19, 2022
@csweichel
Copy link
Contributor

csweichel commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-gpl-6285-edomains.5

@geropl
Copy link
Member Author

geropl commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-gpl-6285-edomains.6

@geropl
Copy link
Member Author

geropl commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-gpl-6285-edomains.8

@geropl
Copy link
Member Author

geropl commented Apr 19, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-gpl-6285-edomains.9

🤞

jankeromnes
jankeromnes approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Code looks good and works as intended!

But we should really rename filter to something much less subtle. 🤪

components/server/src/user/user-service.ts
@@ -142,7 +152,8 @@ export class UserService {
this.config.blockNewUsers.passlist.some((e) => mail.endsWith(`@${e}`));
const canPass = newUser.identities.some((i) => !!i.primaryEmail && emailDomainInPasslist(i.primaryEmail));

newUser.blocked = !canPass;
// blocked = if user already blocked OR is not allowed to pass
newUser.blocked = newUser.blocked || !canPass;
Copy link
Contributor

Choose a reason for hiding this comment

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

you-shall-not-pass

components/server/ee/src/auth/email-domain-service.ts
@@ -25,7 +25,7 @@ export class EMailDomainServiceImpl implements EMailDomainService {

async isBlocked(email: string): Promise<boolean> {
const { domain } = this.parseMail(email);
return !this.domainFilterDb.filter(domain);
return !(await this.domainFilterDb.filter(domain));
Copy link
Contributor

Choose a reason for hiding this comment

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

😵

@roboquat roboquat merged commit 1987cb3 into main Apr 19, 2022
@roboquat roboquat deleted the gpl/6285-edomains branch April 19, 2022 14:45
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobhero jakobhero jakobhero left review comments

@AlexTugarev AlexTugarev AlexTugarev left review comments

@jankeromnes jankeromnes jankeromnes approved these changes

Assignees

@jankeromnes jankeromnes

Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filtered email domains are not working
6 participants
@geropl @jankeromnes @csweichel @AlexTugarev @jakobhero @roboquat