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

OAuth 2.0 end-to-end experience #267

Closed
michaelstingl opened this issue Feb 10, 2019 · 24 comments
Closed

OAuth 2.0 end-to-end experience #267

michaelstingl opened this issue Feb 10, 2019 · 24 comments
Assignees
Labels
Design Estimation - 3 (M) 3 points Login p2-high Escalation, on top of current planning, release blocker
Milestone

Comments

@michaelstingl
Copy link
Contributor

Screenshot 1 Screenshot 2 Screenshot 3 Screenshot 4
image image image image

Problem 1

  • Regular users don't understand why the 2nd screen (see Screenshot 1) after entering the server URL is useful
  • We could explain this feature, and users should have an option to skip this screen in the future

@felix-schwarz @mneuwert @hosy

Problem 2

  • Name of the app is different
  • In the warning message, it says "ownCloud" Wants to Use… (see Screenshot 2)
  • The OAuth 2.0 login screen says The application "iOS" is requesting… (see Screenshot 3 & Screenshot 4)

@DeepDiver1975 we should rename it in the OAuth 2.0 app to "ownCloud iOS app".

@felix-schwarz Any chance it could say "ownCloud app" or "ownCloud iOs app" in the warning message?

Steps to reproduce

  1. Install OAuth 2.0 master (40fe0ab) on ownCloud server
  2. Add account to the ownCloud iOS app

Client

iOS version:
iOS 12.1.4 (16D57)

