Skip to content

handle parsing exception for OIDCHttpClient::checkOIDCIsAvailable() #3459

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

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

florentos17
Copy link
Collaborator

here is a fix for a bug pointed out by this user experience review.

  • upon providing https://api.fastmail.com/jmap/session, the actual error is a 401 Unauthorized and this case was handeled in this pull request ;
  • upon providing the correct https://api.fastmail.com, another error occured because the server answers with HTML instead of JSON, eg, FormatException. the correct behaviour, which i fix with the current PR, is to ignore webfinger and fall back to the classic credential form.

Copy link

This PR has been deployed to https://linagora.github.io/tmail-flutter/3459.

@chibenwa
Copy link
Member

Please record a little video accessing a fastmail account after this change, if possible.

@tddang-linagora
Copy link
Collaborator

From the same discussion:

Our JMAP endpoint does not support basic auth

There's no point showing our own log in form. We need to add OAuth support in order for FastMail to work on Twake Mail.

@florentos17
Copy link
Collaborator Author

florentos17 commented Feb 3, 2025

There's no point showing our own log in form. We need to add OAuth support in order for FastMail to work on Twake Mail.

correct, issue 3455 has already been open on the topic

Please record a little video accessing a fastmail account after this change, if possible.

thus with the current work we still cannot access a fastmail account, but the parsing exception is handled and the app falls back to the basic auth form instead of throwing an error:

fix-parsing-exception.mp4

@florentos17
Copy link
Collaborator Author

Is it ok if I force push here to clean the 39 commits that don't belong to this PR ?

@hoangdat
Copy link
Member

first, please use git rebase to only pick your modification for this issue, then force push.

@florentos17
Copy link
Collaborator Author

@hoangdat @tddang-linagora @dab246 I rebased on latest master and checked that the demo is still the same, it is all good.

Tell me if you have any last comment

Copy link
Member

@dab246 dab246 left a comment

Choose a reason for hiding this comment

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

lgtm

@hoangdat hoangdat merged commit 005790a into master Feb 18, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants