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

Deduplicate email invites (SEC#19) #2543

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Deduplicate email invites (SEC#19) #2543

merged 8 commits into from
Jul 31, 2023

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jul 31, 2023

Description

Deduplicate the list of external invites in the API call.
To achieve this, we need a canonical form of the Email addresses and for Gmail, we need to remove the "dot trick" aliases.

add getCanonicalEmail function to db/utils

deduplicate invitees

unit tests for getCanonicalEmail function

add test for deduplication of invitees on the api

Tests:

db utils
    ✔ should export an object
    getCanonicalEmail
      ✔ should be a function
      ✔ should return a string
      ✔ should return null if email is null
      ✔ should return null if email is empty
      ✔ should return a lower case email
      ✔ should trim an email
      ✔ should remove dots from gmail addresses by default
      ✔ should not remove dots from NON gmail address by default
      ✔ should remove dots from other email domains if specified

  Team Invitations API
    Create an invitation
      ✔ deduplicate user invites
      external users
        ✔ deduplicate emails and gmail "dot-trick" aliases in list of external invites

Related Issue(s)

https://github.com/flowforge/security/issues/19

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl requested a review from hardillb July 31, 2023 10:47
@hardillb
Copy link
Contributor

hardillb commented Jul 31, 2023

@Steve-Mcl looks like one of your new tests has failed

Sorry, the change has broken old tests.

@Steve-Mcl
Copy link
Contributor Author

yeah, trying to understand why - I suppose thats why we do do tests tho right :)

@Steve-Mcl
Copy link
Contributor Author

that took some doing.

I inadvertently broke tests in a different area - turns out my deduplication code was not as well behaved or accurate enough.

I have added additional test for ensuring deduplication is case insentitive on users too.

All tests pass locally. 🤞

@hardillb
Copy link
Contributor

@Steve-Mcl still failing

@Steve-Mcl
Copy link
Contributor Author

yup - grrr.

my test foo is broken - easer one this time

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #2543 (bfb5c29) into main (0483922) will increase coverage by 0.00%.
Report is 12 commits behind head on main.
The diff coverage is 85.18%.

@@           Coverage Diff           @@
##             main    #2543   +/-   ##
=======================================
  Coverage   74.63%   74.63%           
=======================================
  Files         228      228           
  Lines        9129     9164   +35     
  Branches     1879     1891   +12     
=======================================
+ Hits         6813     6840   +27     
- Misses       2316     2324    +8     
Flag Coverage Δ
backend 74.63% <85.18%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
forge/routes/api/teamInvitations.js 72.05% <75.00%> (-0.67%) ⬇️
forge/db/utils.js 83.33% <100.00%> (+3.74%) ⬆️

... and 7 files with indirect coverage changes

@Steve-Mcl
Copy link
Contributor Author

@hardillb third time is the charm

@hardillb hardillb merged commit 929a774 into main Jul 31, 2023
4 checks passed
@hardillb hardillb deleted the SEC19-dedup-invites branch July 31, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants