Skip to content

Allow generating human-in-the-loop links that can be redirected to the corresponding page and perform action - #53907

Closed
sunank200 wants to merge 3 commits into
apache:mainfrom
astronomer:simplified-hitl-shared-links
Closed

sunank200 wants to merge 3 commits into
apache:mainfrom
astronomer:simplified-hitl-shared-links

Conversation

@sunank200

@sunank200 sunank200 commented Jul 30, 2025 •

Copy link
Copy Markdown
Collaborator

Why

closes: #52202

What

  • config
    • hitl_enable_shared_links: whether to enable this feature
    • hitl_shared_link_expiration_hours: the default hours to expire a shareable link
  • endpoints
    • POST /hitlSharedLinks/generate/{DAG_ID}/{DAG_RUN_ID}/{TASK_ID}
      • generate token/url for redirect and respond
        • e.g.,
          • http://testserver/api/v2/redirect/{token}
          • http://testserver/api/v2/respond/{token}
        • example payload
          • link_type
          • chosen_options
          • params_input
          • expires_at
    • GET /hitlSharedLinks/generate/{DAG_ID}/{DAG_RUN_ID}/{TASK_ID}/{map_index}
      • same as above, but for mapped tasks. this is designed this way to align with task instance endpoint
    • GET /hitlSharedLinks/redirect/{token}
      • redirect to http://testserver/dags/{DAG_ID}/runs/{DAG_RUN_ID}/tasks/{TASK_ID}/required_actions
    • POST /hitlSharedLinks/respond/{token}
      • equivalent PATCH /hitlDetails/{dag_id}/{dag_run_id}/{task_id} or PATCH /hitlDetails/{dag_id}/{dag_run_id}/{task_id}/{map_index} with pre-defined payload

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@Lee-W

Lee-W commented Aug 6, 2025

Copy link
Copy Markdown
Member

Hey, thank you for working on this draft version. As the release date approaches, I will follow up, assist with this, and get it merged.

@Lee-W Lee-W self-assigned this Aug 6, 2025
@Lee-W
Lee-W force-pushed the simplified-hitl-shared-links branch from 36b2f34 to c0dfe69 Compare August 6, 2025 08:40
@Lee-W
Lee-W force-pushed the simplified-hitl-shared-links branch 5 times, most recently from 3cf26fc to d2885f5 Compare August 12, 2025 13:26
@Lee-W
Lee-W force-pushed the simplified-hitl-shared-links branch 5 times, most recently from a1231f5 to 0d53520 Compare August 14, 2025 13:12
@Lee-W
Lee-W marked this pull request as ready for review August 14, 2025 13:14
Comment on lines +123 to +134
def generate_mapped_ti_shared_link(
# task instance identifier
dag_id: str,
dag_run_id: str,
task_id: str,
map_index: int,
# payload
generate_shared_link_request: GenerateHITLSharedLinkRequest,
# utitliy
request: Request,
session: SessionDep,
) -> HITLSharedLinkResponse:

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.

Having two methods that are otherwise identical apart from the map_index parameter they pass on seems overkill -- we already have an established pattern of -1 or None = not mapped, so I don't think we need two functions.

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.

@Lee-W Lee-W Aug 18, 2025 •

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.

If the consistency here doesn't matter, I'm more than happy to just merge it into one. 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's merge it into one. No need to repeat bad practices

@ashb ashb 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.

At first glance, this all seems overly complex.

If we want a redirect we can probably do it without a token, just using the UUID of the task instance -- .../redirect/{ti_id} etc.

At the very least, the link should be tied to the specific TI try (i.e the TI.id UUID value) not the dag_id,run_id,task_id,map_index, otherwise the link from one try could be mistakenly used on the wrong try.

But mostly: Why do we need this?

@jscheffl

Copy link
Copy Markdown
Contributor

But mostly: Why do we need this?

@ashb this is for as stated in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=362056119#AIP90Humanintheloop-Whatchangedoyouproposetomake? in the section:

