geteduroam / ionic-app

iOS and Android app for geteduroam
BSD 3-Clause "New" or "Revised" License
16 stars 12 forks source link

Consistently handle prefilling and checking of the realm #36

Closed jornane closed 4 years ago

jornane commented 4 years ago

We have now gotten guidelines form the CAT team on how to handle realm hints. Currently we do this:

This is apparently not as the CAT developers intended these flags to be used; GEANT/CAT#187, they have documented the intended behaviour here: MappingCATOptionsIntoSupplicantConfig.md

The behaviour should thus be changed as such

For a suggestion on how to do this, check the Windows application.

jornane commented 4 years ago

Replying to message from Slack

How can we check it is obvious for the user which username is going to be used for login? Currently we send what is shown in the "Username" field. But you might need something else on our side to clarify this point to the user.

Currently you use what is shown in the "Username" field, but @realm might silently get appended. That should not happen anymore.

If InnerIdentityHint is not set, should we wait until the focus is out of the field to provide feedback regarding the realm? This way we might wait until the user provides the whole username.

Correct

You can also test with the Windows application to see how it handles entry of different usernames. Uninett has without InnerIdentityHint, NTNU has with InnerIdentityHint.

Cardeniosa commented 4 years ago
How can we check it is obvious for the user which username is going to be used for login? Currently we send what is shown in the "Username" field. But you might need something else on our side to clarify this point to the user.

Currently you use what is shown in the "Username" field, but @realm might silently get appended. That should not happen anymore.

We understand that we should only suggest the user to add the @realm and we shouldn't append anything in the Username field any more.

Regarding the placeholder: Should we change if InnerIdentityHint is not set to true for something like username@[optionalprefix.]realm

jornane commented 4 years ago

We understand that we should only suggest the user to add the @realm and we shouldn't append anything in the Username field any more.

This is not correct; see the last point in the ticket: "It's okay to help the user by prefilling or postfilling, but.. "

What you should not do anymore is SILENTLY appending anything to the user input. So:

Regarding the placeholder: Should we change if InnerIdentityHint is not set to true for something like username@[optionalprefix.]realm

Yes, if InnerIdentityHint is true, the [optionalprefix.] part is not allowed; the username MUST end with the exact @realm.

Cardeniosa commented 4 years ago

Fixed in commit b4dea6f54d19d0932233ceef4b6ee40910ca37b7

jornane commented 4 years ago

When entering a username such as user.uninett.no, the app tells that the username is wrong, but not why it is wrong.

amachadogarcia commented 4 years ago

Fixed in commit 04edab60b9a3f7960130ffc6dae00d49c53411b8

jornane commented 4 years ago

In the specific example, user.uninett.no is missing an @. When testing this username in the proposed fix 04edab6, the username is correctly rejected, with the message The username is incorrect, must follow the pattern.

There is no indication on screen of a pattern, so this message is confusing. It would be better to have something like must contain symbol @, so the user has an actionable way to resolve the problem themselves.

Moving back to In Progress.

jornane commented 4 years ago

We expect the user to always have an actionable error message, so from the error message it must be clear for the user what change should be made for the username to be correct.

If you are unsure what we are looking for, please try the Windows application, it already implements this correctly.

Cardeniosa commented 4 years ago

Regarding the behaviour in windows application, what we see is that realm is always added to the user and no error related to suffix is shown: image

Would this approach be enough? If so, will create a PR. If not, could you please tell us what's missing? image image