fission-codes / fission-server

Apache License 2.0
9 stars 0 forks source link

[bug] `account/manage` username does not fail on conflict #236

Closed hugomrdias closed 9 months ago

hugomrdias commented 9 months ago

This should fail:

const account1 = await createAccount()
const account2 = await createAccount()

const changeUsername = await account1.client.accountManageUsername(
  account1.account.did,
  account2.account.username
)
// IT SHOULD fail with conflict because username is already taken by account2

but instead returns 200

{
  did: 'did:key:z6Mkne6NMkBcFeK5Vdy781HiaWvLyaPAS8gsfzTq7HkmVvbb',
  username: 'usernametest-1707414157907.localhost.localhost', // NOTICE .localhost.localhost
  email: 'usernametest-1707414157831@gmail.com'
}
matheus23 commented 9 months ago

Hm. This case wasn't covered in the server tests, so I wrote this one:

#[test_log::test(tokio::test)]
async fn test_patch_username_conflict() -> TestResult {
    let ctx = &TestContext::new().await?;

    let username = "oedipa";
    let username2 = "oedipa2";
    let email = "oedipa@trystero.com";
    let email2 = "oedipa2@trystero.com";
    let issuer = &EdDidKey::generate();
    let issuer2 = &EdDidKey::generate();

    let (_, auth) = create_account::<AccountAndAuth>(username, email, issuer, ctx).await?;
    let (_, _) = create_account::<AccountAndAuth>(username2, email2, issuer2, ctx).await?;

    let (status, _) = patch_username::<ErrorResponse>(username2, &auth, issuer, ctx).await?;

    assert_eq!(status, StatusCode::CONFLICT);

    Ok(())
}

And it seems to work.

I guess it's kinda confusing that the username you use for patching the username needs to be without the domain name, while the username that's returned from the GET account route does contain the domain name.

So what you did there was create one account with username usernametest-1707414157907. When you ask for account info, it tells you its username is usernametest-1707414157907.localhost. Then when you set that as the username for the other account, you get usernametest-1707414157907.localhost.localhost and it works.

In the end you have two accounts, one with the username usernametest-1707414157907, one with the username usernametest-1707414157907.localhost.

This is all kinda weird and a historical artifact. I should probably do some of these things:

hugomrdias commented 9 months ago

ah yes thats it, i mention these inconsistencies in the other issue

matheus23 commented 9 months ago

So yeah - I'll close this then - it does indeed fail on conflict, and I've added a test case in #238 to confirm & make sure that'll be the case in the future.