Skip to content

Comments

fix: duplicate username when inviting to team#13478

Closed
AnthonyJHizon wants to merge 2 commits intocalcom:mainfrom
AnthonyJHizon:fix/duplicate-username-when-inviting-to-team
Closed

fix: duplicate username when inviting to team#13478
AnthonyJHizon wants to merge 2 commits intocalcom:mainfrom
AnthonyJHizon:fix/duplicate-username-when-inviting-to-team

Conversation

@AnthonyJHizon
Copy link

@AnthonyJHizon AnthonyJHizon commented Jan 31, 2024 •
edited
Loading

What does this PR do?

Fixes 500 case when duplicate username when inviting to team

Approach: Uses '@' instead of '-' to separate name and domain name for non auto accepted email domains.

Bug was caused by cases:
Email 1, Email 2
adam-gmail@autoAcceptedEmailDomain.com, adam@gmail.com
Generated usernames: adam-gmail, adam-gmail

Email 1, Email 2
joe-flowers@autoAcceptedEmailDomain.com, joe@flowers.com
Generated usernames: joe-flowers, joe-flowers

Since '-' is a valid character in an email address.
Consider using '@' as a delimiter when identifying non auto accepted email domains.

Limited fix to only when inserting to database.
Might cause mismatch when comparing username queried from database and slugify(username)

Fixes #13464

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@vercel
Copy link

vercel bot commented Jan 31, 2024

@AnthonyJHizon is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added Medium priority Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Jan 31, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2024 •
edited
Loading

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2024 •
edited
Loading

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

