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

Add seperate Respone Schema for 2FA #352

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0xkubectl
Copy link

@0xkubectl 0xkubectl commented Jul 21, 2024

Fixes #241 - Based on the work of @C0D3-M4513R from this PR

Adds a new Response schema that is able to encapsulate the 2FA challange/request. This is done via the oneOf operator on the CurrentUserLoginResponse.
Here are the docs for oneOf: https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/

I have tested this with the Rust Generator and have found no issues. I cannot say how other languages' generator would fare with this fix. If someone finds the time to test this on other languages, please go ahead :3

Tested languages

  • python @ 7.7.0
  • rust @ 7.7.0
  • typescript @ 7.7.0
  • csharp @ ?
  • java @ 7.7.0
  • dart @ ?

@C0D3-M4513R
Copy link
Contributor

from #241:

You can specify that a code returns either, e.g. <User | ReqMFAAuth>, but almost no generator, and not all languages, support that. So due to this API design it is something we will have to fix in each SDK instead.

@0xkubectl
Copy link
Author

Seen that, but i figured that the latest Rust generator is able to do this, it might be worth a try to bump the other languages aswell and see if that functionality exists.

@C0D3-M4513R
Copy link
Contributor

Rust is a really, really powerful language when it comes to serialization and type constructs.
A lot of other languages can't represent many things as neatly as rust can.

@0xkubectl
Copy link
Author

Ok, ill just try the python one and see if it works :3

@C0D3-M4513R
Copy link
Contributor

C0D3-M4513R commented Jul 21, 2024

Also it would be interesting, if rust supports this without my manual patch or not.
And of course other languages.

@0xkubectl
Copy link
Author

I checked TS, Python and Rust, all of them can deal with the oneOf operator.
Caveat: I bumped everyones openapi-generator version to 7.7.0

This indicates to me that there is at least baseline support for this.

@0xkubectl
Copy link
Author

Not sure what is up with the docs, but java for example generates this code, which looks pretty functional to me:

@Override
public GetCurrentUser200Response read(JsonReader in) throws IOException {
    Object deserialized = null;
    JsonElement jsonElement = elementAdapter.read(in);

    int match = 0;
    ArrayList<String> errorMessages = new ArrayList<>();
    TypeAdapter actualAdapter = elementAdapter;

    // deserialize CurrentUser
    try {
        // validate the JSON object to see if any exception is thrown
        CurrentUser.validateJsonElement(jsonElement);
        actualAdapter = adapterCurrentUser;
        match++;
        log.log(Level.FINER, "Input data matches schema 'CurrentUser'");
    } catch (Exception e) {
        // deserialization failed, continue
        errorMessages.add(String.format("Deserialization for CurrentUser failed with `%s`.", e.getMessage()));
        log.log(Level.FINER, "Input data does not match schema 'CurrentUser'", e);
    }
    // deserialize RequiresTwoFactorAuth
    try {
        // validate the JSON object to see if any exception is thrown
        RequiresTwoFactorAuth.validateJsonElement(jsonElement);
        actualAdapter = adapterRequiresTwoFactorAuth;
        match++;
        log.log(Level.FINER, "Input data matches schema 'RequiresTwoFactorAuth'");
    } catch (Exception e) {
        // deserialization failed, continue
        errorMessages.add(String.format("Deserialization for RequiresTwoFactorAuth failed with `%s`.", e.getMessage()));
        log.log(Level.FINER, "Input data does not match schema 'RequiresTwoFactorAuth'", e);
    }

    if (match == 1) {
        GetCurrentUser200Response ret = new GetCurrentUser200Response();
        ret.setActualInstance(actualAdapter.fromJsonTree(jsonElement));
        return ret;
    }

    throw new IOException(String.format("Failed deserialization for GetCurrentUser200Response: %d classes match result, expected 1. Detailed failure message for oneOf schemas: %s. JSON: %s", match, errorMessages, jsonElement.toString()));
    }
}.nullSafe();

Also the javascript library is generated with the typescript generator which seems also seems to say it does not support it, but generates these types just fine:

export type GetCurrentUser200Response = CurrentUser | RequiresTwoFactorAuth;

To be fair idk what to make of this.

@ariesclark
Copy link
Member

We rely on the following generators:

  • typescript-axios
  • rust
  • csharp-netcore
  • python-legacy
  • dart-dio
  • java

Out of these, according to documentation, Only rust & dart-dio support oneOf.
I see you've done testing above, does the actual support differ from the documented support?

@0xkubectl
Copy link
Author

I found i quite strange that generators that list not having that feature, still create files that resemble to work.
Looking into this i found the oneof template files for:

My best guess is that oneOf can be used not just in schemas/models but also somewhere else and that you only claim full support when all those other usages are also working. Still emphazising that this is on 7.7.0 a major version above what is currently being used by everything but the rust language. I would propose we bump those versions and see if stuff breaks, and then come back to this PR.

@Rexios80
Copy link
Collaborator

oneOf definitely caused issues with the dart generator in the past although maybe we weren't using dart-dio then? I can try generating this next week.

@Rexios80
Copy link
Collaborator

This is what got generated:

GetCurrentUser200Response({
  required this.acceptedTOSVersion,
  this.acceptedPrivacyVersion,
  this.accountDeletionDate,
  this.accountDeletionLog,
  this.activeFriends,
  required this.allowAvatarCopying,
  this.badges,
  required this.bio,
  required this.bioLinks,
  required this.currentAvatar,
  required this.currentAvatarAssetUrl,
  required this.currentAvatarImageUrl,
  required this.currentAvatarThumbnailImageUrl,
  required this.currentAvatarTags,
  required this.dateJoined,
  required this.developerType,
  required this.displayName,
  required this.emailVerified,
  this.fallbackAvatar,
  required this.friendGroupNames,
  required this.friendKey,
  required this.friends,
  required this.hasBirthday,
  this.hideContentFilterSettings,
  this.userLanguage,
  this.userLanguageCode,
  required this.hasEmail,
  required this.hasLoggedInFromClient,
  required this.hasPendingEmail,
  required this.homeLocation,
  required this.id,
  this.isBoopingEnabled = true,
  this.isFriend = false,
  this.lastActivity,
  required this.lastLogin,
  required this.lastMobile,
  required this.lastPlatform,
  required this.obfuscatedEmail,
  required this.obfuscatedPendingEmail,
  required this.oculusId,
  this.googleId,
  this.googleDetails,
  this.picoId,
  this.viveId,
  this.offlineFriends,
  this.onlineFriends,
  required this.pastDisplayNames,
  this.presence,
  required this.profilePicOverride,
  required this.profilePicOverrideThumbnail,
  required this.pronouns,
  required this.state,
  required this.status,
  required this.statusDescription,
  required this.statusFirstTime,
  required this.statusHistory,
  required this.steamDetails,
  required this.steamId,
  required this.tags,
  required this.twoFactorAuthEnabled,
  this.twoFactorAuthEnabledDate,
  required this.unsubscribe,
  this.updatedAt,
  required this.userIcon,
  this.username,
  required this.requiresTwoFactorAuth,
});

It looks like it just combined the fields of both possible responses which definitely will not work

@ariesclark
Copy link
Member

dart-dio does claim support for oneOf though, are you sure you're using the latest version of the generator?

@Rexios80
Copy link
Collaborator

Yes my generate script updates the generator and I double checked it's using 7.7.0

@0xkubectl
Copy link
Author

0xkubectl commented Jul 22, 2024

After long debugging I found the issue - the serilization library is not compatible with oneOf. Commenting out serializationLibrary: json_serializable generated:

//
// AUTO-GENERATED FILE, DO NOT MODIFY!
//

// ignore_for_file: unused_element
import 'package:built_collection/built_collection.dart';
import 'package:vrchat_dart_generated/src/model/current_user.dart';
import 'package:vrchat_dart_generated/src/model/requires_two_factor_auth.dart';
import 'package:built_value/built_value.dart';
import 'package:built_value/serializer.dart';
import 'package:one_of/one_of.dart';

