Skip to content

Add Reset Password button to Edit User view - #70243

Merged
potiuk merged 7 commits into
apache:mainfrom
Aaryan123456679:fix/reset-password-edit-user-view-37030
Sep 21, 2026
Merged

potiuk merged 7 commits into
apache:mainfrom
Aaryan123456679:fix/reset-password-edit-user-view-37030

Conversation

@Aaryan123456679

@Aaryan123456679 Aaryan123456679 commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

The Show User view already surfaces a Reset Password action button (via FAB's show-widget actions block), but the Edit User view has no equivalent, forcing admins to navigate back to Show User just to reset a password.

This wires the same, already-registered resetpasswords action into the Edit User page:

  • CustomUserDBModelView gets a dedicated edit_template (appbuilder/general/model/user_edit.html) and overrides edit() to pass the resetpasswords action, pk, and modelview_name straight to render_template.
  • The new template extends FAB's built-in appbuilder/general/model/edit.html and overrides its edit_form block, calling {{ super() }} for the original form and then rendering the action link via the same render_action_links macro used by show.html, outside the model's own <form> tag. An earlier version of this PR copied form_vertical.html into a new widget instead, but render_action_links emits its own nested <form>, which produced invalid nested-form HTML when called from inside the edit form; extending the page-level template avoids that. No new routes or permissions were introduced — the fix reuses the existing resetpasswords action, route, and permission mapping, and render_action_links applies its own permission filter before rendering anything.

closes: #37030

Test plan
  • Added test_user_edit_view_shows_reset_password_action_with_access — asserts the Reset Password link renders on the Edit User page for a user with read/edit access to Users and read access to Passwords.
  • Added test_user_edit_view_hides_reset_password_action_without_access — asserts the link is absent when the user lacks read access to Users.
  • Ran the full existing test file (test_views_custom_user_views.py, 28 tests) plus the broader providers/fab/tests/unit/fab/www/views/ and providers/fab/tests/unit/fab/auth_manager/views/ suites (46 tests total) — all passed, no regressions.
  • prek run --from-ref upstream/main --stage pre-commit and --stage manual — passed.
  • breeze run mypy providers/fab/src/airflow/providers/fab/auth_manager/views/user.py — passed.

Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Sonnet 5)

Generated-by: Claude Code (Sonnet 5) following the guidelines

@Aaryan123456679
Aaryan123456679 force-pushed the fix/reset-password-edit-user-view-37030 branch from 1780b15 to dbd37f4 Compare July 22, 2026 18:24
@Aaryan123456679
Aaryan123456679 force-pushed the fix/reset-password-edit-user-view-37030 branch from dbd37f4 to ca04383 Compare July 26, 2026 08:35
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 28, 2026
@potiuk

potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member

The feature makes sense — having to bounce back to Show User just to reset a password is a genuine annoyance — and I checked the part that matters most here: there is no permission bypass. lib.render_action_links applies its own filter as the first thing it does:

{% macro render_action_links(actions, pk, modelview_name) %}
    {% set actions = actions | get_actions_on_show(modelview_name) %}

so injecting the action into template_args cannot surface a button the user is not entitled to, and the action endpoint is separately protected. Your tests cover both directions, which is the right shape for this.

I'd like to suggest a different approach to the template, though, because copying form_vertical.html brings a problem with it.

render_action_links emits its own <form id="action_form" ...> (see appbuilder/general/lib.html). In the copied template the call sits inside the model <form class="form-vertical">, so the rendered page has a form nested inside another form. That is invalid HTML — browsers drop the inner element — so the button may work only incidentally, and could stop working on a browser or FAB version that handles it differently. FAB's own show.html does not hit this because the Show view has no surrounding form, so the pattern does not carry over as directly as it looks.

You are right that form_vertical.html cannot be extended — it defines no blocks, which I assume is exactly why you copied it. But the page template can be: appbuilder/general/model/edit.html wraps the widget in {% block edit_form %}. Overriding that block puts the actions after the form rather than inside it:

{% extends "appbuilder/general/model/edit.html" %}
{% import 'appbuilder/general/lib.html' as lib %}

{% block edit_form %}
  {{ super() }}
  {% if actions %}
  <div class="well well-sm">
      {{ lib.render_action_links(actions, pk, modelview_name) }}
  </div>
  {% endif %}
{% endblock %}

and the view can pass the values straight to render_template rather than reaching into widget internals:

edit_template = "appbuilder/general/model/user_edit.html"

@expose("/edit/<pk>", methods=["GET", "POST"])
@has_access
def edit(self, pk):
    pk = self._deserialize_pk_if_composite(pk)
    widgets = self._edit(pk)
    if not widgets:
        return self.post_edit_redirect()
    return self.render_template(
        self.edit_template,
        title=self.edit_title,
        widgets=widgets,
        related_views=self._related_views,
        actions={"resetpasswords": self.actions.get("resetpasswords")},
        pk=pk,
        modelview_name=self.__class__.__name__,
    )

That drops the ~45 lines of duplicated form markup, removes the need for UserEditFormWidget altogether, and means any future FAB change to form_vertical.html is picked up automatically instead of silently drifting from our copy. The permission filtering is unchanged, since it is the same macro doing the work.

Two smaller notes:

The description says the new template "extends FAB's built-in form_vertical.html" — it is a copy rather than an extends. Worth correcting before merge, since the body becomes the commit message and the maintenance implications of the two are quite different.