Seventy-two Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load % of Budget (350 KB)
/[user]/[type] 264.09 KB 452.19 KB 129.20% (🟢 -0.29%)
/[user]/[type]/embed 264.09 KB 452.19 KB 129.20% (🟢 -0.29%)
/apps 271.36 KB 459.46 KB 131.27% (🟢 -2.24%)
/apps/[slug] 288.87 KB 476.97 KB 136.28% (🟢 -2.25%)
/apps/[slug]/[...pages] 546.36 KB 734.46 KB 209.85% (🟢 -0.29%)
/apps/categories 249.04 KB 437.14 KB 124.90% (🟢 -2.25%)
/apps/categories/[category] 253.35 KB 441.45 KB 126.13% (🟢 -2.25%)
/apps/installed/[category] 272.45 KB 460.55 KB 131.58% (🟢 -2.24%)
/auth/setup 150.52 KB 338.62 KB 96.75% (🟡 +0.28%)
/availability/troubleshoot 171.87 KB 359.97 KB 102.85% (🟢 -0.24%)
/booking/[uid] 190.98 KB 379.08 KB 108.31% (🟡 +0.23%)
/booking/[uid]/embed 190.98 KB 379.08 KB 108.31% (🟡 +0.23%)
/d/[link]/[slug] 263.91 KB 452.01 KB 129.15% (🟢 -0.29%)
/enterprise 249.51 KB 437.61 KB 125.03% (🟢 -2.25%)
/more 248.67 KB 436.77 KB 124.79% (🟢 -2.25%)
/org/[orgSlug] 229.78 KB 417.88 KB 119.40% (🟢 -2.05%)
/org/[orgSlug]/[user] 236.25 KB 424.35 KB 121.24% (🟢 -2.02%)
/org/[orgSlug]/[user]/[type] 264.29 KB 452.39 KB 129.25% (🟢 -0.28%)
/org/[orgSlug]/[user]/[type]/embed 264.31 KB 452.41 KB 129.26% (🟢 -0.29%)
/org/[orgSlug]/[user]/embed 236.28 KB 424.38 KB 121.25% (🟢 -2.02%)
/org/[orgSlug]/embed 229.8 KB 417.9 KB 119.40% (🟢 -2.05%)
/org/[orgSlug]/instant-meeting/team/[slug]/[type] 263.93 KB 452.03 KB 129.15% (🟢 -0.29%)
/org/[orgSlug]/team/[slug] 229.79 KB 417.89 KB 119.40% (🟢 -2.05%)
/org/[orgSlug]/team/[slug]/[type] 263.96 KB 452.06 KB 129.16% (🟢 -0.29%)
/settings/admin 255.35 KB 443.45 KB 126.70% (🟢 -2.25%)
/settings/admin/apps 268.62 KB 456.72 KB 130.49% (🟢 -1.74%)
/settings/admin/apps/[category] 268.6 KB 456.7 KB 130.49% (🟢 -1.74%)
/settings/admin/flags 259.12 KB 447.22 KB 127.78% (🟢 -2.26%)
/settings/admin/impersonation 255.65 KB 443.75 KB 126.79% (🟢 -2.26%)
/settings/admin/oAuth 267.37 KB 455.47 KB 130.13% (🟢 -2.25%)
/settings/admin/orgMigrations/_OrgMigrationLayout 248.1 KB 436.21 KB 124.63% (🟢 -2.26%)
/settings/admin/organizations 257.38 KB 445.48 KB 127.28% (🟢 -2.25%)
/settings/admin/organizations/[id]/edit 255.87 KB 443.97 KB 126.85% (🟢 -2.25%)
/settings/admin/users 258.07 KB 446.17 KB 127.48% (🟢 -2.26%)
/settings/billing 255.56 KB 443.66 KB 126.76% (🟢 -2.26%)
/settings/developer/api-keys 259.98 KB 448.08 KB 128.02% (🟢 -2.25%)
/settings/developer/webhooks 259.91 KB 448.01 KB 128.00% (🟢 -2.25%)
/settings/developer/webhooks/[id] 260.96 KB 449.06 KB 128.30% (🟢 -2.25%)
/settings/developer/webhooks/new 261 KB 449.1 KB 128.32% (🟢 -2.26%)
/settings/my-account/calendars 266.35 KB 454.45 KB 129.84% (🟢 -2.25%)
/settings/my-account/conferencing 267.23 KB 455.33 KB 130.09% (🟢 -2.25%)
/settings/my-account/general 343.7 KB 531.8 KB 151.94% (🟢 -2.25%)
/settings/my-account/out-of-office 259.78 KB 447.88 KB 127.97% (🟢 -2.26%)
/settings/my-account/profile 392.66 KB 580.76 KB 165.93% (🟢 -2.10%)
/settings/organizations/[id]/about 151.53 KB 339.63 KB 97.04% (🟢 -0.25%)
/settings/organizations/[id]/add-teams 151.58 KB 339.68 KB 97.05% (🟢 -0.24%)
/settings/organizations/[id]/set-password 151.51 KB 339.61 KB 97.03% (🟢 -0.24%)
/settings/organizations/appearance 278.97 KB 467.08 KB 133.45% (🟢 -2.25%)
/settings/organizations/billing 255.59 KB 443.69 KB 126.77% (🟢 -2.26%)
/settings/organizations/general 336.24 KB 524.34 KB 149.81% (🟢 -2.25%)
/settings/organizations/members 428.09 KB 616.19 KB 176.05% (🟢 -2.25%)
/settings/organizations/new 151.53 KB 339.63 KB 97.04% (🟢 -0.25%)
/settings/organizations/profile 389.68 KB 577.78 KB 165.08% (🟢 -2.09%)
/settings/organizations/teams/other 256.41 KB 444.51 KB 127.00% (🟢 -2.25%)
/settings/organizations/teams/other/[id]/appearance 268.06 KB 456.16 KB 130.33% (🟢 -2.25%)
/settings/organizations/teams/other/[id]/members 262.92 KB 451.02 KB 128.86% (🟢 -2.26%)
/settings/organizations/teams/other/[id]/profile 461.28 KB 649.38 KB 185.54% (🟢 -2.26%)
/settings/security/impersonation 260.67 KB 448.77 KB 128.22% (🟢 -2.25%)
/settings/security/sso 265.65 KB 453.75 KB 129.64% (🟢 -2.25%)
/settings/security/two-factor-auth 264.5 KB 452.6 KB 129.31% (🟢 -2.25%)
/settings/teams 255.09 KB 443.19 KB 126.63% (🟢 -2.26%)
/settings/teams/[id]/appearance 268.05 KB 456.15 KB 130.33% (🟢 -2.26%)
/settings/teams/[id]/billing 255.59 KB 443.7 KB 126.77% (🟢 -2.25%)
/settings/teams/[id]/profile 462.11 KB 650.21 KB 185.77% (🟢 -2.25%)
/settings/teams/[id]/sso 266.18 KB 454.29 KB 129.80% (🟢 -2.26%)
/settings/teams/new 190.58 KB 378.68 KB 108.20% (🟢 -4.67%)
/team/[slug] 229.75 KB 417.85 KB 119.38% (🟢 -2.04%)
/team/[slug]/[type] 263.92 KB 452.03 KB 129.15% (🟢 -0.29%)
/team/[slug]/[type]/embed 263.95 KB 452.05 KB 129.16% (🟢 -0.29%)
/team/[slug]/embed 229.79 KB 417.89 KB 119.40% (🟢 -2.05%)
/teams 248.95 KB 437.05 KB 124.87% (🟢 -2.25%)
/upgrade 249.12 KB 437.22 KB 124.92% (🟢 -2.25%)
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

