Repository navigation
Conversation
|
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. |
36b2f34 to
c0dfe69
Compare
3cf26fc to
d2885f5
Compare
a1231f5 to
0d53520
Compare
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/get_task_instance
https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/get_task_instance
This is designed this way to align the design of the get task instance endpoints.
There was a problem hiding this comment.
If the consistency here doesn't matter, I'm more than happy to just merge it into one. 🙂
There was a problem hiding this comment.
Let's merge it into one. No need to repeat bad practices
ashb
left a comment
There was a problem hiding this comment.
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?
@ashb this is for as stated in https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=362056119#AIP90Humanintheloop-Whatchangedoyouproposetomake? in the section:
|
To meet the frontend's needs. I last heard that we don’t have the 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
|
|
@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 |
|
@ guan404ming, we need to hold off the frontend part for the time being. Sorry for the inconvenience. |
|
Mark it as a draft till we decide whether to keep this feature |
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. 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. |
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.
Currently, there is nothing stored in the database for this share link feature. The
This is something stated in the AIP-90. I am ok with removing it, but would like to confirm with @jscheffl or @vikramkoka. This
Yes, this is something I don't think we should do. |
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:
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:
So it's not really impossible or insecure to do it this way. |
|
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 I think we somewhat achieve what's needed, but I'm still not super comfortable with |
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. |
|
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. |
💯 |
|
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. |
|
The fact that this might not be enabled by default doesn't by itself make it secure.
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. |
|
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.
Let's. FIrst let's see however if maybe we can achieve consensus. |
|
If we need an endpoint to generate the URL to the UI then I'm fine with using |
|
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?) 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. |
|
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 And we'll need to add |
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) |
|
de-scope as consensus reached |

Why
closes: #52202
What
hitl_enable_shared_links: whether to enable this featurehitl_shared_link_expiration_hours: the default hours to expire a shareable linkPOST /hitlSharedLinks/generate/{DAG_ID}/{DAG_RUN_ID}/{TASK_ID}redirectandrespondhttp://testserver/api/v2/redirect/{token}http://testserver/api/v2/respond/{token}GET /hitlSharedLinks/generate/{DAG_ID}/{DAG_RUN_ID}/{TASK_ID}/{map_index}GET /hitlSharedLinks/redirect/{token}http://testserver/dags/{DAG_ID}/runs/{DAG_RUN_ID}/tasks/{TASK_ID}/required_actionsPOST /hitlSharedLinks/respond/{token}PATCH /hitlDetails/{dag_id}/{dag_run_id}/{task_id}orPATCH /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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.