...
API support:
...

  • An URL endpoint is provided that can be used to provide a prepared link that can be shared (utility to generate included in provider to automate creation of notifications)
    • Directly opens and re-directs to the dialog with details
    • Directly trigger one action as a single-click (e.g. single click on a link in an email to approve) - assuming user is logged-in before
    • Security first! This would need to be explicitly enabled in configuration and a token would be added when generating to ensure CSRF is not made by artificial URLs. URLs would need to be generated by the utility before distribution.

@Lee-W

Lee-W commented Aug 18, 2025 •

Copy link
Copy Markdown
Member

At the very least, the link should be tied to the specific TI try (i.e the TI.id UUID value) not the dag_id,run_id,task_id,map_index, otherwise the link from one try could be mistakenly used on the wrong try.

To meet the frontend's needs. I last heard that we don’t have the ti_id from the frontend.

We can either call the "get task instance" endpoint to retrieve the ID and then make a subsequent API call, or we could choose not to support this from the frontend. Additionally, I noticed that none of our public API endpoints use the ti_id.

If we want a redirect we can probably do it without a token, just using the UUID of the task instance -- .../redirect/{ti_id} etc.
...
But mostly: Why do we need this?

as stated in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=362056119#AIP90Humanintheloop-Whatchangedoyouproposetomake?

* An URL endpoint is provided that can be used to provide a prepared link that can be shared (utility to generate included in provider to automate creation of notifications)

    * Directly opens and re-directs to the dialog with details
    * Directly trigger one action as a single-click (e.g. single click on a link in an email to approve) - assuming user is logged-in before
    * Security first! This would need to be explicitly enabled in configuration and a token would be added when generating to ensure CSRF is not made by artificial URLs. URLs would need to be generated by the utility before distribution.

@Lee-W

Lee-W commented Aug 18, 2025 •

Copy link
Copy Markdown
Member

@jscheffl @vikramkoka @ashb I feel like we have reached a point where we need to decide whether we want to keep this sharing feature. If we choose not to have it, I'll send an email to the dev list to raise awareness. Also, this is the last piece of AIP-90 (except for some doc improvement), so if we reach a consensus on removing the sharing feature, I think we can call AIP-90 complete and ask everyone to test it

@Lee-W

Lee-W commented Aug 18, 2025

Copy link
Copy Markdown
Member

@ guan404ming, we need to hold off the frontend part for the time being. Sorry for the inconvenience.

@Lee-W Lee-W moved this from In progress to On Hold in AIP-90 - Human in the loop Aug 18, 2025
@Lee-W

Lee-W commented Aug 18, 2025

Copy link
Copy Markdown
Member

Mark it as a draft till we decide whether to keep this feature

@Lee-W
Lee-W marked this pull request as draft August 18, 2025 07:46
@ashb

ashb commented Aug 18, 2025

Copy link
Copy Markdown
Member

To meet the frontend's needs. I last heard that we don’t have the ti_id from the frontend.

Then lets change that. We control both ends, we can change it 😄

I also wonder if a lot of that could be done with query params and on the front end. ?option=Approve or something like that pre-populates the form. i.e. nothing needed to store in the DB for that plan to work.

Single click to approve in an email I don't think is safe -- for that to work I think it would need to be a GET request and having a GET mutate state seems like a poor idea. We could maybe have a GET request that loads the page and then submits via JS, but that still feels like an error/risk-prone approach to me.

A pure GET request that mutates the state runs the risk of some background process (be it the browser itself pre-fetching, or the MUA etc) hitting it and running the endpoint, so I think that is out to my mind.

@Lee-W

Lee-W commented Aug 18, 2025

Copy link
Copy Markdown
Member

Then lets change that. We control both ends, we can change it 😄

I don't think it's the best idea to make this change. However, I might miss some context from the frontend perspective. Thus, I'd love to hear what Brent and @pierrejeambrun think before proceeding with the change.

