WebDAV System Module #21106
WebDAV System Module #21106
Conversation
SHBBusinessSolutions
requested review from
JesperSchulz,
darjoo,
IhorHandziuk,
stkillen and
AndreasMoth
as code owners
|
@SHBBusinessSolutions you already mention good examples. Additionally you should focus on secure authentication patterns and avoid Support for Basic Authentication. |
|
@pri-kise regarding authentication, there's not a lot of options with WebDAV available. We'll take a look a digest authentication and Microsoft should decide if we should support NTLM / kerberos. But basic auth should be available in my opinion. The usage scenarios that we see include simple 3rd party OnPrem solutions where Basic Auth would be the lowest common denominator. |
|
@JesperSchulz |
|
Generally, it sounds like a very useful module to have! But I worry a little about the request for Microsoft's involvement in this case, as I can't really free up any resources at the moment to provide special support for this pull request. |
I'm not sure what the "usual support" would be ;-) I at least would need the app-manifest(s) prepared from you guys. And some guidance on what to do with test automation, since it's an external API we're connecting to: Should there be a fixed test-webdav instance somewhere or a powershell script to setup Webdav on a Docker IIS or do we need a complete mock interface? And maybe an input on what authentication types to support |
|
@JulianTillmann "usual support" normally involves review of code and after some draft of your pull request, suggestions for refactoring / restructuring. There are at least a few major principles. I'm no expert in testing, but I received some feedback for my PR from @adrogin . |
|
We've also got some documentation on how to get started with a new module here: http://aka.ms/newalmodule. |
Microsoft Aheads
|
|
okay, so I made a prototype and tested it manually against IIS Webdav and Nextcloud WebDAV with BasicAuth and leaning heavily on the Sharepoint API system app. There are basically two parts on which I need your input:
Looking forward to your answers |
There was a problem hiding this comment.
This is an exciting idea and it certainly looks like a good start. I did notice a few generic things (I haven't flagged all of them individually):
- The MIT heading was missing in some of the objects
- Exit should not be capitalized
- A lot of superfluous whitespace in several places
- Missing xml documentation tags for public codeunits
| { | ||
| Access = Public; | ||
|
|
||
| procedure GetWebDAVBasicAuth(Username: Text; Password: Text): Interface "WebDAV Authorization" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| codeunit 5680 "WebDAV Basic Authorization" | ||
| { | ||
| Access = Public; | ||
|
|
There was a problem hiding this comment.
Since this is the public facade, I would add xml comments to the codeunit and the procedure here.
| WebDAVBasicAuthImpl: Codeunit "WebDAV Basic Auth. Impl."; | ||
| begin | ||
| WebDAVBasicAuthImpl.SetUserNameAndPassword(Username, Password); | ||
| Exit(WebDAVBasicAuthImpl); |
There was a problem hiding this comment.
No capitalization is needed for "exit"
| end; | ||
|
|
||
| [NonDebuggable] | ||
| internal procedure SetParameters(NewIsSuccesss: Boolean; NewHttpStatusCode: Integer; NewResponseReasonPhrase: Text; NewRetryAfter: Integer; NewErrorMessage: Text) |
There was a problem hiding this comment.
Codeunit access modifier is already Internal. You can remove the access modifier here.
There was a problem hiding this comment.
I still see the internal modifier
|
|
||
| [NonDebuggable] | ||
| [TryFunction] | ||
| internal procedure GetResultAsText(var Result: Text); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
prefix added and NonDEbuggable removed
| local procedure GetNextEntryNo(): Integer; | ||
| begin | ||
| NextEntryNo += 1; | ||
| Exit(NextEntryNo); |
|
|
||
| local procedure GetLevelFromPath(Path: Text): Integer | ||
| begin | ||
| Exit(Path.Split('/').Count); |
| @@ -0,0 +1,84 @@ | |||
| codeunit 5678 "WebDAV Client" | |||
There was a problem hiding this comment.
This is the facade, so xml documentation would need to be added here. (Also MIT stuff)
There was a problem hiding this comment.
XML documentation and MIT headers added
| WebDAVClientImpl.Initialize(BaseUrl, WebDAVAuthorization); | ||
| end; | ||
|
|
||
| // [NonDebuggable] |
There was a problem hiding this comment.
Should all these be here or not?
There was a problem hiding this comment.
removed NonDebuggable
There was a problem hiding this comment.
I see that the NonDebuggable are commented out, remove them entirely if not needed.
| Access = Internal; | ||
|
|
||
| var | ||
|
|
|
@PeterConijn but I'm still not getting answers to my two fundemental questions:
|
|
To start with your second question: you cannot test an actual web call, since it requires external connection. What I often do when testing such functionality is two things:
As far as authentication goes: that might bear a bit more thought. Basic is still being used a lot, but OAuth 2.0 is gaining traction quickly. Perhaps - and this is just an idea - we can redesign the authentication to use an interface structure, so that we can provide some methods, but it is extensible if someone needs a more exotic method. I will be away for a couple of weeks and not be able to re-review until after, so I hope you'll forgive my lack of response after this one :) I missed it in the CR, but given the nature of the funtionality, your test app will likely need access to your module's internals, so you may need to add the internalsVisibleTo flag in your app.json. |
|
@PeterConijn mocking up a xml response seems time consuming and I would there's a big chance I would accidentaly create this mock up so that the test works. A real scenario would be better, but if that's okay with you than I'll do the mock testing regarding authentication But I'll put this in the backlog until you're back and concentrate on the testing for now |
|
From my personal account, but in addition to my comment on testing APIs, you might find this video by @lvanvugt handy: https://www.youtube.com/watch?v=P-8MOfNppVQ |
|
@JulianTillmann you can have a look at the sharepoint test codeunit. It makes use of manual event subscribers to mock the requests. |
|
@JesperSchulz can maybe someone of microsoft have a look at the pull request, too. @JulianTillmann could maybe provide some example could how to call the public procedure and how to initialize the authentication? |
| 'COPY': | ||
| GetSuccessfullResponse(Uri, WebDAVOperationResponse); | ||
| 'MOVE': | ||
| GetSuccessfullResponse(Uri, WebDAVOperationResponse); |
There was a problem hiding this comment.
Teeny thing, but Successful is with one l
| end; | ||
|
|
||
| [NonDebuggable] | ||
| procedure Propfind(Uri: Text; Recursive: Boolean) WebDAVOperationResponse: Codeunit "WebDAV Operation Response"; |
| { | ||
| Caption = 'Level'; | ||
| } | ||
| field(20; "Is Collection"; Boolean) |
There was a problem hiding this comment.
I agree that it happens with older NAV objects, but not sure that is a reason to continue the practice. I will defer to Microsoft's opinion on the matter though.
| { | ||
| Caption = 'Content Length'; | ||
| } | ||
| field(30; "Creation Date"; DateTime) |
|
Indeed looks like this PR now is in a shape, which requires us to look into it. We've implemented a new guideline to not start reviewing, before two partners have approved a PR. Hopefully that will allow things to scale better :-) |
|
@JesperSchulz I still have the questions about authentication ... |
|
The next major is more or less locked down (for new features). This would be a candidate for a minor, following the next major. I'll see if we can find someone to drive this forward, when the 2023 Wave 1 is done and dusted. Should be soon. Then we'll take care of all open questions too. |
| /// <param name="Username">Text.</param> | ||
| /// <param name="Password">Text.</param> | ||
| /// <returns>Return value of type Interface "WebDAV Authorization".</returns> | ||
| procedure GetWebDAVBasicAuth(Username: Text; Password: Text): Interface "WebDAV Authorization" |
| { | ||
| Access = Public; | ||
|
|
||
| procedure GetWebDAVBasicAuth(Username: Text; Password: Text): Interface "WebDAV Authorization" |
There was a problem hiding this comment.
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.
| end; | ||
|
|
||
| [NonDebuggable] | ||
| internal procedure SetParameters(NewIsSuccesss: Boolean; NewHttpStatusCode: Integer; NewResponseReasonPhrase: Text; NewRetryAfter: Integer; NewErrorMessage: Text) |
There was a problem hiding this comment.
I still see the internal modifier
| RequestMessage := PrepareRequestMsg('PROPFIND', Uri, WebDAVHttpContent); | ||
| RequestMessage.GetHeaders(Headers); | ||
|
|
||
| // TODO Depth = 0? |
There was a problem hiding this comment.
Is this something the developers/admin etc should be able to set?
| HttpContent := WebDAVHttpContent.GetContent(); | ||
| HttpContent.GetHeaders(Headers); | ||
|
|
||
| if Headers.Contains(GetContentType) then |
There was a problem hiding this comment.
Add () to the end of function calls.
All the calls to GetContentType etc
| { | ||
| Caption = 'Level'; | ||
| } | ||
| field(20; "Is Collection"; Boolean) |
There was a problem hiding this comment.
I don't have a strong opinion on this as long as it makes sense. Internally, unless it's a localized (US, DK etc) field, we've been going sequentially for new tables in general.
I do notice it jumps from 20 to 22. Any reason 21 is missing?
| WebDAVClientImpl.Initialize(BaseUrl, WebDAVAuthorization); | ||
| end; | ||
|
|
||
| // [NonDebuggable] |
There was a problem hiding this comment.
I see that the NonDebuggable are commented out, remove them entirely if not needed.
|
@darjoo But there is still the big question open if we need to provide more than BasicAuth for WebDAV? There is NTLM available, but that's an OnPrem thing and there is a rather complicated "DigestAuth" that's possible with WebDAV. |
|
@JulianTillmann Do we know how widely used WebDAV would be used with onprem customers? I'm not sure if that'd be worth the effort at the beginning unless we know there's demand for that with the OnPrem customers. I did take a brief look at DigestAuth, and initially I was going to recommend to implement it as well. Maybe you could take a look to see if you could estimate how much effort that would take? It did look complicated as you mentioned. I'm sure all customers will always prefer secure authentication methods, if we could add it. |
|
@darjoo I'll take another look at DigestAuthentication when I get to it. Maybe we could ship this WebDAV module with BasicAuth and deliver Digest later? |
|
@JulianTillmann Yeah, that's also fine to deliver DigestAuth later. |
| Headers.Remove('Authorization'); | ||
| Headers.Add('Authorization', AuthorizationString); | ||
|
|
||
| Headers.Remove('Translate'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't understand what you mean?
There was a problem hiding this comment.
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.Remove('Translate'); | ||
| Headers.Add('Translate', 'F'); | ||
| end; | ||
|
|
| { | ||
| Access = Public; | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Docs should explain what the function does, what the parameters and return value mean, not the type that can be seen from the procedure. Ex. (I did not spend a lot of time on this so feel free to improve) Summary: "Creates WebDAV Authorization initialized with the given username and password.", Username: "The name of the user that is authenticating the call", same for password, return: "A WebDAV Authorization interface initialized with the credentials".
| SuccessStatusCode: Boolean; | ||
|
|
||
| /// <summary> | ||
| /// Gets reponse details. |
There was a problem hiding this comment.
Same summary all along, also please make return value more human readable.
| end; | ||
|
|
||
| // TODO DIGEST?? | ||
| procedure SetRequestDigest(RequestDigestValue: Text) |
There was a problem hiding this comment.
I don't see any use of RequestDigest... is this needed?
| // ------------------------------------------------------------------------------------------------ | ||
| codeunit 5682 "WebDAV Content Parser" | ||
| { | ||
|
|
|
|
||
| if OnlyCollections then | ||
| if not WebDAVContent."Is Collection" then | ||
| exit; |
There was a problem hiding this comment.
I think it makes sense to return whether the single was parsed or not. In this case exit(false)
| exit(XmlNode.SelectSingleNode(PropertyName, XmlNamespaceManager, ChildNode)); | ||
| end; | ||
|
|
||
| local procedure GetNextEntryNo(): Integer; |
There was a problem hiding this comment.
This assumes all entries are created within the context of the same codeunit. Would be better if we in the caller just had a FindLast() initially, then init() and then increase the entry number by 1 if it doesn't already auto-increment.
| exit(true); | ||
| end; | ||
|
|
||
| procedure GetFilesAndCollections(var WebDAVContent: Record "WebDAV Content"; Recursive: Boolean): Boolean |
There was a problem hiding this comment.
All the "WebDAV Content Parser" functions are NonDebuggable, such as the Initialize, if that shouldn't be debuggable, then these functions shouldn't be debuggable either.
Personally I don't see any reason why the Initialize function is NonDebuggable though, so maybe that one should have the tag removed.
| end; | ||
|
|
||
| [TryFunction] | ||
| procedure TryGetResponseAsText(var Response: Text) |
There was a problem hiding this comment.
Again these two functions should just return the response directly instead of using a by-var variable.
|
Hi @JulianTillmann, I'm trying to push all of the open pull requests into the product - and this one has been around for a while 😊 I can see there are quite a few unaddressed code review comments; some which must be looked at before we can proceed with this PR. When do you think you'll find the time wrap this module up? We're not in a hurry as such, but ideally I'd love to get this one merged before its 1 year anniversary 😋 Let me know how I can help get this wrapped up! Looking forward to hear back from you! |
|
@JesperSchulz also this has not been the best experience contributing, lot's of "style" feedback but questions about content not beeing answered. And it doesn't seem like anybody really test it in a real scenario, only mock tests are expected. |
|
Sorry to hear that, Julian. All of it. |
JesperSchulz
added
the
system application
@JesperSchulz
Would you be interested in getting a System Application module for connecting to WebDAV like the Azure Blob Storage or Sharepoint API apps?
We already connected a PTE and do have a bit of experience in that regard. But we would need a bit of structure on how to create the app "in the Microsoft way" and on how to create the test automation.