[AuthenticationServices] Updates for Xcode13 Beta3 #12203
Conversation
tj-devel709
added
the
note-highlight
src/authenticationservices.cs
Outdated
|
|
||
| [Mac (12,0), iOS (15,0), MacCatalyst (15,0)] | ||
| [Export ("privateKeys")] | ||
| NSObject[] PrivateKeys { get; } |
There was a problem hiding this comment.
+/*! @abstract Private SecKeys returned from the AuthenticationServices extension.
+ */
+@property (nonatomic, readonly) NSArray *privateKeys API_AVAILABLE(ios(15.0), macos(12.0)) API_UNAVAILABLE(tvos, watchos);
SecKey is a type - so it should be an array of them
There was a problem hiding this comment.
Hmm, so I see that SecKey is created in this manual class here src/Security/Certificate.cs.
The namespace is Security, but when I add 'using Security' the compiler still says that it cannot find the reference.
There was a problem hiding this comment.
That might require some changes in the generator. It does not seems to allow SecKey at the moment.
Still it's important because SecKey is not a subclass of NSObject, so the binding are incorrect without that fix.
There was a problem hiding this comment.
Should I create an issue and link that in the binding?
There was a problem hiding this comment.
yes, comment the bindings and create an issue
also add it to the checklist for xcode13
There was a problem hiding this comment.
Clarification: leave a comment saying this should be SecKey or comment out this whole property and leave a note there about it?
There was a problem hiding this comment.
- Comment the property binding
- Add a link to the issue you created inside the comment
- Add a link to the issue to Xcode 13: list of changes to be reviewed/reverted before final release #11883
Why ?
- It's not usable as an
NSObject(sinceSecKeyis not a subclass of it) - if we forget it (or lack time to update) then we're not creating a future breaking change
It's always easier to add (later) than to remove :)
IOW never add something "new" that's not right / needs to be fixed
There was a problem hiding this comment.
Not an issue with the generator. SecKey was in a #if COREBUILD moving the #if to provide the minimum fixed the problem. I have pushed the fix.
| [DisableDefaultCtor] | ||
| interface ASAuthorizationPlatformPublicKeyCredentialAssertionRequest : ASAuthorizationPublicKeyCredentialAssertionRequest | ||
| { | ||
| [Sealed] |
There was a problem hiding this comment.
why ? iow worth a comment
There was a problem hiding this comment.
So I actually don't remember told me to add the Sealed or if I saw a suggestion somewhere, perhaps @dalexsoto told me?
But when I remove it, I get these build errors: https://gist.github.com/tj-devel709/d459d9e717a186486deab6479f57c050
There was a problem hiding this comment.
That error is because you're using the same selector for multiple methods/properties.
Why is that called PlatformAllowedCredentials if the selector is allowedCredentials ?
There was a problem hiding this comment.
Oh I just remembered what is going on! Alex did help me with this one a few weeks ago!
So there are two different classes that use this similar "allowedCredentials" header: ASAuthorizationPlatformPublicKeyCredentialAssertionRequest and ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest
The return type of the former is ASAuthorizationPlatformPublicKeyCredentialDescriptor[]
and the return type of the second one is ASAuthorizationSecurityKeyPublicKeyCredentialDescriptor[]
but they both are overriding a property from the protocol ASAuthorizationPublicKeyCredentialAssertionRequest
which has the base property "allowedCredentials" that is of type IASAuthorizationPublicKeyCredentialDescriptor[]
So to get past this typing confusion, Alex suggested giving them different names and sealing the properties in the child classes. I definitely should have made a comment because I had already forgotten about this.
There was a problem hiding this comment.
That used to be a problem but C# now have covariant returns
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/covariant-returns
I wonder if just using [Override] would work...
or if more change are needed
The main problem with this rename-approach is that it's not easily discoverable by developers - so they might still use the original (base) one and have to deal with casts
There was a problem hiding this comment.
Using just the Override attribute gives me the following errors
I was wondering if maybe the protocol interface method needs a virtual attribute?
[The Sealed Attribute] Instructs the generator to flag the generated method as sealed.
If this attribute is not specified, the default is to generate a virtual method
(either a virtual method, an abstract method or an override depending on how other attributes are used).
This method is marked abstract and is maybe not virtual by default because of that?
There was a problem hiding this comment.
it's not a big deal, let's land this as-is (unless someone else objects) and I might have a have a quick look
| [DisableDefaultCtor] | ||
| interface ASAuthorizationSecurityKeyPublicKeyCredentialAssertionRequest : ASAuthorizationPublicKeyCredentialAssertionRequest | ||
| { | ||
| [Sealed] |
src/authenticationservices.cs
Outdated
| NSData HttpBody { get; set; } | ||
|
|
||
| [Export ("privateKeys", ArgumentSemantic.Assign)] | ||
| NSObject[] PrivateKeys { get; set; } |
src/authenticationservices.cs
Outdated
| IntPtr Constructor (nint algorithm); | ||
|
|
||
| [Export ("algorithm")] | ||
| nint Algorithm { get; } |
There was a problem hiding this comment.
+@property (nonatomic, readonly) ASCOSEAlgorithmIdentifier algorithm;
why not use the type ?
There was a problem hiding this comment.
I cannot find this type in our bindings using grep
There was a problem hiding this comment.
it might be new... not sure why xtro did not pick it up
you need to track the header files to see/learn why :)
There was a problem hiding this comment.
Hmm okay so the headers didn't really reveal anything to me: @abstract A COSE algorithm indentifier.
but looking at the web docs and looking up what ASCOSEAlgorithmIdentifier reveals that it is a type alias and has this:
typedef NSInteger ASCOSEAlgorithmIdentifier;
But I am not sure if this is any better?
https://developer.apple.com/documentation/authenticationservices/ascosealgorithmidentifier?language=objc
There was a problem hiding this comment.
I'll keep looking at this after the meeting though
There was a problem hiding this comment.
It's not how most enums are defined - and it seems sharpie does not process them when defined this way.
API_AVAILABLE(macos(12.0), ios(15.0)) API_UNAVAILABLE(watchos)
typedef NSInteger ASCOSEAlgorithmIdentifier NS_TYPED_EXTENSIBLE_ENUM;
/*! @abstract The COSE algorithm identifier representing ECDSA with SHA-256.
*/
API_AVAILABLE(macos(12.0), ios(15.0)) API_UNAVAILABLE(watchos)
static ASCOSEAlgorithmIdentifier const ASCOSEAlgorithmIdentifierES256 = -7;
/Applications/Xcode_13.0.0-beta3.app//Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks/AuthenticationServices.framework/Headers/ASCOSEConstants.h
Also check for ASCOSEEllipticCurveIdentifier since it might suffer from the same nine issue
There was a problem hiding this comment.
AH thanks for finding this! I will grep the header files next time!
There was a problem hiding this comment.
Interestingly, ASCOSEEllipticCurveIdentifier is only declared but not used
There was a problem hiding this comment.
some new (b4?) API might eventually use it
but no need to bind it until we have a need for it :)
|
|
||
| public static /* ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport[] */ NSString[] GetAllSupportedPublicKeyCredentialDescriptorTransports () { | ||
| return NSArray.ArrayFromHandle<NSString> (ASAuthorizationAllSupportedPublicKeyCredentialDescriptorTransports ()); | ||
| } |
There was a problem hiding this comment.
you have ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport defined
there's no point in exposing the list (only) as NSString
There was a problem hiding this comment.
Are you suggesting to do something like this?
public static ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport[] GetAllSupportedPublicKeyCredentialDescriptorTransports () {
return NSArray.ArrayFromHandle<ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport> (ASAuthorizationAllSupportedPublicKeyCredentialDescriptorTransports ());
}
There was a problem hiding this comment.
For the API signature ? yes
Not sure the implementation will compile...
There was a problem hiding this comment.
it does not currently compile since ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport is an enum and it complains saying that it needs to be a reference type:
error CS0452: The type 'ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport' must
be a reference type in order to use it as parameter 'T' in the generic type or method
'NSArray.ArrayFromHandle<T>(IntPtr)'
Should I try turning the enum into an interface so that it can be a reference type?
There was a problem hiding this comment.
no, you just need a bit more code
You already get an array of NSString and you can create an array of the enum and convert each string constant into it's enum value
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 88 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 88 tests passed.Failed tests
Pipeline on Agent XAMBOT-1102.BigSur' |
src/authenticationservices.cs
Outdated
| @@ -1002,6 +1003,18 @@ enum ASAuthorizationSecurityKeyPublicKeyCredentialDescriptorTransport { | |||
| Bluetooth, | |||
| } | |||
|
|
|||
| [Mac (12,0), iOS (15,0), MacCatalyst (15,0), NoWatch, NoTV] | |||
| [Native] | |||
| enum ASCOSEAlgorithmIdentifier : long { | |||
There was a problem hiding this comment.
ASCoseAlgorithmIdentifier
src/authenticationservices.cs
Outdated
| [Mac (12,0), iOS (15,0), MacCatalyst (15,0), NoWatch, NoTV] | ||
| [Native] | ||
| enum ASCOSEAlgorithmIdentifier : long { | ||
| Es256 = -7, |
There was a problem hiding this comment.
but ES256 since two-letters acronyms remains uppercase in .net
3+ letters acronyms become Pascal-cased
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 87 tests passed.Failed tests
Pipeline on Agent XAMBOT-1102.BigSur' |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 89 tests passed 🎉Pipeline on Agent XAMBOT-1104.BigSur' |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 89 tests passed 🎉Pipeline on Agent XAMBOT-1100.BigSur' |
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 89 tests passed 🎉Pipeline on Agent XAMBOT-1105.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffAPI & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results3 tests failed, 87 tests passed.Failed tests
Pipeline on Agent XAMBOT-1103.BigSur' |
Closed #12043 in favor of this PR.
I tried to squash and rebase on top of main for this PR so hopefully I didn't miss any commits!