part 'get_current_user200_response.g.dart';

/// GetCurrentUser200Response
///
/// Properties:
/// * [id] - A users unique ID, usually in the form of `usr_c1644b5b-3ca4-45b4-97c6-a2a0de70d469`. Legacy players can have old IDs in the form of `8JoV9XEdpo`. The ID can never be changed.
/// * [requiresTwoFactorAuth] 
@BuiltValue()
abstract class GetCurrentUser200Response implements Built<GetCurrentUser200Response, GetCurrentUser200ResponseBuilder> {
  /// One Of [CurrentUser], [RequiresTwoFactorAuth]
  OneOf get oneOf;

  GetCurrentUser200Response._();

  factory GetCurrentUser200Response([void updates(GetCurrentUser200ResponseBuilder b)]) = _$GetCurrentUser200Response;

  @BuiltValueHook(initializeBuilder: true)
  static void _defaults(GetCurrentUser200ResponseBuilder b) => b;

  @BuiltValueSerializer(custom: true)
  static Serializer<GetCurrentUser200Response> get serializer => _$GetCurrentUser200ResponseSerializer();
}

class _$GetCurrentUser200ResponseSerializer implements PrimitiveSerializer<GetCurrentUser200Response> {
  @override
  final Iterable<Type> types = const [GetCurrentUser200Response, _$GetCurrentUser200Response];

  @override
  final String wireName = r'GetCurrentUser200Response';

  Iterable<Object?> _serializeProperties(
    Serializers serializers,
    GetCurrentUser200Response object, {
    FullType specifiedType = FullType.unspecified,
  }) sync* {
  }

  @override
  Object serialize(
    Serializers serializers,
    GetCurrentUser200Response object, {
    FullType specifiedType = FullType.unspecified,
  }) {
    final oneOf = object.oneOf;
    return serializers.serialize(oneOf.value, specifiedType: FullType(oneOf.valueType))!;
  }

  @override
  GetCurrentUser200Response deserialize(
    Serializers serializers,
    Object serialized, {
    FullType specifiedType = FullType.unspecified,
  }) {
    final result = GetCurrentUser200ResponseBuilder();
    Object? oneOfDataSrc;
    final targetType = const FullType(OneOf, [FullType(CurrentUser), FullType(RequiresTwoFactorAuth), ]);
    oneOfDataSrc = serialized;
    result.oneOf = serializers.deserialize(oneOfDataSrc, specifiedType: targetType) as OneOf;
    return result.build();
  }
}

I dont think its a reasonable ask to require a library change, but @Rexios80 let me know what you think. I have no idea how Dart works in detail, so this might not be as an impactful change as I believe.

Regardless, as there is clearly inconsistent support for oneOf i would rather patch the openapi.yaml in the individual repos that want this.
Not sure if it makes sense to keep the PR around, feel free to close.

@Rexios80
Copy link
Collaborator

@0xkubectl The built_value generated code is borderline unreadable and also many times the size. I would rather not go back to it.

@0xkubectl 0xkubectl force-pushed the kubectl/fix-2fa-inconsistencies branch from 3553a0b to cc4a59f Compare July 23, 2024 13:09
0xkubectl added a commit to 0xkubectl/specification that referenced this pull request Jul 27, 2024
@Rexios80
Copy link
Collaborator

Rexios80 commented Oct 3, 2024

I am almost positive the dart generator does not like this

@0xkubectl
Copy link
Author

0xkubectl commented Oct 3, 2024

Lets keep this open until we know how to deal with different capabilities of generators.
This exists, and would solve it but it introduces more specs.
I.e. a spec for all features (anyOf, oneOf, etc) and their combinations, not sure if that is the way to go. But I am in favor of having a spec that is built with all the features OAPI3 offers for those generators that can deal with it as it means less hacks in consuming libraries etc.

Edit:
Im refering to this issue #370

Copy link
Member

@ariesclark ariesclark left a comment

Choose a reason for hiding this comment

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

Blocked by x-feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing "requiresTwoFactorAuth" object
5 participants