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

Feature/signing #956

Draft
wants to merge 56 commits into
base: main
Choose a base branch
from
Draft

Feature/signing #956

wants to merge 56 commits into from

Conversation

HauklandJ
Copy link
Contributor

@HauklandJ HauklandJ commented Dec 6, 2024

Description

PR so that vi can publish an experimental package

Related Issue(s)

  • #{issue number}

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

HauklandJ and others added 30 commits September 26, 2024 09:31
* adr for transaction handling

* scaffold signee service
… is supplied, authorize it as a write action. Move deletion of stale data elements to ProcessTaskInitializer.
* Expose endpoint for searching for person using ssn and last name.

* Use class and drop dto postfix.

* Move mapping to reponse class.

* Add success property to PersonSearchResult. Move person details into nested object.
* adds endpoint for getting an organisation from Enhetsregisteret

* renames OrganisationSearch -> Lookup
* temp

* feat: add phone and email to signee state

* split signee states on person/org, handle delegation before notification

* split signing notification and delegation to seperate services

* Move SigneeParty to Models folder

* update signeeConfig -> signeeParty

* split out method for processing signees to support retries

* restructure signeecontext, party and state

* update signing service to use new signingcontext structure

* split notification config based on the receiving system

* Make telemetry nullable in signing service.

* some more stuff

* Fix compilation error

* Touples in notification service. Sms number from registry. Store reason for sms/email failure.

* Extract SigningService interface. Various adjustments after mob session.

* For now: Add user action for initializing delegated signing.

---------

Co-authored-by: Bjørn Tore Gjerde <bjorn.tore.gjerde@vivende.no>
* add model for delegation request

* add step builder for delegation request

* add scaffold for delegation client

* add scaffold to signing delegation service

* temp solution for const instead of magic strings

* add delegation client

* weird state

* update handling of party id to use party uuid

* rm sign delegate rights from access management client
# Conflicts:
#	src/Altinn.App.Core/Configuration/PlatformSettings.cs
#	src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
* update builder to standard set in correspondance (1/2)

* restructure: add builders folder

* formatting

* add TryGet method to retrieve app resource id

* cleanup

* formatting

* add custom exceptions

* use IOptions for plattformsettings

* simplify builder

* format

* trailing comma

* more formatting
* update controllers

* add 500 annotation for org lookup

* fix copy pasta

* update swagger

* lastname usage goes too deep (storage)

* update swagger

* update test paths

* use 200 OK when no hit

* typo

* log error

* format

* use the correct namespacing for the logger

* shorten method
bjorntore and others added 7 commits December 17, 2024 09:58
* adds self links to signing data elements

* updates oas
* rename method

* add terlemetry to singing delegation

* use instancemuitator instead of instance

* lastname changes

* format

* temp fix for bad model

* add debug logging

* format

* domain model for application resource id

* format

* use app resource id

* add AppResourceId and simplify parameters to signing delegation

* debug logging

* lets try loggin again

* format

* update instantiation of AppIdentifier

* public

* use actual instance id

* inject taskId as param, due to being unable to resolve it

* add delegation builder test

* better logging for access management issues

* rm unused using

* better logging

* just log everything

* just log everything but better

* forgot one

* add summary for all public props

* use party uuid

* clarify vars, rm comments

* log response

* summaries

* add app and org delegation

* simplify delegation abstractions

* move appResourceId into a named namespace

* rm builders, use domain objects

* client cleanup

* add revoke signee rights

* format

* rm debug logging

* workaround -> work around

* block scoped variables

* rm debug logging of sensitive data

* delagate from current user

* use UserParty instead of Party
* simplify lastname handling

* get lastname from fullname

* use invariant culture

* rm redundant semi

