Skip to content

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented Aug 2, 2019

<exception cref="T:System.Security.Cryptography.CryptographicException">The hash algorithm option specified by the PKCS#12 PFX contents could not be identified or is not supported by this platform.</exception>
</Docs>
</Member>
<Member MemberName="VerifyMac">
Copy link
Member Author

Choose a reason for hiding this comment

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

@mairaw or @rpetrusha : This file doesn't have a MethodGroup entry for this overloaded member (and I feel like I've seen other new API in that state). I don't know if that means a) we're not bothering with them, b) there's an error in the ingestor which results in it not producing them c) there's an error in the validator that makes it not report them as missing d) other e) some multiple combination of a-d.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the MemberGroup element to add summary, remarks, etc. that apply to all overloads?
If so, the reflection tool doesn't add them automatically and we're not required to document them.

When we want to avoid repeating the same text, we manually add them.

If I misunderstood, please let me know.

Choose a reason for hiding this comment

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

It seems, though, that the <summary> element of a member group is ignored if there aren't some <AssemblyInfo> elements present.

@mairaw
Copy link
Contributor

mairaw commented Aug 2, 2019

Made some small changes. Please take a look.

@mairaw mairaw added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Aug 2, 2019
@mairaw mairaw added this to the August 2019 milestone Aug 2, 2019
@bartonjs
Copy link
Member Author

bartonjs commented Aug 2, 2019

Changes look good to me. As does the explanation about the MemberGroup element.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

This looks really good, @bartonjs. I've left some minor suggestions and added a <MemberGroup> section that you can add a <remarks> section to (hopefully I didn't create some badly formatted XML). A number of links are broken in the build, though I can't see any reason why. I've suggested adding a space between the closing quotation mark and the end tag in crefs in the event that that fixes the problem, though I rather doubt that it will.

@bartonjs
Copy link
Member Author

bartonjs commented Aug 3, 2019

Ah, being removed from the text for a couple hours I see I wrote "Pkcs12" too many times... it's "S.S.C.Pkcs.Pkcs12Info", not "S.S.C.Pkcs12.Pkcs12Info". I'll remove the extraneous 12s

Copy link
Member Author

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Removing all of the erroneous 12s and adding a MemberGroup remark via the suggestions interface.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've left some additional comments and one suggestion for you to consider, @bartonjs.

Therefore, there is no guarantee that a password which results in a `true` return from this method will succeed on a call to <xref:System.Security.Cryptography.Pkcs.Pkcs12SafeContents.Decrypt%2A>.
In the PKCS#12 specification, a distinction is made between a `null` password and an "empty" password, and that difference is reflected in the return value of this method.
If the `null` string returns `true`, then <xref:System.String.Empty> returns `false`, and vice versa.

Choose a reason for hiding this comment

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

Suggested change
If the `null` string returns `true`, then <xref:System.String.Empty> returns `false`, and vice versa.
If the `null` string returns `true`, then <xref:System.String.Empty?displayProperty=nameWithType> returns `false`, and vice versa.

## Remarks
It's not possible to distingush the error due to the password being incorrect from the error due to the contents having been altered.

Choose a reason for hiding this comment

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

Since these two paragraphs now appear in the member group, they can be removed from here.

## Remarks
It's not possible to distinguish the error due to the password being incorrect from the error due to the contents having been altered.

Choose a reason for hiding this comment

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

Since these two paragraphs now appear in the member group, they can be removed from here.

@rpetrusha rpetrusha merged commit 72af7a5 into dotnet:master Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants