Add auth to State

[nemanjabogdanovic]

No description provided.

[bdebinska]

I'm not entirely convinced this is necessary. Why does "auth" have to be a top level key? Can't it be part of the action itself?

[bdebinska]

maybe use Req.merge?

[nemanjabogdanovic]

Good catch, but it's older code and not super-relevant for this PR, so I'd prefer handling it separately if that's okay

[bdebinska]

Another comment from my side: This branch and PR should be linked to the ticket

[nemanjabogdanovic]

I'm not entirely convinced this is necessary. Why does "auth" have to be a top level key? Can't it be part of the action itself?

It's so that the services themselves (e.g. Batch Service) can generate and set the auth key outside a single action/step (a step shouldn't be involved in the auth of the calling services).

[bdebinska]

I'm not entirely convinced this is necessary. Why does "auth" have to be a top level key? Can't it be part of the action itself?

It's so that the services themselves (e.g. Batch Service) can generate and set the auth key outside a single action/step (a step shouldn't be involved in the auth of the calling services).

Bad formulation on my side sorry. I meant more that it seems that having a separate "auth" action/step, instead of having it in the state directly seems more fitting to me.

[nemanjabogdanovic]

I'm not entirely convinced this is necessary. Why does "auth" have to be a top level key? Can't it be part of the action itself?

It's so that the services themselves (e.g. Batch Service) can generate and set the auth key outside a single action/step (a step shouldn't be involved in the auth of the calling services).

Bad formulation on my side sorry. I meant more that it seems that having a separate "auth" action/step, instead of having it in the state directly seems more fitting to me.

Hm, maybe.. My line of thinking was always that the current workflows shouldn't be affected/touched, so I put everything on the services. We could discuss this other way as well

[iStefo]

Hi all 👋

I'd like to throw a different suggestion into the ring: Make authentication a callback-based operation (optional of course). This brings flexibility while keeping complexity out of workflow engine.

While I haven't fully thought this through, I could image it looking like this:

defmodule MyApp.WorkflowEngine do
  use WorkflowEngine, …

  def authenticate(:http, target, auth_context) do
    {:ok, {:bearer, "foobar"}}
  end

  def authenticate(_action_type, _target, _auth_context), do: {:ok, nil}
end

where :http is hardcoded based on the action type, target would depend on the action type (for HTTP it could be either the full URL or a {method, host, path} tuple) and auth_context is a struct with

%{
  state: %WorkflowEngine.State{},
  action: %{}, # action JSON being executed
  workflow: %{} # whole workflow JSON definition or identifier?
}

Information about the acting entity would be placed into a new key in the workflow engine's state when calling execute. But then no token needs to be fetched or created if the workflow doesn't even ask for auth, while automatically centralizing authentication code for all workflow executions.

The return format would depend on the action type, for HTTP you could initially support {:ok, nil} or {:ok, {:bearer, "token"}} and then later also basic auth etc. when needed. Other actions might need different auth formats (SSH or API keys, client certificates...)

To make it easy to use, WorkflowEngine should add/generate a get_auth method that can be used by actions to fetch auth for the current action if the callback has been implemented by the hosting application.

[nemanjabogdanovic]

Hi all 👋

I'd like to throw a different suggestion into the ring: Make authentication a callback-based operation (optional of course). This brings flexibility while keeping complexity out of workflow engine.

While I haven't fully thought this through, I could image it looking like this:
...

Heyy, thanks for the suggestion! I like the idea overall, I'll try and see if I can cook something up around it

[bdebinska]

Code-wise looks good, but I'm still not convinced we should add this feature if we won't be meaningfully using it in the foreseeable future

[mpneuried]

Code wise it looks good.
Only some small doc extensions would be nice.

I partially agree with BDE, on the other side we now put effort in it and it's covered by tests. So I'm fine with it.