* update summary for PersonSignee.FullName
Comment on lines +31 to +89
foreach (SigneeContext signeeContext in signeeContexts)
{
if (signeeContext.SigneeState.IsAccessDelegated is true)
{
try
{
DelegationRequest delegationRequest = new()
{
ResourceId = appResourceId.Value,
InstanceId = instanceGuid,
From = new DelegationParty
{
Value =
delegatorParty.PartyUuid.ToString()
?? throw new InvalidOperationException("Delegator: PartyUuid is null"),
},
To = new DelegationParty
{
Value =
signeeContext.Party.PartyUuid.ToString()
?? throw new InvalidOperationException("Delegatee: PartyUuid is null"),
},
Rights =
[
new RightRequest
{
Resource =
[
new AppResource { Value = appIdentifier.App },
new OrgResource { Value = appIdentifier.Org },
new TaskResource { Value = taskId },
],
Action = new AltinnAction { Value = ActionType.Read },
},
new RightRequest
{
Resource =
[
new AppResource { Value = appIdentifier.App },
new OrgResource { Value = appIdentifier.Org },
new TaskResource { Value = taskId },
],
Action = new AltinnAction { Value = ActionType.Sign },
},
],
};
DelegationResponse? response = await accessManagementClient.RevokeRights(delegationRequest, ct);
signeeContext.SigneeState.IsAccessDelegated = false;
telemetry?.RecordDelegationRevoke(DelegationResult.Success);
}
catch (Exception ex)
{
logger.LogError(ex, "Failed to revoke signee rights");
signeeContext.SigneeState.DelegationFailedReason = "Failed to revoke signee rights: " + ex.Message;
telemetry?.RecordDelegationRevoke(DelegationResult.Error);
success = false;
}
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
- consider filtering the sequence explicitly using '.Where(...)'.
},
],
};
DelegationResponse? response = await accessManagementClient.RevokeRights(delegationRequest, ct);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
response
is useless, since its value is never read.
Comment on lines +81 to +87
catch (Exception ex)
{
logger.LogError(ex, "Failed to revoke signee rights");
signeeContext.SigneeState.DelegationFailedReason = "Failed to revoke signee rights: " + ex.Message;
telemetry?.RecordDelegationRevoke(DelegationResult.Error);
success = false;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
},
],
};
DelegationResponse? response = await accessManagementClient.DelegateRights(delegationRequest, ct);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
response
is useless, since its value is never read.
Comment on lines +163 to +169
catch (Exception ex)
{
logger.LogError(ex, "Failed to delegate signee rights");
state.DelegationFailedReason = "Failed to delegate signee rights: " + ex.Message;
telemetry?.RecordDelegation(DelegationResult.Error);
success = false;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
{
using var activity = telemetry?.StartAppInstanceRevokeActivity();

HttpResponseMessage? httpResponseMessage = null;

Check notice

Code scanning / CodeQL

Missed 'using' opportunity Note

This variable is manually
disposed
in a
finally block
- consider a C# using statement as a preferable resource management technique.
Comment on lines +68 to +83
catch (Exception e)
{
var ex =
e is AccessManagementRequestException
? e
: new AccessManagementRequestException(
$"Something went wrong when processing the access management request.",
null,
httpResponseMessage?.StatusCode,
httpContent,
e
);
logger.LogError(ex, "Error when processing access management request.");

throw ex;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
{
using var activity = telemetry?.StartAppInstanceDelegationActivity();

HttpResponseMessage? httpResponseMessage = null;

Check notice

Code scanning / CodeQL

Missed 'using' opportunity Note

This variable is manually
disposed
in a
finally block
- consider a C# using statement as a preferable resource management technique.
Comment on lines +131 to +146
catch (Exception e)
{
var ex =
e is AccessManagementRequestException
? e
: new AccessManagementRequestException(
$"Something went wrong when processing the access management request.",
null,
httpResponseMessage?.StatusCode,
httpContent,
e
);
logger.LogError(ex, "Error when processing access management request.");

throw ex;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
@HauklandJ
Copy link
Contributor Author

/publish

Copy link

github-actions bot commented Jan 6, 2025

cammiida and others added 2 commits January 7, 2025 09:04
* add traces and metrics to notifications for signing

* extract getUserLanguage from pdfService

* add metrics

* add default notification bodies

* remove language support from signing notification

* cleanup usings

* update default sms body

* Populate email request, clarify sms sender number

* Add default email subject

* Add reference for email tracking

* Consolidate sms and email notification interfaces

* move signing notification default to signingnotificationservice

* add signing notification service tests

* robustify, add more tests

* robustify, add more tests

* break -> continue + test

* typo

* pr feedback, try send email if sms fails

* typo
@cammiida
Copy link
Contributor

/publish

Copy link

* fixes setting delegated status by inverting logic

* removes is
@cammiida
Copy link
Contributor

/publish

Copy link

HauklandJ and others added 2 commits January 16, 2025 10:25
* Download signDocuments and synchronize with SigneeContexts. Enables checking signature status.

* Download signature documents in parallell.

* Make sure to add sign document to on-the-fly signeeContexts.

* Move download signDocument into a method.

* Extract logic for creating SigneeContext from SignDocument to method.

* Add some validation to SigningProcessTask.Start and skip running signee initialization and process signees if there is  no point.

* Fix merge error.

* Use xUnit for asserts instead of FluentAssertions.
@bjorntore
Copy link
Contributor

/publish

Copy link

# Conflicts:
#	src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
#	test/Altinn.App.Api.Tests/OpenApi/swagger.yaml
_logger.LogError(
ex,
"Failed to download signature document for DataElement with ID {DataElementId}.",
signatureDataElement.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
.

Copilot Autofix AI 5 days ago

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

  1. Identify the line where signatureDataElement.Id is logged.
  2. Sanitize the Id by replacing newline characters with an empty string before logging it.
  3. Ensure that the fix does not alter the existing functionality of the code.
Suggested changeset 1
src/Altinn.App.Core/Features/Signing/SigningService.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/Features/Signing/SigningService.cs b/src/Altinn.App.Core/Features/Signing/SigningService.cs
--- a/src/Altinn.App.Core/Features/Signing/SigningService.cs
+++ b/src/Altinn.App.Core/Features/Signing/SigningService.cs
@@ -342,2 +342,3 @@
         {
+            string sanitizedId = signatureDataElement.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
             _logger.LogError(
@@ -345,3 +346,3 @@
                 "Failed to download signature document for DataElement with ID {DataElementId}.",
-                signatureDataElement.Id
+                sanitizedId
             );
EOF
@@ -342,2 +342,3 @@
{
string sanitizedId = signatureDataElement.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
_logger.LogError(
@@ -345,3 +346,3 @@
"Failed to download signature document for DataElement with ID {DataElementId}.",
signatureDataElement.Id
sanitizedId
);
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

public class SigningNotificationServiceTests
{
Mock<ILogger<SigningNotificationService>> _loggerMock = new();

Check notice

Code scanning / CodeQL

Missed 'readonly' opportunity Note test

Field '_loggerMock' can be 'readonly'.
Comment on lines +351 to +356
new HttpResponseMessage()
{
StatusCode = HttpStatusCode.Conflict,
ReasonPhrase = "Conflict",
Content = new StringContent("Failed to send SMS notification"),
},

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'HttpResponseMessage' is created but not disposed.
Comment on lines +398 to +403
new HttpResponseMessage()
{
StatusCode = HttpStatusCode.Conflict,
ReasonPhrase = "Conflict",
Content = new StringContent("Failed to send email notification"),
},

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'HttpResponseMessage' is created but not disposed.

_logger.LogInformation(
"Signing action handler invoked for instance {Id}. In task: {CurrentTaskId}",
context.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
.

Copilot Autofix AI 4 days ago

To fix the problem, we need to sanitize the context.Instance.Id before logging it. Since the log entries are plain text, we should remove any newline characters from the user input to prevent log forging. This can be done using the String.Replace method to replace newline characters with an empty string.

The best way to fix the problem without changing existing functionality is to modify the logging statement to sanitize the context.Instance.Id before it is logged. This involves updating the logging statement in the HandleAction method of the SigningUserAction class.

Suggested changeset 1
src/Altinn.App.Core/Features/Action/SigningUserAction.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/Features/Action/SigningUserAction.cs b/src/Altinn.App.Core/Features/Action/SigningUserAction.cs
--- a/src/Altinn.App.Core/Features/Action/SigningUserAction.cs
+++ b/src/Altinn.App.Core/Features/Action/SigningUserAction.cs
@@ -101,5 +101,6 @@
 
+        string sanitizedInstanceId = context.Instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
         _logger.LogInformation(
             "Signing action handler invoked for instance {Id}. In task: {CurrentTaskId}",
-            context.Instance.Id,
+            sanitizedInstanceId,
             currentTask.Id
EOF
@@ -101,5 +101,6 @@

string sanitizedInstanceId = context.Instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "");
_logger.LogInformation(
"Signing action handler invoked for instance {Id}. In task: {CurrentTaskId}",
context.Instance.Id,
sanitizedInstanceId,
currentTask.Id
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
Comment on lines +163 to +167
catch (Exception e)
{
// TODO: What do we do here? This failure is pretty silent... but throwing would cause havoc
_logger.LogError(e, "Correspondence send failed: {Exception}", e.Message);
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
ApplicationMetadata appMetadata,
AltinnSignatureConfiguration? signatureConfiguration,
IHostEnvironment hostEnvironment,
IAltinnCdnClient? altinnCdnClient = null

Check notice

Code scanning / CodeQL

Missed 'using' opportunity Note

This variable is manually
disposed
in a
finally block
- consider a C# using statement as a preferable resource management technique.
Comment on lines +48 to +90
var responseMessage = new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new StringContent(
"""
{
"orgs": {
"ttd": {
"name": {
"en": "Test Ministry",
"nb": "Testdepartementet",
"nn": "Testdepartementet"
},
"logo": "",
"orgnr": "",
"homepage": "",
"environments": [
"at22",
"at23",
"at24",
"tt02",
"yt01",
"production"
]
},
"udi": {
"name": {
"en": "The Norwegian Directorate of Immigration",
"nb": "Utlendingsdirektoratet",
"nn": "Utlendingsdirektoratet"
},
"logo": "https://altinncdn.no/orgs/udi/udi.png",
"orgnr": "974760746",
"homepage": "https://www.udi.no",
"environments": [
"tt02",
"production"
]
}
}
}
"""
),
};

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'HttpResponseMessage' is created but not disposed.
public async Task GetOrgs_ThrowsJsonException_WhenResponseIsLiteralNull()
{
// Arrange
var httpClientFactoryMock = new Mock<IHttpClientFactory>();

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning test

This assignment to
httpClientFactoryMock
is useless, since its value is never read.
ItExpr.IsAny<HttpRequestMessage>(),
ItExpr.IsAny<CancellationToken>()
)
.ReturnsAsync(new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent("null") });

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'HttpResponseMessage' is created but not disposed.
public async Task GetOrgs_CancellationTokenIsRespected()
{
// Arrange
var httpClientFactoryMock = new Mock<IHttpClientFactory>();

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning test

This assignment to
httpClientFactoryMock
is useless, since its value is never read.
{
// Arrange
var httpClientFactoryMock = new Mock<IHttpClientFactory>();
var cancellationTokenSource = new CancellationTokenSource();

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'CancellationTokenSource' is created but not disposed.
ItExpr.IsAny<HttpRequestMessage>(),
ItExpr.IsAny<CancellationToken>()
)
.ReturnsAsync(new HttpResponseMessage(HttpStatusCode.OK));

Check warning

Code scanning / CodeQL

Missing Dispose call on local IDisposable Warning test

Disposable 'HttpResponseMessage' is created but not disposed.
public void IsNullOrEmpty_ReturnsTrue_WhenEnumerableIsNull()
{
IEnumerable<string>? source = null;
var result = source.IsNullOrEmpty();

Check failure

Code scanning / CodeQL

Dereferenced variable is always null Error test

Variable
source
is always null at this dereference.
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
53.0% Coverage on New Code (required ≥ 65%)
E Reliability Rating on New Code (required ≥ A)
46.59% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@HauklandJ
Copy link
Contributor Author

/publish

Copy link

github-actions bot commented Jan 21, 2025

PR release:

⚙️ Building...
✅ Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Label Pull requests with new features. Used when generation releasenotes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants