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

Use 'instanceandevents' endpoint when updating process state #937

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Altinn.App.Api/Altinn.App.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

<ItemGroup>
<PackageReference Include="Altinn.Common.PEP" Version="4.1.0" />
<PackageReference Include="Altinn.Platform.Storage.Interface" Version="4.0.3" />
<PackageReference Include="Altinn.Platform.Storage.Interface" Version="4.0.4" />
<PackageReference Include="Microsoft.FeatureManagement.AspNetCore" Version="3.5.0" />
<PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.10.0" />
<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.10.0" />
Expand Down
12 changes: 12 additions & 0 deletions src/Altinn.App.Api/Controllers/InstancesController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,10 @@ public async Task<ActionResult<Instance>> Post(

// create the instance
instance = await _instanceClient.CreateInstance(org, app, instanceTemplate);
foreach (var instanceEvent in result.ProcessStateChange?.Events ?? [])
{
instanceEvent.InstanceId = instance.Id;
}
}
catch (Exception exception)
{
Expand Down Expand Up @@ -566,6 +570,10 @@ [FromBody] InstansiationInstance instansiationInstance
}

instance = await _instanceClient.CreateInstance(org, app, instanceTemplate);
foreach (var instanceEvent in processResult.ProcessStateChange?.Events ?? [])
{
instanceEvent.InstanceId = instance.Id;
}

if (isCopyRequest && source is not null)
{
Expand Down Expand Up @@ -683,6 +691,10 @@ [FromRoute] Guid instanceGuid
ProcessChangeResult startResult = await _processEngine.GenerateProcessStartEvents(processStartRequest);

targetInstance = await _instanceClient.CreateInstance(org, app, targetInstance);
foreach (var instanceEvent in startResult.ProcessStateChange?.Events ?? [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These loops are needed because GenerateProcessStartEvents is creating the events, at which point InstanceId is null..

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a bug before this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, as it started to happen after switching to the new endpoint.. Don't know why, will do some digging to make sure

{
instanceEvent.InstanceId = targetInstance.Id;
}

await CopyDataFromSourceInstance(application, targetInstance, sourceInstance);

Expand Down
2 changes: 1 addition & 1 deletion src/Altinn.App.Core/Altinn.App.Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<PackageReference Include="Altinn.Common.EFormidlingClient" Version="1.3.3" />
<PackageReference Include="Altinn.Common.PEP" Version="4.1.0" />
<PackageReference Include="Altinn.Platform.Models" Version="1.6.1" />
<PackageReference Include="Altinn.Platform.Storage.Interface" Version="4.0.3" />
<PackageReference Include="Altinn.Platform.Storage.Interface" Version="4.0.4" />
<PackageReference Include="JsonPatch.Net" Version="3.1.1" />
<PackageReference Include="JWTCookieAuthentication" Version="3.0.1" />
<!-- The follwoing are depencencies for JWTCookieAuthentication, but we need newer versions-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,11 @@ internal void InstanceDeleted(Instance instance)
return activity;
}

internal Activity? StartUpdateProcessActivity(Instance instance)
internal Activity? StartUpdateProcessActivity(Instance instance, int eventCount = 0)
{
var activity = ActivitySource.StartActivity($"{Prefix}.UpdateProcess");
activity?.SetInstanceId(instance);
activity?.SetTag(Labels.InstanceEventsCount, eventCount);
return activity;
}

Expand Down
5 changes: 5 additions & 0 deletions src/Altinn.App.Core/Features/Telemetry/Telemetry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ public static class Labels
/// </summary>
public static readonly string InstanceGuid = "instance.guid";

/// <summary>
/// Label for the guid that identifies the instance.
/// </summary>
public static readonly string InstanceEventsCount = "instance.events.count";

/// <summary>
/// Label for the guid that identifies the data.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,36 @@
}
}

