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

Problems with displaying UI messages on specific cases from version 3.1.2105.0+ #189

Closed
apr-un opened this issue Jun 30, 2021 · 10 comments
Closed

Comments

@apr-un
Copy link
Contributor

apr-un commented Jun 30, 2021

Hi @redhook62
I think that I found a bug in version 3.1.2105.0

When user try to log in with MFA enabled, and won't select authentication method on form "ChooseMethodForm" but instead click Cancel (mfa-cancelButton), systems ends on form "lockingForm" with no message (empty string) and only visible Close button (buttonquit), which in fact does nothing (just reloads page).

Instead of empty message there should be visible message from resource "ErrorInvalidIdentificationRestart" (Invalid identification, please restart your session.)

I think that empty mesage is caused by changes in BasePresentation.cs in commit 8370d52, which adds support for custom provider. On line 165 there is called constructor AdapterPresentationDefault with only two params instead five like in earlier version - omitted are: message, suite, disableoptions - so message defaults to empty value :(

8370d52#diff-db00a0ebb157e51327183df3faae8cfd8b1e915548c00d9d0874ece17413658f

Please check that code.
Regards
Arnold

@apr-un
Copy link
Contributor Author

apr-un commented Jun 30, 2021

Similar problems with lack of UI messages exists when user tries to change password - there is no confirmation/error when password doesn't meet complexity requirements.

@redhook62
Copy link
Member

I don't remember, but it is possible that we have disabled these messages so as not to display too precise information.
I will look for the next update

@apr-un apr-un changed the title Problems with displaying UI messages on specific case from version 3.1.2105.0+ Problems with displaying UI messages on specific cases from version 3.1.2105.0+ Jun 30, 2021
@apr-un
Copy link
Contributor Author

apr-un commented Jun 30, 2021

I think that this bug can have more reach - we did some more tests and there are no messages displayed when users correctly finishes biometric registration, just Finish button on empty page :(

@redhook62
Copy link
Member

Hi, @apr-un

I did not notice any such errors, however this could be due to the wrong formatting of the "SameSite".
More serious, if you cancel your session ("Cancel") you bypass the MFA (version 2106 or older?)
A blocking exception is not raised ...

a quick update soon

regards

@redhook62
Copy link
Member

Done in 3.1.2107.0

@apr-un
Copy link
Contributor Author

apr-un commented Jul 2, 2021

@redhook62 but fix in 2107 didn't address "no message" problem - in many places it should show something like this (with redacted logo ;))
obraz
but it shows something like this:
obraz

Empty white popup with one button is not user friendly :)
I'm ok with not displaying too precise information, but at least let user know that something went wrong

@apr-un
Copy link
Contributor Author

apr-un commented Jul 2, 2021

@redhook62 check please here:
https://github.com/neos-sdi/adfsmfa/blob/master/Neos.IdentityServer%203.1/Neos.IdentityServer.MultiFactor/Neos.IdentityServer.MultiFactor.BasePresentation.cs
in function

public override string GetFormHtmlMessageZone(AuthenticationContext usercontext)
        {
            string result = string.Empty;
            if (IsPermanentFailure)
            {
                result += "<br/>";
                if (!String.IsNullOrEmpty(usercontext.UIMessage))
                    result += "<div id=\"error\" class=\"fieldMargin error smallText\"><label id=\"errorText\" name=\"errorText\" for=\"\">" + usercontext.UIMessage + "</label></div>";
                else
                    result += "<div id=\"error\" class=\"fieldMargin error smallText\"><label id=\"errorText\" name=\"errorText\" for=\"\">" + Resources.GetString(ResourcesLocaleKind.Html, "HtmlErrorRestartSession") + "</label></div>";
            }
            else
            {
                if (!String.IsNullOrEmpty(usercontext.UIMessage)) /// empty UIMessage? No info for user :(
                {
                    result += "<br/>";
                    if (IsMessage)
                        result += "<div id=\"error\" class=\"fieldMargin smallText\" style=\"color: #6FA400\"><label id=\"errorText\" name=\"errorText\" for=\"\">" + usercontext.UIMessage + "</label></div>";
                    else
                        result += "<div id=\"error\" class=\"fieldMargin error smallText\"><label id=\"errorText\" name=\"errorText\" for=\"\">" + usercontext.UIMessage + "</label></div>";  
                } /// <-- HERE, else ? show some generic error message when UIMessage is empty
            }
            return result;

I did check that on debug and message which should be shown to user is just ignored (not set when creating Adapter) - in case of !IsPermanentFailure system always try to show usercontext.UIMessage (which is empty, because it is set in constructor with default equal to empty string).

@redhook62
Copy link
Member

redhook62 commented Jul 2, 2021

@apr-un

As stated, I cannot reproduce your problem.
Can you indicate the full context and the constructor call ?
This code is quite old, but why not add a generic message. a message must always be passed!
if there is no message why invent one ?
give me more informations to reproduce.

The code you show is not in the BasePresentation class or it is abstract but is in the BaseMFAPresentation class from which the presenations for ADFS 2019 and ADFS2016 / 2012r2 are derived.

What to put in the case you indicate, if it is supposed to be a message or an error message ?

image

regards

@redhook62
Copy link
Member

Hi, @apr-un

Yes finally ! I managed to reproduce the problem.
This only happens with the default interface (2016 / 2012r2), for the ADFS 2019 paginated interface there is no problem.

regards

@redhook62 redhook62 reopened this Jul 2, 2021
@redhook62
Copy link
Member

Hi @apr-un

Solved ! -> new build 3.1.2107.0

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

No branches or pull requests

2 participants