ownCloud app version:
ownCloud beta version 1.0 build 105 (https://github.com/owncloud/ios-app/tree/b19e57b2f67234460ae6dec8f9587d62ed34d2f0, built from branch)

Device model:
Model: iPhone XS Max

Server configuration

ownCloud version:
ownCloud server 10.1.0 with master-OAuth 2.0 (40fe0ab) on ownCloud server

/cc @hodyroff

@michaelstingl michaelstingl added Design Login p2-high Escalation, on top of current planning, release blocker labels Feb 10, 2019
@hosy
Copy link
Collaborator

hosy commented Feb 21, 2019

Problem 1

  • Regular users don't understand why the 2nd screen (see Screenshot 1) after entering the server URL is useful
  • We could explain this feature, and users should have an option to skip this screen in the future

@felix-schwarz @mneuwert @hosy

This is happening because OAuth2 uses SFAuthenticationSession by default instead of SFSafariViewController. There is unfortunately nothing we can do at the moment to either change or remove these alerts when using SFAuthenticationSession.

@michaelstingl
Copy link
Contributor Author

@hosy Problem 1 ==> Screenshot 1. This isn't about the alert popping up.

@hosy
Copy link
Collaborator

hosy commented Feb 21, 2019

@hosy Problem 1 ==> Screenshot 1. This isn't about the alert popping up.

Maybe this have something to do with the server certificate. I think @felix-schwarz could have an answer for this step.

@hosy
Copy link
Collaborator

hosy commented Feb 22, 2019

@michaelstingl a possible solution could look like this examples:

  • More details for certificate validation
  • Explanation for the user, why an alert will be shown in the next step
Certificate passed validation Certificate validation failed
simulator screen shot - iphone xr - 2019-02-22 at 11 36 41 simulator screen shot - iphone xr - 2019-02-22 at 11 31 52

@michaelstingl
Copy link
Contributor Author

michaelstingl commented Feb 22, 2019

@hosy Yeah, I like! 👍 But: color as only differentiator doesn't work for colorblind user.

Suggestions:

  • Find something better to indicate valid connection or warning
  • Improve the first text below
  • Improve the explanation about the next step

I was also thinking about an option to skip this step in the future, but this will clutter too…

@hosy
Copy link
Collaborator

hosy commented Mar 18, 2019

@michaelstingl here you can see a new draft for this view.

  • Certificate state is shown in Certificate Details row
  • Short explanation for the certificate state as footer text
  • Info panel as footer view to give the user a description for the next oAuth step

Left screenshot has info panel on top
Right screenshot has info panel below

Certificate passed validation Certificate validation failed
Simulator Screen Shot - iPhone X - 2019-03-18 at 17 33 42 Simulator Screen Shot - iPhone X - 2019-03-18 at 17 21 58

@michaelstingl
Copy link
Contributor Author

Left screenshot has info panel on top

My gut feeling would favorite the left one (info on top)

hosy added a commit that referenced this issue Mar 19, 2019
…nformations for OAuth 2 login

- added additional informations for certificate validation as label and text
@jesmrec
Copy link
Contributor

jesmrec commented Mar 20, 2019

Let's go with a bit of QA here

@jesmrec
Copy link
Contributor

jesmrec commented Mar 20, 2019

In the message to clarify the warning, there is a small typo.

"If you 'Continue', you will be prompted to allow the '%@' App to open OAuth 2 login where you can enter your credentials."

"OAuth 2" => "OAuth2"

@jesmrec
Copy link
Contributor

jesmrec commented Mar 20, 2019

If the server is http with OAuth2, there is no intermediate step, so the new warning is displayed at the same time as the alert, because no certificate is required:

Mar-20-2019 15-41-06

Should we include something in the flow to stop the alert before?

@jesmrec
Copy link
Contributor

jesmrec commented Mar 20, 2019

About the OCCertificate color labels, i can reproduce Error, Passed, Warning and Accepted. But Rejected, i do not find steps to get it. Any idea?

@hosy
Copy link
Collaborator

hosy commented Mar 20, 2019

@jesmrc I don’t know how to force Rejected.
I am not sure how to handle the http step. Maybe we should include an additional step!? @michaelstingl

@jesmrec
Copy link
Contributor

jesmrec commented Mar 20, 2019

about the statuses, i will add more UI tests to cover all options.

@hosy
Copy link
Collaborator

hosy commented Mar 20, 2019

@jesmrec is there a public OC server with OAuth2 and http for testing?

@jesmrec
Copy link
Contributor

jesmrec commented Mar 21, 2019

no, afaik. But you can create a docker in this way:

  1. Create the container:
    docker run -name OAuth2container -p 15000:8080 owncloud/server:10.1

  2. Install and enable OAuth2 inside:
    docker exec -ti OAuth2container bash -l
    cd apps
    git clone https://github.com/owncloud/oauth2.git
    ../occ app:enable oauth2

Maybe, there are another "scripted" way to do it.

@jesmrec
Copy link
Contributor

jesmrec commented Mar 21, 2019

typo is fixed. i will address the tests in a separate issue. When http issue is done, please ping me to check and move it forward.

@michaelstingl
Copy link
Contributor Author

2. Install and enable OAuth2 inside:
docker exec -ti OAuth2container bash -l

You don't need to exec into the Docker to install apps:
https://github.com/owncloud-docker/base#available-environment-variables

@michaelstingl
Copy link
Contributor Author

I am not sure how to handle the http step. Maybe we should include an additional step!? @michaelstingl

Missing SSL should also trigger a cert warning 😱 /cc @felix-schwarz

@jesmrec
Copy link
Contributor

jesmrec commented Mar 21, 2019

You don't need to exec into the Docker to install apps:
https://github.com/owncloud-docker/base#available-environment-variables

thanks for the tip!!

@felix-schwarz
Copy link
Contributor

I am not sure how to handle the http step. Maybe we should include an additional step!? @michaelstingl

Missing SSL should also trigger a cert warning 😱 /cc @felix-schwarz

Unless the user enters a URL starting with http://, the SDK will pre-pend https://, so it takes extra effort on behalf of the user to enter http://. That's why it currently doesn't respond to it with an issue.

I think we have several options here:

  • handle the http://-case fully inside the app (which would then bring up an alert on its own)
  • handle the http://-case inside the SDK, which should require little to no changes in the app.

If we pick the SDK-option, I'd also like to add an OCClassSettings/MDM option HTTPURLPolicy with these possible options:

  • allow (would be present state: no warning)
  • warn (would turn a http-URL into a warning as proposed)
  • forbidden (would turn a http-URL into an error, so no bookmark can be created for it)

@hosy
Copy link
Collaborator

hosy commented Mar 21, 2019

@felix-schwarz I like the SDK-option, because it is fixed inside the SDK and cannot be removed. It is more secure by default.
I also like to implement your suggestion with the possible options. But we should show always an warning, if a http:// server is entered. Maybe allow should also the same like warn.

@felix-schwarz
Copy link
Contributor

@hosy I agree. warn should be the default. But allow as a distinct option is useful for keeping unit tests / testing simple. Some tests would otherwise have to choose between supplying their own certificate - or handling the warning.

@felix-schwarz
Copy link
Contributor

felix-schwarz commented Mar 22, 2019

@hosy Just pushed the SDK-implementation and some necessary app-changes in owncloud/ios-sdk@89f5b7b and #328 respectively. Chances are some UI tests need to be adapted (I didn't check, yet).

@hosy
Copy link
Collaborator

hosy commented Mar 23, 2019

@jesmrec for http I added an additional step to show the OAuth info text, before the OAuth authentication process is performed. There is now one step more for http connections, but this should be rare, because https should be the standard.
I also linked against the newest SDK version
owncloud/ios-sdk@89f5b7b

hosy added a commit that referenced this issue Mar 26, 2019
* #267 - added a info view as tableview header for showing additional informations for OAuth 2 login
- added additional informations for certificate validation as label and text

* better value handling

* changed function name to better reflect, what it does

* now using theme colors

* fixed typo

* Fix for http connections and OAuth authentication
- new sdk is used, which supports warnings for https connections
- for http connections, continue only, if OAuthInfoHeader was shown to the user, before OAuth authentication is performed

* fixed ui test for OAuth http test, added additional Continue step

* - correct handling of OAuth Header in edit mode
- remove certificate description, if certificate was removed from bookmark
- added new helper methods for updating header / footer title in TableView Section without loosing text field focus

* - fixed wrong table header view height, if it was not set
- fixed additional continue step in edit mode for http token based connections

* fixed ui test, because password field is not visible on iPhone 8 (used test device)
@hosy hosy closed this as completed Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Estimation - 3 (M) 3 points Login p2-high Escalation, on top of current planning, release blocker
Projects
None yet
Development

No branches or pull requests

4 participants