/// <inheritdoc />
public async Task<Instance> UpdateProcessAndEvents(Instance instance, List<InstanceEvent> events)
{
using var activity = _telemetry?.StartUpdateProcessActivity(instance, events.Count);
ProcessState processState = instance.Process;

string apiUrl = $"instances/{instance.Id}/process/instanceandevents";
string token = _userTokenProvider.GetUserToken();

var update = new ProcessStateUpdate { State = processState, Events = events };
string updateString = JsonConvert.SerializeObject(update);
_logger.LogInformation($"update process state: {updateString}");

StringContent httpContent = new(updateString, Encoding.UTF8, "application/json");
HttpResponseMessage response = await _client.PutAsync(token, apiUrl, httpContent);
if (response.StatusCode == HttpStatusCode.OK)
{
string instanceData = await response.Content.ReadAsStringAsync();
Instance updatedInstance =
JsonConvert.DeserializeObject<Instance>(instanceData)
?? throw new Exception("Could not deserialize instance");
return updatedInstance;
}
else
{
_logger.LogError($"Unable to update instance process with instance id {instance.Id}");

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.
This log entry depends on a
user-provided value
.

Copilot Autofix AI 4 days ago

To fix the problem, we need to sanitize the instance.Id before logging it. This can be done by removing any newline characters from the instance.Id to prevent log forging. We will use the String.Replace method to achieve this.

  1. Identify the logging statement that uses instance.Id.
  2. Sanitize the instance.Id by removing newline characters before logging it.
  3. Ensure that the fix does not change the existing functionality of the code.
Suggested changeset 1
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs b/src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs
--- a/src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs
+++ b/src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceClient.cs
@@ -189,3 +189,4 @@
         {
-            _logger.LogError($"Unable to update instance process with instance id {instance.Id}");
+            string sanitizedInstanceId = instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
+            _logger.LogError($"Unable to update instance process with instance id {sanitizedInstanceId}");
             throw await PlatformHttpException.CreateAsync(response);
EOF
@@ -189,3 +189,4 @@
{
_logger.LogError($"Unable to update instance process with instance id {instance.Id}");
string sanitizedInstanceId = instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
_logger.LogError($"Unable to update instance process with instance id {sanitizedInstanceId}");
throw await PlatformHttpException.CreateAsync(response);
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
throw await PlatformHttpException.CreateAsync(response);
}
}

/// <inheritdoc/>
public async Task<Instance> CreateInstance(string org, string app, Instance instanceTemplate)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Altinn.App.Core/Internal/Instances/IInstanceClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ public interface IInstanceClient
/// </summary>
Task<Instance> UpdateProcess(Instance instance);

/// <summary>
/// Updates the process model of the instance and the instance events and returns the updated instance.
/// </summary>
Task<Instance> UpdateProcessAndEvents(Instance instance, List<InstanceEvent> events);

/// <summary>
/// Creates an instance of an application with no data.
/// </summary>
Expand Down
25 changes: 1 addition & 24 deletions src/Altinn.App.Core/Internal/Process/ProcessEventDispatcher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ namespace Altinn.App.Core.Internal.Process;
public class ProcessEventDispatcher : IProcessEventDispatcher
{
private readonly IInstanceClient _instanceClient;
private readonly IInstanceEventClient _instanceEventClient;
private readonly IEventsClient _eventsClient;
private readonly IOptions<AppSettings> _appSettings;
private readonly ILogger<ProcessEventDispatcher> _logger;
Expand All @@ -23,14 +22,12 @@ public class ProcessEventDispatcher : IProcessEventDispatcher
/// </summary>
public ProcessEventDispatcher(
IInstanceClient instanceClient,
IInstanceEventClient instanceEventClient,
IEventsClient eventsClient,
IOptions<AppSettings> appSettings,
ILogger<ProcessEventDispatcher> logger
)
{
_instanceClient = instanceClient;
_instanceEventClient = instanceEventClient;
_eventsClient = eventsClient;
_appSettings = appSettings;
_logger = logger;
Expand All @@ -39,12 +36,7 @@ ILogger<ProcessEventDispatcher> logger
/// <inheritdoc/>
public async Task<Instance> DispatchToStorage(Instance instance, List<InstanceEvent>? events)
{
// need to update the instance process and then the instance in case appbase has changed it, e.g. endEvent sets status.archived
Instance updatedInstance = await _instanceClient.UpdateProcess(instance);
await DispatchProcessEventsToStorage(updatedInstance, events);

// remember to get the instance anew since AppBase can have updated a data element or stored something in the database.
updatedInstance = await _instanceClient.GetInstance(updatedInstance);
Instance updatedInstance = await _instanceClient.UpdateProcessAndEvents(instance, events ?? []);

return updatedInstance;
}
Expand Down Expand Up @@ -74,19 +66,4 @@ await _eventsClient.AddEvent(
}
}
}

private async Task DispatchProcessEventsToStorage(Instance instance, List<InstanceEvent>? events)
{
string org = instance.Org;
string app = instance.AppId.Split("/")[1];

if (events != null)
{
foreach (InstanceEvent instanceEvent in events)
{
instanceEvent.InstanceId = instance.Id;
await _instanceEventClient.SaveInstanceEvent(instanceEvent, org, app);
}
}
}
}
5 changes: 5 additions & 0 deletions test/Altinn.App.Api.Tests/Mocks/InstanceClientMockSi.cs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ public Task<Instance> UpdateProcess(Instance instance)
return Task.FromResult(storedInstance);
}

public Task<Instance> UpdateProcessAndEvents(Instance instance, List<InstanceEvent> events)
{
return UpdateProcess(instance);
}

private static Instance GetTestInstance(string app, string org, int instanceOwnerId, Guid instanceId)
{
string instancePath = GetInstancePath(app, org, instanceOwnerId, instanceId);
Expand Down
3 changes: 2 additions & 1 deletion test/Altinn.App.Api.Tests/OpenApi/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -5365,7 +5365,8 @@
"type": "boolean"
},
"allowInSubform": {
"type": "boolean"
"type": "boolean",
"deprecated": true
},
"shadowFields": {
"$ref": "#/components/schemas/ShadowFields"
Expand Down
1 change: 1 addition & 0 deletions test/Altinn.App.Api.Tests/OpenApi/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3290,6 +3290,7 @@ components:
type: boolean
allowInSubform:
type: boolean
deprecated: true
shadowFields:
$ref: '#/components/schemas/ShadowFields'
additionalProperties: false
Expand Down
Loading
Loading