Skip to content

Feature/issue 530 google grant#1043

Merged
tomholub merged 22 commits intomasterfrom
feature/issue-530-google-grant
Nov 23, 2021
Merged

Feature/issue 530 google grant#1043
tomholub merged 22 commits intomasterfrom
feature/issue-530-google-grant

Conversation

@sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Nov 19, 2021

This PR separates asking for mail and contacts permissions.

close #530


Tests (delete all except exactly one):

  • Difficult to test (explain why) - it's difficult to revoke given permissions through iOS simulator to be able to test sign up process.

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
  • is documented clearly and usefully, or doesn't need documentation

@sosnovsky sosnovsky requested a review from tomholub November 19, 2021 13:45
@sosnovsky sosnovsky marked this pull request as ready for review November 19, 2021 13:46
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 - sharing general code feedback. Didn't run it yet

@tomholub
Copy link
Collaborator

Tried to log in, saw the "Gmail" permissing unchecked. Left it unchecked and proceeded. No visual feedback in the app, this was in the logs:

"ℹ️[App Start][GlobalRouter] Sign in with gmailLogin(<FlowCrypt.SignInViewController: 0x13a852400>)"
"ℹ️[App Start][GlobalRouter] gmail login failed with error The operation couldn’t be completed. (FlowCrypt.GoogleUserServiceError error 1.)"
"ℹ️[App Start][GlobalRouter] Sign in with gmailLogin(<FlowCrypt.SignInViewController: 0x13a852400>)"
"ℹ️[App Start][GlobalRouter] gmail login failed with error The operation couldn’t be completed. (org.openid.appauth.general error -3.)"

@tomholub
Copy link
Collaborator

However, on the first prompt it was still showing 3 other permissions + checkbox for Gmail.

Could we split it into two steps? In first step, grant 3 other permissions. In second step, grant Gmail (which will then be without checkbox).

@sosnovsky
Copy link
Collaborator Author

In the case when user doesn't check mail checkbox on the first auth screen app should show next screen with message about "Inbox access", and then show Google auth screen only with mail screen. Here how it works for me:

Simulator.Screen.Recording.-.iPhone.12.-.2021-11-19.at.22.28.06.mp4

We can remove mail permission from the first step, I left it as described here #530 (comment) - so if user grants mail permission on the first step then no need for the second step, and user goes straight to Inbox.

@tomholub
Copy link
Collaborator

Got it. I'll re-test

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.

code good - need to retry manual testing

@tomholub
Copy link
Collaborator

Same (unchecked gmail)

"ℹ️[App Start][GlobalRouter] Sign in with gmailLogin(<FlowCrypt.SignInViewController: 0x10a85b600>)"
"ℹ️[App Start][GlobalRouter] gmail login failed with error The operation couldn’t be completed. (FlowCrypt.GoogleUserServiceError error 1.)"

@tomholub
Copy link
Collaborator

From your testing - "other contacts" don't include regular contacts? We need both?

image

@tomholub
Copy link
Collaborator

Currently, if I only choose one of the two, it will not tell me that I did something wrong and yet it will not search contacts (it seems). First though, need to confirm we really need both. And before that will need to debug the issue I'm having when logging in. Maybe add some more logs?

@sosnovsky
Copy link
Collaborator Author

Other contacts are contacts which automatically created by Google and not stored in user's contacts (#844). So we need both scopes for full contacts search.

Currently, if I only choose one of the two, it will not tell me that I did something wrong and yet it will not search contacts (it seems). First though, need to confirm we really need both.

I'm working on adding popup with error message as you've described here #1043 (comment), then users will see message about missing scope.

And before that will need to debug the issue I'm having when logging in. Maybe add some more logs?

Yes, it's strange as I didn't notice any issue with unchecked mail permission during testing - it always redirected me to Inbox permission screen.

Did you remove Flowcrypt iOS App from Third-party apps with account access list at https://myaccount.google.com/u/2/permissions before testing? Maybe it redirects you to the next screen because you already gave access to your Inbox?

@tomholub
Copy link
Collaborator

tomholub commented Nov 21, 2021

Did you remove Flowcrypt iOS App from Third-party apps with account access list at https://myaccount.google.com/u/2/permissions before testing? Maybe it redirects you to the next screen because you already gave access to your Inbox?

I did remove it - else I wouldn't see the grant screen at all.

Once I've seen the error once and try again, it only asks for Gmail auth (that means from google's perspective it was successfully granted) and proceeds successfully.

@tomholub tomholub marked this pull request as draft November 21, 2021 19:49
@tomholub
Copy link
Collaborator

Marking as draft until resolved

# Conflicts:
#	FlowCrypt/Extensions/UIViewControllerExtensions.swift
#	FlowCrypt/Functionality/Services/GlobalRouter.swift
@sosnovsky
Copy link
Collaborator Author

@tomholub I added handling for contacts permission errors and also updated logging to print more detailed messages.
Can you please check permissions flow now? It'll be useful if you'll be able to record video of it, if something goes wrong

@tomholub
Copy link
Collaborator

updated logs:

"ℹ️[App Start][GlobalRouter] Sign in with gmailLogin(<FlowCrypt.SignInViewController: 0x10e8c9000>)"
"ℹ️[App Start][GlobalRouter] gmail login failed with error Missing scopes error: Gmail"

I'm testing by going to a menu and then adding a secondary account - sorry for not mentioning it earlier. It does work properly when I just tried it when adding a first account, issue can be reproduced when adding additional accounts (and leaving out one scope) - please see if you also observe the same.

@sosnovsky
Copy link
Collaborator Author

@tomholub I reproduced this issue with secondary account adding, it's fixed now ✅

@sosnovsky sosnovsky marked this pull request as ready for review November 22, 2021 21:38
tomholub
tomholub previously approved these changes Nov 23, 2021
@tomholub
Copy link
Collaborator

@sosnovsky all good but tests seem unhappy

tomholub
tomholub previously approved these changes Nov 23, 2021
@sosnovsky
Copy link
Collaborator Author

It seems UI tests for other PRs and master are failing too, maybe your tests timeout increase will fix it.

tomholub
tomholub previously approved these changes Nov 23, 2021
@tomholub tomholub merged commit 51f5cf5 into master Nov 23, 2021
@tomholub tomholub deleted the feature/issue-530-google-grant branch November 23, 2021 15:49
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.

google grant checkboxes unticked

2 participants