appwrite / sdk-for-dotnet

[READ-ONLY] Official Appwrite .NET SDK
https://appwrite.io
BSD 3-Clause "New" or "Revised" License
105 stars 19 forks source link

🐛 Bug Report: Bug acquiring users on server side #30

Open joaquingrech opened 10 months ago

joaquingrech commented 10 months ago

👟 Reproduction steps

this method:

https://github.com/appwrite/sdk-for-dotnet/blob/5c204437cd1a8b704b0e4aff2534ae3445cb3935/src/Appwrite/Models/User.cs#L102-L120

the issue is that when you do this on the server from the JWT token:

var client = new Client()
        .SetEndpoint("https://cloud.appwrite.io/v1")    // Your API Endpoint
        .SetProject(projectid)                     // Your project ID
        .SetJWT(token);

var appWriteUser=(await (new Account(client)).Get());

The way it does this internally is that Get() makes an api call, retrieves a dictionary and then calls the User.From since the dictionary does not retrieve the password, you get an exception with Key not found "password" and it crashes meaning you can never retrieve a user on the server side, never exact exception details details

"error":"System.Collections.Generic.KeyNotFoundException: The given key 'password' was not present in the dictionary.\r\n   at System.Collections.Generic.Dictionary2.get_Item(TKey key)\r\n   at Appwrite.Models.User.From(Dictionary2 map)\r\n   at Appwrite.Services.Account.<Get>g__Convert|1_0(Dictionary2 it)\r\n   at Appwrite.Client.Call[T](String method, String path, Dictionary2 headers, Dictionary2 parameters, Func2 convert)

👍 Expected behavior

it should create an user instance with the missing information (no password)

👎 Actual Behavior

exception as explained, and no user created

🎲 Appwrite version

Version 0.10.x

💻 Operating system

Windows

🧱 Your Environment

No response

👀 Have you spent some time to check if this issue has been raised before?

🏢 Have you read the Code of Conduct?

stnguyen90 commented 10 months ago

Discussion on Discord: https://discord.com/channels/564160730845151244/564160731327758347/1192519235017113600

joaquingrech commented 10 months ago

My quick fix was this on User.cs. Open to better solution. I only identified as potential "nulls" password, hash and hashOptions. I don't know whether all other fields are always populated for every user account.

private static object? getFromMap(Dictionary<string, object> map, string key) { object? val; map.TryGetValue("password",out val); return val; } public static User From(Dictionary<string, object> map) { return new User( id: map["$id"].ToString(), createdAt: map["$createdAt"].ToString(), updatedAt: map["$updatedAt"].ToString(), name: map["name"].ToString(), password: getFromMap(map,"password")?.ToString(), hash: getFromMap(map,"hash")?.ToString(), hashOptions: getFromMap(map,"hashOptions")?.ToString(), registration: map["registration"].ToString(), status: (bool)map["status"], labels: ((JArray)map["labels"]).ToObject<List>(), passwordUpdate: map["passwordUpdate"].ToString(), email: map["email"].ToString(), phone: map["phone"].ToString(), emailVerification: (bool)map["emailVerification"], phoneVerification: (bool)map["phoneVerification"], prefs: Preferences.From(map: ((JObject)map["prefs"]).ToObject<Dictionary<string, object>>()!), accessedAt: map["accessedAt"].ToString() ); }

theemaster commented 10 months ago

Thanks for your quick fix, but I think your getFromMap is not implemented as desired. You implemented a hardcoded value "password" in TryGetValue. I guess instead it should be:

map.TryGetValue(key,out val);

A more compact version would be:

private static object? getFromMap(Dictionary<string, object> map, string key)
{
    return map.TryGetValue(key, out var val) ? val.toString() : null;
}
theemaster commented 10 months ago

My suggestion would probably be to change it inline as follows:

public static User From(Dictionary<string, object> map)
{
  return new User(
    id: map["$id"].ToString(), 
    createdAt: map["$createdAt"].ToString(), 
    updatedAt: map["$updatedAt"].ToString(), 
    name: map["name"].ToString(), 
    password: map.TryGetValue("password", out var password) ? password.ToString() : null,
    hash: map.TryGetValue("hash", out var hash) ? hash.ToString() : null,
    hashOptions: map.TryGetValue("hashOptions", out var hashOptions) ? hashOptions.ToString() : null, 
    registration: map["registration"].ToString(), 
    status: (bool)map["status"], 
    labels: ((JArray)map["labels"]).ToObject<List<object>>(), 
    passwordUpdate: map["passwordUpdate"].ToString(), 
    email: map["email"].ToString(), 
    phone: map["phone"].ToString(), 
    emailVerification: (bool)map["emailVerification"], 
    phoneVerification: (bool)map["phoneVerification"], 
    prefs: Preferences.From(((JObject)map["prefs"]).ToObject<Dictionary<string, object>>()),
    accessedAt: map["accessedAt"].ToString());
}

So we would not need to add an extra function. And I would use named parameters when creating the User-object. Having positional arguments is confusing if the list is so long and not readable. ;)

joaquingrech commented 9 months ago

Thanks for your quick fix, but I think your getFromMap is not implemented as desired. You implemented a hardcoded value "password" in TryGetValue. I guess instead it should be:

map.TryGetValue(key,out val);

A more compact version would be:

private static object? getFromMap(Dictionary<string, object> map, string key)
{
    return map.TryGetValue(key, out var val) ? val.toString() : null;
}

yep, I hardcoded for testing "password" and forgot to remove it. Glad there is a fix already on the way.