Skip to content

Added an ability to allow attester search only for domains.| #1827#1831

Merged
tomholub merged 4 commits intomasterfrom
issue_1827_allow_attester_search_only_for_domains
Jun 7, 2022
Merged

Added an ability to allow attester search only for domains.| #1827#1831
tomholub merged 4 commits intomasterfrom
issue_1827_allow_attester_search_only_for_domains

Conversation

@DenBond7
Copy link
Collaborator

@DenBond7 DenBond7 commented Jun 6, 2022

This PR added an ability to allow attester search only for domains

close #1827


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

@DenBond7 DenBond7 marked this pull request as ready for review June 7, 2022 07:40
@DenBond7 DenBond7 requested a review from IvanPizhenko as a code owner June 7, 2022 07:40
@DenBond7 DenBond7 requested a review from tomholub June 7, 2022 07:40
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

looks good, but see comment - there are now two ways to disable attester completely

Eventually we'll remove the * method

What exactly does disallowLookupOnAttester do that canLookupThisRecipientOnAttester wouldn't already do?

Comment on lines +214 to +215
return allowAttesterSearchOnlyForDomains == null
&& (disallowAttesterSearchForDomains ?: emptyList()).contains("*")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add (original condition) || (allowAttesterSearchOnlyForDomains != null && allowAttesterSearchOnlyForDomains.isEmpty)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eventually we'll remove the * method

Maybe will be better to remove disallowLookupOnAttester in that case. Please look at #1831 (comment)

@tomholub
Copy link
Collaborator

tomholub commented Jun 7, 2022

What exactly does disallowLookupOnAttester do that canLookupThisRecipientOnAttester wouldn't already do?

maybe that's for fingerprint lookup, but do we still support that?

@DenBond7
Copy link
Collaborator Author

DenBond7 commented Jun 7, 2022

maybe that's for fingerprint lookup, but do we still support that?

@tomholub we use this code here

} else if (orgRules?.disallowLookupOnAttester() == true) {

      if (identData.isValidEmail()) {
        val wkdResult = getResult(requestCode = requestCode) {
          val pgpPublicKeyRingCollection = WkdClient.lookupEmail(context, identData)

          //For now, we just peak at the first matching key. It should be improved inthe future.
          // See more details here https://github.com/FlowCrypt/flowcrypt-android/issues/480
          val firstMatchingKey = pgpPublicKeyRingCollection?.firstOrNull {
            KeyRingInfo(it)
              .getEncryptionSubkeys(EncryptionPurpose.ANY)
              .isNotEmpty()
          }
          firstMatchingKey?.armor()?.let { armoredPubKey ->
            Response.success(armoredPubKey)
          } ?: Response.error(HttpURLConnection.HTTP_NOT_FOUND, "Not found".toResponseBody())
        }

        if (wkdResult.status == Result.Status.SUCCESS && wkdResult.data?.isNotEmpty() == true) {
          return@withContext resultWrapperFun(wkdResult)
        }

        if (orgRules?.canLookupThisRecipientOnAttester(identData) == false) {
          return@withContext Result.success(
            requestCode = requestCode,
            data = PubResponse(null, null)
          )
        }
      } else if (orgRules?.disallowLookupOnAttester() == true) { // <------- HERE
        return@withContext Result.success(
          requestCode = requestCode,
          data = PubResponse(null, null)
        )
      }

      val apiService = ApiHelper.getInstance(context).retrofit.create(ApiService::class.java)
      val result = getResult(requestCode = requestCode) { apiService.getPubFromAttester(identData) }
      return@withContext resultWrapperFun(result)

But maybe it's not relevant today. It may relay to the following #1399 and #1201 (comment)

@tomholub
Copy link
Collaborator

tomholub commented Jun 7, 2022

I suspect maybe this branch is never called anymore

      } else if (orgRules?.disallowLookupOnAttester() == true) { // <------- HERE
        return@withContext Result.success(
          requestCode = requestCode,
          data = PubResponse(null, null)
        )
      }

because when getting updated public keys, we no longer do it by fingerprint. We search them again on email, regardless of source (wkd or attester). I don't think we have any search by fingerprint functionality in the app anymore.

@DenBond7
Copy link
Collaborator Author

DenBond7 commented Jun 7, 2022

I don't think we have any search by fingerprint functionality in the app anymore.

Agree. Currently we have identData = email over the whole app

@DenBond7 DenBond7 marked this pull request as draft June 7, 2022 12:04
@DenBond7 DenBond7 marked this pull request as ready for review June 7, 2022 13:01
@DenBond7 DenBond7 requested a review from tomholub June 7, 2022 13:02
@tomholub tomholub merged commit 838daa1 into master Jun 7, 2022
@tomholub tomholub deleted the issue_1827_allow_attester_search_only_for_domains branch June 7, 2022 13:03
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.

add client configuration allow_attester_search_only_for_domains

2 participants