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

implement disallow_attester_search_for_domains OrgRule #1203

Closed
tomholub opened this issue Apr 28, 2021 · 5 comments · Fixed by #1387
Closed

implement disallow_attester_search_for_domains OrgRule #1203

tomholub opened this issue Apr 28, 2021 · 5 comments · Fixed by #1387
Assignees

Comments

@tomholub
Copy link
Collaborator

tomholub commented Apr 28, 2021

part of #1298

including wildcard * support meaning don't search on attester at all

@tomholub tomholub added this to the later milestone Apr 28, 2021
@tomholub tomholub mentioned this issue Jun 21, 2021
6 tasks
@tomholub tomholub modified the milestones: later, soon Jun 21, 2021
@tomholub tomholub modified the milestones: soon, 1.2.0 Jul 21, 2021
@tomholub
Copy link
Collaborator Author

tomholub commented Jul 21, 2021

Here is the implementation in OrgRules class:

https://github.com/FlowCrypt/flowcrypt-browser/blob/48cb02a244b71a2acffbbc0fdb6ed4254baa0da3/extension/js/common/org-rules.ts#L170-L192

Here is the implementation in public key lookup code - only affects lookup from Attester:

(search by email) https://github.com/FlowCrypt/flowcrypt-browser/blob/48cb02a244b71a2acffbbc0fdb6ed4254baa0da3/extension/js/common/api/key-server/attester.ts#L22-L28

(search by fingerprint) https://github.com/FlowCrypt/flowcrypt-browser/blob/48cb02a244b71a2acffbbc0fdb6ed4254baa0da3/extension/js/common/api/key-server/attester.ts#L38-L51

Right now, on Android, Attester is the only source of public keys. However, after #1201 , there will be two sources: WKD and Attester. This OrgRule here will only affects Attester, and will not affect WKD.

@DenBond7
Copy link
Collaborator

DenBond7 commented Aug 5, 2021

@tomholub Could you clarify what place should I change. As I understand, I should modify the logic on the following screen

image

@DenBond7
Copy link
Collaborator

DenBond7 commented Aug 5, 2021

It can be done after #1382 will be merged

@tomholub
Copy link
Collaborator Author

tomholub commented Aug 5, 2021

The behavior throughout the app needs to be modified. Any place that calls flowcrypt.com/attester. Instead of changing several places, you should just change the API client class instead.

On browser extension, we have a PubLookup class which will call Wkd class first, then Attester. Every time we need to look up public keys externally, we use the general PubLookup. Then the logic to skip Attester depending on the domain is there.

While this updates the behavior of the compose message view, the logic should be in the public key lookup classes themselves.

@DenBond7
Copy link
Collaborator

DenBond7 commented Aug 6, 2021

On browser extension, we have a PubLookup class which will call Wkd class first, then Attester. Every time we need to look up public keys externally, we use the general PubLookup. Then the logic to skip Attester depending on the domain is there.

I understand the described logic. I've alredy added support of disallow_attester_search_for_domains. But it looks a little different due to the existing approaches for Android and the app. In the Android app, the main point for API calls is FlowcryptApiRepository. Maybe when I will see the full logic with WKD I will try to make it looks closer to your realization.

DenBond7 added a commit that referenced this issue Aug 6, 2021
DenBond7 added a commit that referenced this issue Aug 9, 2021
IvanPizhenko pushed a commit that referenced this issue Aug 9, 2021
* Added handling 'disallow_attester_search_for_domains' OrgRule.| #1203

* Added tests.| #1203

* Fixed the case sensitive issue.| #1203
@DenBond7 DenBond7 mentioned this issue Aug 10, 2021
1 task
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 a pull request may close this issue.

2 participants