@Udit-takkar Udit-takkar requested a review from a team January 31, 2024 09:14
@keithwillcode keithwillcode added the community Created by Linear-GitHub Sync label Jan 31, 2024
@keithwillcode keithwillcode added this to the v3.9 milestone Jan 31, 2024
@sean-brydon
Copy link
Member

sean-brydon commented Feb 5, 2024 •
edited
Loading

What does this PR do?

Fixes 500 case when duplicate username when inviting to team

Approach: Uses '@' instead of '-' to separate name and domain name for non auto accepted email domains.

Bug was caused by cases: Email 1, Email 2 adam-gmail@autoAcceptedEmailDomain.com, adam@gmail.com Generated usernames: adam-gmail, adam-gmail

Email 1, Email 2 joe-flowers@autoAcceptedEmailDomain.com, joe@flowers.com Generated usernames: joe-flowers, joe-flowers

Since '-' is a valid character in an email address. Consider using '@' as a delimiter when identifying non auto accepted email domains.

Limited fix to only when inserting to database. Might cause mismatch when comparing username queried from database and slugify(username)

Fixes #13464

Type of change

  • Bug fix (non-breaking change which fixes an issue)

RFC 3986 - https://datatracker.ietf.org/doc/html/rfc3986#section-2.2

I dont have much context on this issue but i dont think @ is a good choice of delimiter to use here.

We rely on our usernames for booking pages -> cal.com/sean-brydon

We can't have cal.com/sean@brydon as @ isnt IIRC a valid middle path segment of a URL. To make this valid we'd need to encode it as %40 which is a rabbit hole i dont think we should go down.

I would probably recommend following one of the delimiters mentioned in the RFC above

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

CC: @zomars

sean-brydon
sean-brydon requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

As per comment

@PeerRich PeerRich modified the milestones: v3.9, v3.8 Feb 5, 2024
@AnthonyJHizon AnthonyJHizon reopened this Feb 5, 2024
@AnthonyJHizon
Copy link
Author

AnthonyJHizon commented Feb 5, 2024

What does this PR do?

Fixes 500 case when duplicate username when inviting to team
Approach: Uses '@' instead of '-' to separate name and domain name for non auto accepted email domains.
Bug was caused by cases: Email 1, Email 2 adam-gmail@autoAcceptedEmailDomain.com, adam@gmail.com Generated usernames: adam-gmail, adam-gmail
Email 1, Email 2 joe-flowers@autoAcceptedEmailDomain.com, joe@flowers.com Generated usernames: joe-flowers, joe-flowers
Since '-' is a valid character in an email address. Consider using '@' as a delimiter when identifying non auto accepted email domains.
Limited fix to only when inserting to database. Might cause mismatch when comparing username queried from database and slugify(username)
Fixes #13464

Type of change

  • Bug fix (non-breaking change which fixes an issue)

RFC 3986 - https://datatracker.ietf.org/doc/html/rfc3986#section-2.2

I dont have much context on this issue but i dont think @ is a good choice of delimiter to use here.

We rely on our usernames for booking pages -> cal.com/sean-brydon

We can't have cal.com/sean@brydon as @ isnt IIRC a valid middle path segment of a URL. To make this valid we'd need to encode it as %40 which is a rabbit hole i dont think we should go down.

I would probably recommend following one of the delimiters mentioned in the RFC above

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

CC: @zomars

I was not aware the usernames were used to generate page links for bookings.
All unreserved characters are valid in an email address so there's a chance this bug will reoccur if we use them as delimiters.
What do you think about an approach where all usernames are generated by the same logic and we introduce a value to the Membership Table to identify auto accepted email domains? With this approach we might also have to consider creating an indicator for auto accepted members in the frontend as well.

zomars
zomars suggested changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@zomars zomars left a comment

Choose a reason for hiding this comment

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

I'm going to agree with @sean-brydon here. We don't want @ in URLs in here.

@keithwillcode keithwillcode marked this pull request as draft February 6, 2024 09:39
@zomars
Copy link
Contributor

zomars commented Feb 12, 2024

Closing for now. Feel free to re-open. 🙏🏽

@zomars zomars closed this Feb 12, 2024
@dosubot dosubot bot modified the milestones: v3.8, v4.1 Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sean-brydon sean-brydon sean-brydon requested changes

+1 more reviewer

@zomars zomars zomars requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🐛 bug Something isn't working community Created by Linear-GitHub Sync foundation Medium priority Created by Linear-GitHub Sync

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CAL-3055] 500 when inviting a team member and there's a conflict in the generated username

5 participants

@AnthonyJHizon @sean-brydon @zomars @keithwillcode @PeerRich