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

WebDAV System Module #21106

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
3 changes: 3 additions & 0 deletions Modules/System/WebDAV/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# WebDAV

This module provides functions to interact with WebDAV API
27 changes: 27 additions & 0 deletions Modules/System/WebDAV/app.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"id": "199527d2-53dd-48e1-8732-8850d4f9e45b",
"name": "WebDAV",
"publisher": "Microsoft",
"brief": "Connect to WebDAV.",
"description": "Provides a set of AL functionality and Helper libraries to make use of WebDAV API",
"version": "21.0.0.0",
"privacyStatement": "https://go.microsoft.com/fwlink/?linkid=724009",
"EULA": "https://go.microsoft.com/fwlink/?linkid=2009120",
"help": "https://go.microsoft.com/fwlink/?linkid=2103698",
"url": "https://go.microsoft.com/fwlink/?linkid=724011",
"logo": "",
"dependencies": [

],
"screenshots": [],
"platform": "21.0.0.0",
"application": "21.0.0.0",
"idRanges": [
{
"from": 5678,
"to": 5699
}
],
"target": "OnPrem",
"contextSensitiveHelpUrl": "/dynamics365/business-central/"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
interface "WebDAV Authorization"
{
procedure Authorize(var HttpRequestMessage: HttpRequestMessage);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
codeunit 5681 "WebDAV Basic Auth. Impl." implements "WebDAV Authorization"
{
Access = Internal;

var
[NonDebuggable]
AuthorizationString: Text;

[NonDebuggable]
procedure Authorize(var HttpRequestMessage: HttpRequestMessage);
var
Headers: HttpHeaders;
begin
HttpRequestMessage.GetHeaders(Headers);
Headers.Remove('Authorization');
Headers.Add('Authorization', AuthorizationString);

Headers.Remove('Translate');
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: This doesn't seem to be related to Authorization? Could either be moved to a different function or Rename existing function to explain what happens. I believe splitting would be best.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what you mean?

Copy link

Choose a reason for hiding this comment

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

I think he is talking about the "Translate" header that is added with the value "F".
Why is this done? Is it related to authorization?

Headers.Add('Translate', 'F');
end;

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, double newline


[NonDebuggable]
procedure SetUserNameAndPassword(Username: Text; Password: Text)
var
Base64Convert: Codeunit "Base64 Convert";
begin
AuthorizationString := 'Basic ' + Base64Convert.ToBase64(Username + ':' + Password);
end;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
codeunit 5680 "WebDAV Basic Authorization"
{
Access = Public;

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is the public facade, I would add xml comments to the codeunit and the procedure here.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

procedure GetWebDAVBasicAuth(Username: Text; Password: Text): Interface "WebDAV Authorization"
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern kind of defeats the NonDebuggable in the implementation codeunit, since you are passing the credentials as plaintext in the argument. One option to make this more secure is to consider storing the password in the isolated storage and retrieving it from there in the implementation. Would that be a viable option?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing is happening at the SharePoint Auth., the client secret is added in plain text.

This is a Basic Auth interface. If I add an isolated storage implementation I force everybody to use this isolated storage implemenation.

Maybe I could force a Base64 encoding of the password? Would that be okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

Base64 is not a good security measure. Perhaps making the procedure in the public facade non-debuggable would be enough, since at that point, the process as far as this implementation goes should be reasonable secure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Base64 isn't a good security measure but that's how Authentication header Basic works, https://www.rfc-editor.org/rfc/rfc7235#section-1.1.

I don't think anyone likes credentials that can be easily seen/retrieved. There is a new datatype that may come in wave 2 release (v23) which securely prevents the values from being seen. But until then, adding NonDebuggable is the way to go. If the username/passwords are being stored in application, I'd recommend to also provide a IsolatedStorage way to set and then auth with them without retrieving it from DB which can lead to insecurely storing them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add [NonDebuggable]

var
WebDAVBasicAuthImpl: Codeunit "WebDAV Basic Auth. Impl.";
begin
WebDAVBasicAuthImpl.SetUserNameAndPassword(Username, Password);
Exit(WebDAVBasicAuthImpl);
Copy link
Contributor

Choose a reason for hiding this comment

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

No capitalization is needed for "exit"

Copy link
Contributor

Choose a reason for hiding this comment

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

corrected

end;

}
77 changes: 77 additions & 0 deletions Modules/System/WebDAV/src/Helper/WebDAVDiagnostics.Codeunit.al
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

/// <summary>
/// Stores detailed information about failed api call
/// </summary>
codeunit 5685 "WebDAV Diagnostics" implements "HTTP Diagnostics"
{
Access = Internal;

var
ErrorMessage, ResponseReasonPhrase : Text;
HttpStatusCode, RetryAfter : Integer;
SuccessStatusCode: Boolean;

/// <summary>
/// Gets reponse details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same summary all along, also please make return value more human readable.

/// </summary>
/// <returns>HttpResponseMessage.IsSuccessStatusCode</returns>
[NonDebuggable]
procedure IsSuccessStatusCode(): Boolean
begin
exit(SuccessStatusCode);
end;

/// <summary>
/// Gets response details.
/// </summary>
/// <returns>HttpResponseMessage.StatusCode</returns>
[NonDebuggable]
procedure GetHttpStatusCode(): Integer
begin
exit(HttpStatusCode);
end;

/// <summary>
/// Gets response details.
/// </summary>
/// <returns>Retry-after header value</returns>
[NonDebuggable]
procedure GetHttpRetryAfter(): Integer
begin
exit(RetryAfter);
end;

/// <summary>
/// Gets reponse details
/// </summary>
/// <returns>Error message</returns>
[NonDebuggable]
procedure GetErrorMessage(): Text
begin
exit(ErrorMessage);
end;

/// <summary>
/// Gets response details.
/// </summary>
/// <returns>HttpResponseMessage.ResponseReasonPhrase</returns>
[NonDebuggable]
procedure GetResponseReasonPhrase(): Text
begin
exit(ResponseReasonPhrase);
end;

[NonDebuggable]
internal procedure SetParameters(NewIsSuccesss: Boolean; NewHttpStatusCode: Integer; NewResponseReasonPhrase: Text; NewRetryAfter: Integer; NewErrorMessage: Text)
Copy link
Contributor

Choose a reason for hiding this comment

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

Codeunit access modifier is already Internal. You can remove the access modifier here.

Copy link
Contributor

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see the internal modifier

begin
SuccessStatusCode := NewIsSuccesss;
HttpStatusCode := NewHttpStatusCode;
ResponseReasonPhrase := NewResponseReasonPhrase;
RetryAfter := NewRetryAfter;
ErrorMessage := NewErrorMessage;
end;
}
76 changes: 76 additions & 0 deletions Modules/System/WebDAV/src/Helper/WebDAVHttpContent.Codeunit.al
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

codeunit 5686 "WebDAV Http Content"
{
Access = Internal;

var
HttpContent: HttpContent;
ContentLength: Integer;
ContentType: Text;
RequestDigest: Text;

procedure FromFileInStream(var ContentInStream: Instream)
begin
HttpContent.WriteFrom(ContentInStream);
ContentLength := GetContentLength(ContentInStream);
end;

procedure FromText(Content: Text)
var
InStream: InStream;
begin
HttpContent.WriteFrom(Content);
HttpContent.ReadAs(InStream);
ContentLength := GetContentLength(InStream);
end;

procedure FromXML(XMLDoc: XmlDocument)
var
RequestText: Text;
begin
XMLDoc.WriteTo(RequestText);
HttpContent.WriteFrom(RequestText);
ContentLength := StrLen(RequestText);
end;

procedure GetContent(): HttpContent
begin
exit(HttpContent);
end;

procedure GetContentLength(): Integer
begin
exit(ContentLength);
end;

procedure GetContentType(): Text
begin
exit(ContentType);
end;

// TODO DIGEST??
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO

procedure SetRequestDigest(RequestDigestValue: Text)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any use of RequestDigest... is this needed?

begin
RequestDigest := RequestDigestValue;
end;

procedure GetRequestDigest(): Text;
begin
exit(RequestDigest);
end;

local procedure GetContentLength(var SourceInStream: InStream) Length: Integer
var
MemoryStream: DotNet MemoryStream;
begin
MemoryStream := MemoryStream.MemoryStream();
CopyStream(MemoryStream, SourceInStream);
Length := MemoryStream.Length;
Clear(SourceInStream);
end;

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

codeunit 5684 "WebDAV Operation Response"
{
Access = Internal;

[NonDebuggable]
[TryFunction]
Copy link
Contributor

Choose a reason for hiding this comment

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

if you check the output of read, it won't throw any error and you can simply return a boolean instead of relying on throwing and catching an error. It's much nicer to handle it instead.

internal procedure GetResultAsText(var Result: Text);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the general pattern is to prefix try procedures with the word Try, so TryGetResultAsText.

Also, since the codeunit access modifier is Internal, the access modifiers for the procedures are superfluous.

Copy link
Contributor

Choose a reason for hiding this comment

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

prefix added and NonDEbuggable removed

var
ResultInStream: InStream;
begin
TempBlobContent.CreateInStream(ResultInStream);
ResultInStream.Read(Result);
end;

[NonDebuggable]
[TryFunction]
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this cannot throw any errors, no need for tryfunction

internal procedure GetResultAsStream(var ResultInStream: InStream)
begin
TempBlobContent.CreateInStream(ResultInStream);
end;

[NonDebuggable]
internal procedure SetHttpResponse(HttpResponseMessage: HttpResponseMessage)
var
ContentOutStream: OutStream;
ContentInStream: InStream;
begin

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whitespace can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace removed

TempBlobContent.CreateOutStream(ContentOutStream);
HttpResponseMessage.Content().ReadAs(ContentInStream);
CopyStream(ContentOutStream, ContentInStream);
HttpHeaders := HttpResponseMessage.Headers();

WebDAVDiagnostics.SetParameters(HttpResponseMessage.IsSuccessStatusCode, HttpResponseMessage.HttpStatusCode, HttpResponseMessage.ReasonPhrase, GetRetryAfterHeaderValue(), GetErrorDescription());
end;

[NonDebuggable]
internal procedure SetHttpResponse(ResponseContent: Text; ResponseHttpHeaders: HttpHeaders; ResponseHttpStatusCode: Integer; ResponseIsSuccessStatusCode: Boolean; ResponseReasonPhrase: Text)
var
ContentOutStream: OutStream;
begin
TempBlobContent.CreateOutStream(ContentOutStream);
ContentOutStream.WriteText(ResponseContent);
HttpHeaders := ResponseHttpHeaders;
WebDAVDiagnostics.SetParameters(ResponseIsSuccessStatusCode, ResponseHttpStatusCode, ResponseReasonPhrase, GetRetryAfterHeaderValue(), GetErrorDescription());
end;

[NonDebuggable]
internal procedure GetHeaderValueFromResponseHeaders(HeaderName: Text): Text
var
Values: array[100] of Text;
begin
if not HttpHeaders.GetValues(HeaderName, Values) then
Copy link
Contributor

Choose a reason for hiding this comment

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

Use list of text instead, no reason to define a large array of 100 to get this value

exit('');
exit(Values[1]);
end;

[NonDebuggable]
internal procedure GetRetryAfterHeaderValue() RetryAfter: Integer;
var
HeaderValue: Text;
begin
HeaderValue := GetHeaderValueFromResponseHeaders('Retry-After');
if HeaderValue = '' then
exit(0);
if not Evaluate(RetryAfter, HeaderValue) then
exit(0);
end;

[NonDebuggable]
local procedure GetErrorDescription(): Text
var
Result: Text;
JObject: JsonObject;
JToken: JsonToken;
begin
GetResultAsText(Result);
if Result <> '' then
if JObject.ReadFrom(Result) then
if JObject.Get('error_description', JToken) then
exit(JToken.AsValue().AsText());
end;

[NonDebuggable]
internal procedure GetDiagnostics(): Interface "HTTP Diagnostics"
begin
exit(WebDAVDiagnostics);
end;

var
Copy link
Contributor

Choose a reason for hiding this comment

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

Globals in codeunits are generally placed at the top in Microsoft codeunits.

Copy link
Contributor

Choose a reason for hiding this comment

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

moved the variables

TempBlobContent: Codeunit "Temp Blob";
WebDAVDiagnostics: Codeunit "WebDAV Diagnostics";
HttpHeaders: HttpHeaders;
}
Loading