"Mirroring the existing show() override pattern" is also a slightly generous reading: show() removes entries FAB has already populated, whereas this adds one FAB never populates. The result is safe because of the macro-level filtering, but they aren't the same manoeuvre, and it's worth knowing the safety comes from the macro rather than from symmetry with show().


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

The Show User view already surfaces a Reset Password button via FAB's
show-widget actions block, but the Edit User view has no equivalent, so
admins have to navigate back to Show User just to reset a password.
This wires the same, already-registered resetpasswords action into the
Edit User page by giving CustomUserDBModelView its own edit widget and
template that render the action link, mirroring the existing Show User
override pattern.

closes: apache#37030
The previous template copied FAB's form_vertical.html and rendered
the action link inside the model's own <form>. render_action_links
emits its own <form id="action_form">, so the page ended up with a
form nested inside another form, which is invalid HTML and only
happened to work because browsers silently drop the inner element.

Overriding the edit_form block in appbuilder/general/model/edit.html
instead puts the action after the model form closes, drops the
duplicated form markup, and keeps the page in sync with any future
change to FAB's own template.
The view previously reached into widgets["edit"].template_args to inject
the Reset Password action; now that the action is rendered by the
edit_form block instead of a widget, the view can pass it straight to
render_template.
@Aaryan123456679
Aaryan123456679 force-pushed the fix/reset-password-edit-user-view-37030 branch from ca04383 to 5c6d0fd Compare August 1, 2026 19:50
@Aaryan123456679

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up commit that switches the Edit User view to override edit.html's edit_form block instead of copying form_vertical.html, so the Reset Password action now renders outside the model's <form> — no more nested forms. CustomUserDBModelView.edit() now passes actions/pk/modelview_name directly to render_template instead of reaching into widget internals, and the UserEditFormWidget subclass is gone.

Also corrected the PR description: the new template extends FAB's edit.html (it's a genuine override now, not a copy), and I reworded the "mirrors show()" note — show() removes entries FAB already populated, this adds one FAB never populates, and the safety here comes from render_action_links' own permission filter rather than symmetry with show().

Existing tests (test_user_edit_view_shows_reset_password_action_with_access, test_user_edit_view_hides_reset_password_action_without_access) needed no changes since they assert on rendered output, not widget/template internals — reran the full file (28 tests) and it passes.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 16, 2026
@Aaryan123456679

Copy link
Copy Markdown
Contributor Author

Hi Team, kindly look into this PR. Friendly Ping

@github-actions github-actions Bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 17, 2026
@vincbeck

Copy link
Copy Markdown
Contributor

Any chance you can attached a screenshot to the description so that we can see the final result?

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. I traced the permission chain rather than taking the description's word for it, and it holds up: render_action_links → get_actions_on_show → is_item_visible, which resolves resetpasswords through method_permission_name to can_read and checks it against class_permission_name — a property that returns Users on the edit endpoint and Passwords on the action endpoint (user.py:61-70). Link visibility is Users-read, execution is Passwords-read, and both match what the Show view already does. No new permission surface, which is the thing that matters here.

Three more things I went looking for and didn't find problems with:

  • The edit() override is a faithful copy of FAB's ModelView.edit (views.py:253-264) with three extra kwargs. I specifically checked whether form_action was dropped — FAB's base doesn't pass it either, so the widget renders exactly as upstream.
  • self.actions.get("resetpasswords") mirrors FAB's own UserDBModelView.show() (security/views.py:385-387) verbatim. I'd have flagged the None case if it weren't the upstream pattern.
  • The nested-form reasoning in your description is right: {% block edit_form %} in edit.html isn't inside a <form>, so the action links land outside the model form.

Two follow-ups:

Could you attach a screenshot? The template asks for before/after on user-facing UI changes, and there's a substantive reason here: your <div class="well well-sm"> becomes a direct child of .tab-content that isn't a .tab-pane, so it should sit below the tab content and persist across tab switches. That's almost certainly what you intend, but it's a rendering question the tests can't answer — they only assert the string is present.

One test case worth adding — details inline on line 156. The two tests vary two permissions at once, so they don't pin the Users-read / Passwords-read split your own comment describes.

I've approved so this isn't waiting on another review round, but please address the inline comment and add the screenshot, and mark the thread resolved, before it merges. Ping me when they're done and I'll take the next look.

Nice work on the iteration history, incidentally: the widget-subclass approach, discovering the nested-<form> problem, and writing up why the page-level template is the right fix made this much faster to review than it would otherwise have been.


This review was drafted by an AI-assisted tool and confirmed by an Airflow maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Airflow handles maintainer review: contributing-docs/05_pull_requests.rst.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

The existing pair of tests varied Users read and Passwords read together, so
they could not tell apart "link is gated on Users read" from "link is gated on
Passwords read". Cover the seam explicitly: a user with Users read but without
Passwords read sees the link, and following it is refused.
@Aaryan123456679

Copy link
Copy Markdown
Contributor Author
pr70243-after-edit-user

After

pr70243-before-edit-user

Before

@Aaryan123456679

Copy link
Copy Markdown
Contributor Author

@potiuk the follow-ups are done:

The missing test case is in b2a25d6: Users read without Passwords read shows the link, and following it is refused. The with-access test also asserts the same POST reaches the reset form.
Before/after screenshots of the Edit User page are in the PR description.
I replied on the inline thread and resolved it.
Ready for another look whenever you have time.

@potiuk
potiuk merged commit df145d1 into apache:main Sep 21, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:fab ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a Reset Password button under Security > List Users > Edit user

3 participants