I also wonder if a lot of that could be done with query params and on the front end. ?option=Approve or something like that pre-populates the form. i.e. nothing needed to store in the DB for that plan to work.

Currently, there is nothing stored in the database for this share link feature. The ?options=... thing is something in the plan.

Single click to approve in an email I don't think is safe -- for that to work I think it would need to be a GET request and having a GET mutate state seems like a poor idea. We could maybe have a GET request that loads the page and then submits via JS, but that still feels like an error/risk-prone approach to me.

This is something stated in the AIP-90. I am ok with removing it, but would like to confirm with @jscheffl or @vikramkoka. This respond public endpoint is similar to the one used for responses, but it may be easier for users to use. I also view it as somewhat risky. But at least the user knows they're responding to something. So they can decide whether they want to take the risk. If they have doubt, don't click it. while the redirect endpoint won't do any action

A pure GET request that mutates the state runs the risk of some background process (be it the browser itself pre-fetching, or the MUA etc) hitting it and running the endpoint, so I think that is out to my mind.

Yes, this is something I don't think we should do.

@potiuk

potiuk commented Aug 18, 2025

Copy link
Copy Markdown
Member

A pure GET request that mutates the state runs the risk of some background process (be it the browser itself pre-fetching, or the MUA etc) hitting it and running the endpoint, so I think that is out to my mind.

It's also a CSRF issue if GET mutates state. https://security.stackexchange.com/questions/115794/should-i-use-csrf-protection-for-get-requests

@potiuk

potiuk commented Aug 18, 2025 •

Copy link
Copy Markdown
Member

A pure GET request that mutates the state runs the risk of some background process (be it the browser itself pre-fetching, or the MUA etc) hitting it and running the endpoint, so I think that is out to my mind.

It's also a CSRF issue if GET mutates state. https://security.stackexchange.com/questions/115794/should-i-use-csrf-protection-for-get-requests

However, actually I think (after reading the whole thread) it's not dangerous to have a GET request for that if:

  • we have UUID as parameter of the GET
  • we embed short living JWT token in the GET request parameter bound with the UUID and (potentially) with the user
  • we validate both the token and the user in the GET request
  • we never share the URL token in the Airflow App and in any other way than - say - via email or Slack
  • the GET request only allows for that single pre-determined action (approve) and absolutely nothing else

CSRF is only a problem when the GET mutating state is available somewhere in the web page that the attacker might be able read from another tab of the same browser, because then attacker can "execute" the URL in the context of that original tab, and even if they do not see the result they can initiate the action.

This is a different story if the GET URL is shared via a different channel, has short-living time and is only ever displayed in the browser in a tab that is not from the original app (say Gmail window) - then attacker cannot execute the request with all the session cookies of the user for authentication because such GET request will not have the right cookies identifying the user in the "target" application.

While GET "technically" should not allow mutations, such technique is often used - for example all the sites that sent "magic links" - like SLACK when you want to login on the new machine - they are doing exactly this. They are sending GET URL requests that have embedded short-living token than allows the user to confirm that they are who they are (because they received the link via mail) - and effectively they mutate the state of the application after that GET link is used by the user:

Screenshot 2025-08-18 at 11 01 23

So it's not really impossible or insecure to do it this way.

@Lee-W

Lee-W commented Aug 18, 2025

Copy link
Copy Markdown
Member

In the current implementation, the secret key is used to encode both task instance data and predefined HITL data. The task instance data includes the ti_id, which is retrieved from the database and not provided by the user. We will verify if the ti_id corresponds to the expected value. Also, there's an expires_at default to 1 day if not set.

During the token decoding process, we will need the secret key to decode it. Then, we'll check the expires_at value and whether the ti_id is correct. The user also needs the permission to respond to this hitl detail. Otherwise, clicking the link will result in a 403.


I think we somewhat achieve what's needed, but I'm still not super comfortable with GET performing the update of the data. Would love to hear how the AIP authors think back to the time it was proposed

