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

Adds NextFormMixin to match intended behavior #331

Closed
wants to merge 2 commits into from

Conversation

rushilsrivastava
Copy link
Contributor

On the documentation, the intended use case for VerifyForm shows that the next form should be used but it looks like NextFormMixin is missing from the Form itself.

@rushilsrivastava
Copy link
Contributor Author

Actually, this doesn't seem to fix this either. The next parameter is definitely not being respected though.

@rushilsrivastava
Copy link
Contributor Author

Whoops, forgot to fill in the next field! Works as expected now!

@jwag956
Copy link
Collaborator

jwag956 commented May 30, 2020

So 'next' definitely works in the verify form - we have a unit test for that - and you can see it in the VerifyForm itself.
It has always confused me a bit why there are so much code and places one can place 'next' - so perhaps your use case is slightly different. Could you actually explain what you are trying to do and where you are trying to set 'next'?

You can see the test: test_verify_fresh

@rushilsrivastava
Copy link
Contributor Author

@jwag956 Yeah, I looked at that but I am not sure why it doesn't quite work in our use case. We have a custom verify template and a custom form (extended from VerifyForm) but the next parameter never gets passed in. When adding the NextFormMixin the next parameter gets passed into the form request data and FS is able to redirect the user to the correct route.

@jwag956
Copy link
Collaborator

jwag956 commented May 30, 2020

Would you be able to upload your form and template?

@rushilsrivastava
Copy link
Contributor Author

rushilsrivastava commented May 31, 2020

Here is our ExtendedVerifyForm. This is the one that works.

class ExtendedVerifyForm(VerifyForm, NextFormMixin):
    """Verify password form."""

    password = PasswordField(
        "Password", validators=[password_required], widget=BootstrapVerifyPassword(),
    )

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        if not self.next.data:
            self.next.data = request.args.get("next", "")

    def validate(self):
        """Validate the form."""
        initial_validation = super(ExtendedVerifyForm, self).validate()
        if not initial_validation:
            return False

        return True

Here is our old one:

class ExtendedVerifyForm(VerifyForm):
    """Verify password form."""

    password = PasswordField(
        "Password", validators=[password_required], widget=BootstrapVerifyPassword(),
    )

    def validate(self):
        """Validate the form."""
        initial_validation = super(ExtendedVerifyForm, self).validate()
        if not initial_validation:
            return False

        return True

Here is a sample template we were using:

<form method="POST" action="{{ url_for('security.verify') }}" role="form">
                                        {{ verify_form.hidden_tag() }}
                                        {{ verify_form.csrf_token }}
                                        <div class="form-group mb-4">
                                            <div class="d-flex align-items-center justify-content-between">
                                                <div>
                                                    {{ verify_form.password.label(class_="form-control-label") }}
                                                </div>
                                            </div>
                                            <div class="input-group input-group-merge">
                                                <div class="input-group-prepend">
                                                    <span class="input-group-text"><i class="fas fa-key"></i></span>
                                                </div>
                                                {{ verify_form.password(placeholder="Password", class_="form-control") }}
                                                {% for error in verify_form.password.errors %}
                                                    <div class="invalid-feedback"> {{ error }} </div>
                                                {% endfor %}
                                                <div class="input-group-append">
                                                    <span class="input-group-text"><a href="#"
                                                                                      data-toggle="password-text"
                                                                                      data-target="#input-password"><i
                                                            class="fas fa-eye"></i></a></span>
                                                </div>
                                            </div>
                                        </div>
                                        <div class="mt-4">
                                            <button type="submit" class="btn btn-sm btn-primary btn-icon rounded-pill">
                                                <span class="btn-inner--text">Verify</span>
                                                <span class="btn-inner--icon"><i
                                                        class="fas fa-long-arrow-alt-right"></i></span>
                                            </button>
                                        </div>
                                    </form>

@jwag956
Copy link
Collaborator

jwag956 commented May 31, 2020

Thanks - I guess I am wondering where you are even setting 'next'?
The thing is that after a successful verify() the following redirect is called:
return redirect(get_post_verify_redirect())

This in turn calls a method used by all redirects:
def get_post_action_redirect(config_key, declared=None):
urls = [
get_url(request.args.get("next", None)),
get_url(request.form.get("next", None)),
find_redirect(config_key),
]

So it first looks at the request args, then at the request form. Which from what I can tell is precisely the the NextMixin does - so it isn't clear why it isn't working. Is there anywhere in your FORM that you are setting 'next'? (I didn't see it).

@rushilsrivastava
Copy link
Contributor Author

That's where the problem seems to lie. I tried adding print statements in get_post_action_redirect to see what urls looked like. Even though the url had a request arg of next, it was showing as None when printed. I tested some more, and it looks like this only happens when you change the view url in FS config. When we changed the verify url '/verify' to '/auth/', the get args return None.

@jwag956
Copy link
Collaborator

jwag956 commented May 31, 2020

Oh - I am not opposed to your PR - since it is basically similar code to other views - I wish I understood why it works and the current code doesn't :-)

Also - I would appreciate a unit test..

Actually - I/we really need to understand what's going on - there is a lot of legacy code in this code base (3 different places that mess with 'next') - that I would like to see if it can be simplified.

@jwag956
Copy link
Collaborator

jwag956 commented May 31, 2020

Nice sleuthing - could you include your FS config?
And where are you setting the 'next' query param?

@rushilsrivastava
Copy link
Contributor Author

rushilsrivastava commented May 31, 2020

I wish I understood why it works and the current code doesn't

Totally understand 😃 - I am not too sure why this is happening either as the request.args.get seems like it should work.

Nice sleuthing - could you include your FS config?
And where are you setting the 'next' query param?

Here is the FS config for what we are testing:

SECURITY_VERIFY_URL = "/auth/"
SECURITY_VERIFY_TEMPLATE = "auth.html"

Here is the url we are testing against: http://127.0.0.1:5000/auth/?next=http%3A%2F%2F127.0.0.1%3A5000%2Fdashboard%2Fsettings%2F

Also, it looks like SECURITY_VERIFY_TEMPLATE is missing from the documentation.

@jwag956
Copy link
Collaborator

jwag956 commented May 31, 2020

Hmm - here is a test I wrote to try to reproduce - still working as I expect (you can put this in test_misc.py):

@pytest.mark.settings(verify_url="/auth/")
def test_verify_next(app, client, get_message):
    authenticate(client)
    response = client.post(
        "/auth/?next=http://localhost/mynext",
        data=dict(password="password"),
        follow_redirects=False,
    )
    assert response.location == "http://localhost/mynext"

    response = client.post(
        "/auth/?next=http%3A%2F%2F127.0.0.1%3A5000%2Fdashboard%2Fsettings%2F",
        data=dict(password="password"),
        follow_redirects=False,
        base_url="http://127.0.0.1:5000",
    )
    assert response.location == "http://127.0.0.1:5000/dashboard/settings/"

@jwag956
Copy link
Collaborator

jwag956 commented Jun 10, 2020

I am going to close this - I can't reproduce the issue, and have added a test that mimics as best I could the original issue - and everything still works as intended. If a repro case can be found - please open an issue

@jwag956 jwag956 closed this Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants