Skip to content

Added handling disallow_attester_search_for_domains OrgRule - #1387

Merged
IvanPizhenko merged 3 commits into
masterfrom
issue_1203_disallow_attester_search_for_domains
Aug 9, 2021
Merged

Added handling disallow_attester_search_for_domains OrgRule#1387
IvanPizhenko merged 3 commits into
masterfrom
issue_1203_disallow_attester_search_for_domains

Conversation

@DenBond7

Copy link
Copy Markdown
Collaborator

This PR added handling disallow_attester_search_for_domains OrgRule

close#1203


Tests(delete all except exactly one):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

@DenBond7DenBond7 added the PR submitted PR is submitted for this issue label Aug 5, 2021
@DenBond7DenBond7 added this to the 1.2.0: More OrgRules milestone Aug 5, 2021
@DenBond7DenBond7 removed the PR submitted PR is submitted for this issue label Aug 6, 2021
@DenBond7DenBond7 closed this Aug 6, 2021
@DenBond7
DenBond7force-pushed the issue_1203_disallow_attester_search_for_domains branch from 9f1b659 to 8d0fe16CompareAugust 6, 2021 07:12
@DenBond7DenBond7 reopened this Aug 6, 2021
@DenBond7
DenBond7 marked this pull request as ready for review August 6, 2021 13:37

@IvanPizhenkoIvanPizhenko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have a question

throw IllegalStateException("Not a valid email $emailAddr")
}

return !disallowedDomains.contains(userDomain)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

doesn't this require sort of case-insensitive comparison?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In typescript we have:

publiccanLookupThisRecipientOnAttester=(emailAddr: string): boolean=>{
....constuserDomain=Str.getDomainFromEmailAddress(emailAddr);if(!userDomain){thrownewError(`Not a valid email ${emailAddr}`);}return!disallowedDomains.includes(userDomain);}

and then:

publicstaticgetDomainFromEmailAddress=(emailAddr: string)=>{// todo: parseEmail()?returnemailAddr.toLowerCase().split('@')[1];}

So function for getting domain in the typescript casts it to lowercase and then if you configure everything in lowercase, all works.
In Kotlin that doesn't happen:

// from EmailUtil.ktfungetDomain(email:String): String {
returnwhen {
TextUtils.isEmpty(email) ->""
email.contains("@") -> email.substring(email.indexOf('@') +1)
else->""
}
}

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I see. Thank you. I will fix it.

@tomholub

Copy link
Copy Markdown
Collaborator

Well noticed, Ivan - thank you.

@IvanPizhenko
IvanPizhenko merged commit 4771ba0 into masterAug 9, 2021
@IvanPizhenko
IvanPizhenko deleted the issue_1203_disallow_attester_search_for_domains branch August 9, 2021 20:57
Sign up for freeto 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.

implement disallow_attester_search_for_domains OrgRule

3 participants

@DenBond7@tomholub@IvanPizhenko