@potiuk

potiuk commented Aug 18, 2025 •

Copy link
Copy Markdown
Member

I think we somewhat achieve what's needed, but I'm still not super comfortable with GET performing the update of the data. Would love to hear how the AIP authors think back to the time it was proposed

Despite being somewhat "ok-ish" from the security point of view I am also a bit uneasy with it.

I think really better solution would be an action that requires a POST request with all the protection - and the GET link in email would lead to simple page where user could confirm the action that would sends a POST request to our server

This request could include a CSRF token to prevent unauthorized requests.

This also has additional benefit that the user could actually see what they are confirming.

Any "non-user" automation out there could use POST request directly and we could generate enough info for the automation to construct the POST properly with CSRF tokens etc.

@bbovenzi

Copy link
Copy Markdown
Contributor

I can get the desire to approve direct from an email. Let's just direct the user to the relevant page in the Airflow UI for now. HITL actions have forms so its not always necessarily a simple click to approve and then in the Airflow UI the user has the appropriate context.

@potiuk

potiuk commented Aug 18, 2025

Copy link
Copy Markdown
Member

I can get the desire to approve direct from an email. Let's just direct the user to the relevant page in the Airflow UI for now. HITL actions have forms so its not always necessarily a simple click to approve and then in the Airflow UI the user has the appropriate context.

💯

@jscheffl

jscheffl commented Aug 18, 2025 •

Copy link
Copy Markdown
Contributor

I do not have a very strong opinion and also see the risk but (1) the feature needs to be enabled and (2) somebody needs to create and distribute the link. And (3) the link is "secured". Do not forget (4) the feature needs to be enabled by config as well. So if somebody has the need for the option he probably knows what he is doing. GET or POST... mainly it is the option of a "single click" approval/confirm or a "two click" (1 click for the link to open the UI and then another press of the button".

I would be hesitant as well if it would generate emails with "dangerous links" per default but here somebody needs to want id and somebody needs to make the glue e.g. distributing a link in an email. No out of the box danger.

Amazon also got very famous for "1-click" buy and got also patents on it, (I never used it but it seems some people liked it)... having the risk of mis-clicking and directly charging credit card.

TLDR: I am not hesitant merging, it is ready, looks good and is safe by default. But marketing.wise you can make a "1 click approve email" if somebody wants to have this UX. I'd still advise not to make a "click here to approve budget for 1 mio dollar" but maybe minor convenience things like passive approvals... Confluence also does it with "Like the page" direct links.

P.S.: If we decide not to merge this feature and have this discussion here in the PR I think we don't need to be super-formal with devlist if we majority decide here to de-scope this.

@ashb

ashb commented Aug 18, 2025

Copy link
Copy Markdown
Member

The fact that this might not be enabled by default doesn't by itself make it secure.

Any "non-user" automation out there could use POST request directly and we could generate enough info for the automation to construct the POST properly with CSRF tokens etc.

Exactly.

Essentially what I'm asking is (and I think this is what Brent and Jarek are suggesting?), is instead of creating a whole new URL endpoint and a database model, can't we "deep link" to the page in the Airflow UI with the form filled out appropriately, and then the user can click there.

And as for the "respond" end point: we need to remove that. It's a parallel API we have to maintain. Either the public API is fit for purpose, or we should change that to make it more user friendly. We shouldn't be maintaining two APIs to do the same thing.

@potiuk

potiuk commented Aug 18, 2025

Copy link
Copy Markdown
Member

I agree with Ash. We can always think about having "1-click approval" option later if users will want it, but simpler is better and more secure almost by definition, and this one has a potential to be abused or misused.

It might even happen that users might find two-click solution better than 1-click in this case. Would be great to check it, but sometimes introducing a bit of friction is a good solution - especially in "corporate" context. I understand why Amazon likes 1-click but you yourself mentioned that for "cautious" users it's not the best solution, and I think being cautious in business workflows is the default.

I think there is always a good reason why we are asking for approval. I don't imagine businesses would choose manual approval in otherwise automated workflows for anything that is not "serious" - because it needs a person to notice it and react - so I imagine vast majority of the "approvals" generated this way will be "rather serious". in this context having a confirmation /review screen where you actually verify what you confirm is a good idea. And it immediately solves all kinds of accidental clicks / mistakes that people can make. For example I REALLY love the feature of "undoing" whatever gmail action I've done. It saved me many, many, many timesm where things were "too easy" to accidentally trigger or where I had a chance to have "second thought". The UI screen is this kind of chance to have second thought about the action just done.

P.S.: If we decide not to merge this feature and have this discussion here in the PR I think we don't need to be super-formal with devlist if we majority decide here to de-scope this.

Let's. FIrst let's see however if maybe we can achieve consensus.

@bbovenzi

Copy link
Copy Markdown
Contributor

If we need an endpoint to generate the URL to the UI then I'm fine with using ti_id as long as it outputs a URL with task_id, try_nunber and everything else the UI already uses

@Lee-W

Lee-W commented Aug 19, 2025 •

Copy link
Copy Markdown
Member

Follow through the discussion, looks like we prefer to de-scope it.

@ashb @bbovenzi @potiuk for de-scoping (I guess I can view it as +1 for de-scoping?)
@jscheffl don't have a strong opinion (+0 or -0)

I do have an opinion on how things should be done, but don't have a strong opinion on whether we should add it (probably a +0)


Please let me know if I've misinterpreted anything 🙂 If there's no objection by Aug 20th, 10:00 a.m. TST/GMT+8, then I'll close this PR and the relevant issues.

@ashb

ashb commented Aug 19, 2025

Copy link
Copy Markdown
Member

To be clear, I'm not for descoping the whole feature. I'm all for having a link that you can put in an email/slack message that has the right options already chosen and populated that takes you the right place. That makes perfect sense to me, and I think we should keep that part of the feature (no comment either way on current code approach)

@Lee-W

Lee-W commented Aug 19, 2025 •

Copy link
Copy Markdown
Member

To be clear, I'm not for descoping the whole feature. I'm all for having a link that you can put in an email/slack message that has the right options already chosen and populated that takes you the right place. That makes perfect sense to me, and I think we should keep that part of the feature (no comment either way on current code approach)

I think we can already copy the .../required_actions link from the Airflow UI, and that link can be added to any notifier. So /redirect might not be strictly needed.

And we'll need to add .../required_actions?options=...&input=..., but that's from the UI end, and will be another PR, not this on.e

@Lee-W Lee-W mentioned this pull request Aug 19, 2025
1 task done
@jscheffl

Copy link
Copy Markdown
Contributor

To be clear, I'm not for descoping the whole feature. I'm all for having a link that you can put in an email/slack message that has the right options already chosen and populated that takes you the right place. That makes perfect sense to me, and I think we should keep that part of the feature (no comment either way on current code approach)

I think we can already copy the .../required_actions link from the Airflow UI, and that link can be added to any notifier. So /redirect might not be strictly needed.

And we'll need to add .../required_actions?options=...&input=..., but that's from the UI end, and will be another PR, not this on.e

Yeah, agree, a "2-click-solution" is already in place, just started the HITL example and the first form can be directly navigated with http://localhost:28080/dags/example_hitl_operator/runs/manual__2025-08-19T19:31:17.119372+00:00/tasks/wait_for_option/required_actions (or abstract http://localhost:28080/dags/<dag_id>/runs/<run_id>/tasks/<task_id>/required_actions)

@Lee-W

Lee-W commented Aug 20, 2025

Copy link
Copy Markdown
Member

de-scope as consensus reached

@Lee-W Lee-W closed this Aug 20, 2025
@github-project-automation github-project-automation Bot moved this from On Hold to Done in AIP-90 - Human in the loop Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

API endpoints for sharing HITL details, operations through links from HITLOperators

9 participants