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

Change API and structs to camelCase #4386

Merged
merged 9 commits into from
Jun 23, 2024
Merged

Change API and structs to camelCase #4386

merged 9 commits into from
Jun 23, 2024

Conversation

dani-garcia
Copy link
Owner

@dani-garcia dani-garcia commented Feb 28, 2024

This PR changes all the project's structs and API inputs and outputs from the old PascalCase format to the new camelCase format. At the moment the clients support both but that won't be a thing forever.

I haven't had the time to fully test this, but at least the basic things work. There may be some things missing, and I still need to review what we store in the db to make sure that there aren't any backwards compat issues.

This might also cause problems with other pending PRs so we can wait to merge it until they are dealt with.

Some fields that have special casing:

  • OTP in /accounts/verify-otp, as that's what the clients send. That said we also check otp
  • Keys and Nfc in yubikey, this is for backwards compat with older values stored on the server, now we serialize new versions to keys and nfc.
  • AttestationObject and clientDataJson in webauthn, as that's what the clients send. This is a bigger annoyance as it complicates the use of the webauthn crate.
  • Organization policies, this is for backwards compat with older values stored on the server, now we serialize new versions as camelCase.

Fixes #4656

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

I still see a few non-camelCase key's in outputted JSON. For example in identity.rs
Ill check further but this was what is saw rather quickly.

@BlackDex
Copy link
Collaborator

Also, card vault types have SSN in there, that currently seems to be converted to sSN, which will probably break.

@BlackDex
Copy link
Collaborator

Also, not sure, but isn't it more useful to use fn convert_json_key_lcase_first from the utils mod? It might need some adjustments to also check for ssn maybe, but that should help with nested values stored in the database. And also for items like the Org Policies for example.

@quexten
Copy link
Contributor

quexten commented Jun 16, 2024

On this PR, and web 2024.05 confirming users to orgs does not work (I'm not sure if this is due to the PR, or due to the web vault changing), but since this is the same kind of issue it makes sense to solve here. Specifically, the body the web vault sends contains "key:", while the server expects "Key:" on the post body to /api/organizations/{orgid}/users/{userid}/confirm

@quexten
Copy link
Contributor

quexten commented Jun 16, 2024

Also, for native app compatibility, there seem to be some keys missing for organization profile responses, like

https://github.com/bitwarden/server/blob/b5241f1a9731ea74f22c2f4fe1072421f857429e/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs#L157-L160

causing the native app(s) [only tested for iOS] to crash for accounts in an organization.

@quexten
Copy link
Contributor

quexten commented Jun 16, 2024

Also, somehow all custom fields in the sync response are null on this branch, (making custom fields inaccessible on all clients).

@dani-garcia
Copy link
Owner Author

@quexten Thanks for the review! I fixed the invalid case in key for the confirm and added the missing organization profile fields, which seems to work in both the new android and ios apps. The null fields should be also fixed with d5148bc, we were parsing the JSON incorrectly and ignoring the error.

@BlackDex I didn't even notice convert_json_key_lcase_first being there, but yeah I've switched it over in f0bf5bd, which required adding the check for ssn using _process_key (also removed some clones)

@dani-garcia dani-garcia marked this pull request as ready for review June 18, 2024 23:30
@dani-garcia
Copy link
Owner Author

Native apps are available in beta now, so we should try getting this merged soon:
https://fosstodon.org/@bitwarden/112639925158165590
https://community.bitwarden.com/t/about-the-beta-program/39185

@dani-garcia
Copy link
Owner Author

dani-garcia commented Jun 18, 2024

So far only bug remaining that I know of is that the Android app can't upload attachments or sends.

The android app sends a multipart/mixed header instead of the multipart/form-data header that the ios app and the rest of the clients send. I'll see if that's something we can change in the app.

Fixed in the latest main, uploads should work in the next beta update.

@BlackDex
Copy link
Collaborator

Ill see if i can do some more testing soon.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

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

I just did some testing with my local accounts i use for debugging, and these are my findings so far.

  • The admin UI doesn't work, the user and org overview are broken because the handlebar templates are not updated.
  • For some reason in the Android Emulator i can't login at all, but that is not something we can fix i think.

On my physical phone (Pixel 7) it also doesn't work, i can login, but the sync fails on the client side.

  • Sends have a text object which have the keys Hidden and Text those are still capital-case
  "text": {
      "Hidden": false,
      "Text": "2.j2Z/rN...TbjMvCQ==|/Kk0bfdc...68kv8A==|W1qbVban...AVJYYJRPy/WbK5iQeEQJIWwJh1k="
  }
  • The same for Sends file:
  "file": {
      "FileName": "2.KcN9y...GuollclQ==|XxaO+KyT4fjIIYRb...MNxZ8VeHAAz4=|hruuM...PU4A=",
      "Id": "25587de08f158fe7ecfef24d22fc600c596efaf53bc5d7e23125e75c033a49bb",
      "Size": 10737,
      "SizeName": "10.49 KB"
  },

Other than that, i have not yet found anything which could be wrong any futher

@BlackDex
Copy link
Collaborator

Small update, Those sends were added via the PascalCase version of Vaultwarden.

@dani-garcia
Copy link
Owner Author

Okay, sends should be fixed and changed to lowercase now, and the admin templates are updated too.

I've also updated the org revoke API endpoint which was using Ids as key too.

@BlackDex
Copy link
Collaborator

Login with device seems to result in an error, where the request was unable to be processed.
Not checked how this works for the current vault.bitwarden.com though.

@albatrosify
Copy link

albatrosify commented Jun 20, 2024

I've tried sends: Text is okay. File results in an error for me with the beta app...
I compiled the remove_upcase branch just 10 mins ago...

EDIT: Also when I upload a testfile to the webui and refresh the app, it does not the uploaded files. only text sends
EDIT2: nevermind. it doesnt show text-sends that i have created via webui... neither does it update to the new content when I change a mobile-created text send via the webui and then check on mobile
EDIT3: so as soon as there is a file on the sends list on the server, it results in a sync error for me.

@BlackDex
Copy link
Collaborator

Some values need to be converted from int to String, else the client will fail on those.
Not sure if the MasterPasswordPolicy need to be there too, but it at least is in Upstream.

diff --git a/src/api/identity.rs b/src/api/identity.rs
index 89c82859..523a267a 100644
--- a/src/api/identity.rs
+++ b/src/api/identity.rs
@@ -564,8 +564,11 @@ async fn _json_err_twofactor(providers: &[i32], user_uuid: &str, conn: &mut DbCo
     let mut result = json!({
         "error" : "invalid_grant",
         "error_description" : "Two factor required.",
-        "TwoFactorProviders" : providers,
-        "TwoFactorProviders2" : {} // { "0" : null }
+        "TwoFactorProviders" : providers.iter().map(|i| i.to_string()).collect::<Vec<String>>(),
+        "TwoFactorProviders2" : {}, // { "0" : null }
+        "MasterPasswordPolicy": {
+            "Object": "masterPasswordPolicy"
+        }
     });
 
     for provider in providers {

@dani-garcia
Copy link
Owner Author

dani-garcia commented Jun 20, 2024

I fixed an issue with Sends sending their size as a integer instead of a string, which was breaking the ios app, also fixed the issue mentioned with twofactorproviders being sent as ints too

@albatrosify
Copy link

I can confirm this solves the issue I had with sends.

@dani-garcia dani-garcia merged commit a2bf8de into main Jun 23, 2024
8 checks passed
@dani-garcia dani-garcia deleted the remove_upcase branch June 23, 2024 19:31
@dani-garcia
Copy link
Owner Author

I see there's a lot of interest in this issue so for anyone who's following this, the docker images including this PR are now available at the vaultwarden/server:testing and vaultwarden/server:testing-alpine tags. Please let us know if you find any issues with them. Thanks!

@larena1
Copy link

larena1 commented Jun 23, 2024

POST /api/ciphers/create fails on testing using Android Native Beta. Not sure though if app issue or vaultwarden

@BlackDex
Copy link
Collaborator

POST /api/ciphers/create fails on testing using Android Native Beta

I can reproduce this too. Looks like the client still send Cipher instead of cipher. Not sure if that is intended.

@polymo1
Copy link

polymo1 commented Jun 23, 2024

I can also reproduce, running latest Bitwarden Beta on Android 14 and testing branch pulled just a minute ago on my vaultwarden instance. BW app shows "We were unable to process your request. Please try again or contact us."

@lukasj98
Copy link

On IOS it is working perfect ( Version 2024.6.1 Build 1151, IOS 17.5.1)

@dani-garcia
Copy link
Owner Author

The issue with cipher creation in Android should be fixed now with #4670, builds should be ready in half an hour-ish

@andre1808
Copy link

andre1808 commented Jun 24, 2024

Sorry if this is an odd question, but can I share the data set of the prod version along the testing version?

I’ve tried the testing image earlier (seperate data folder) and it was working fine on my iPhone. So I thought about using it along the prod environment and maybe switch apps back and forth.

Thanks for your work and commitment to this amazing project!

@BlackDex
Copy link
Collaborator

If the database is not SQLite, it kinda can. Push/Websocket notifications will not traverse those different servers, but that is about it

@TheRsKing
Copy link

I can also confirm that it works on iOS

@tonyxu-io
Copy link

I'm unable to download/view attachment on iOS beta app. Not sure if that's related though.

@BlackDex
Copy link
Collaborator

I'm unable to download/view attachment on iOS beta app. Not sure if that's related though.

Works for me on Android. Check your domain settings. If those are not correct it will not work.

@lukasj98
Copy link

I'm unable to download/view attachment on iOS beta app. Not sure if that's related though.

It is working perfect on iOS

@tonyxu-io
Copy link

I'm unable to download/view attachment on iOS beta app. Not sure if that's related though.

It is working perfect on iOS

I just tested again. It seems like image attachments works fine but PDF attachment doesn't work.

@BlackDex
Copy link
Collaborator

I'm unable to download/view attachment on iOS beta app. Not sure if that's related though.

It is working perfect on iOS

I just tested again. It seems like image attachments works fine but PDF attachment doesn't work.

PDF works fine for me too. it might be your phone which has issues with the attachment maybe? For me it just saves the files and i need to open it with a different app afterwards. So it doesn't matter what extension it is using.

@felixoswald
Copy link

Can confirm. PDF and other attachments work fine in the web interface and in the iOS beta app.

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

Successfully merging this pull request may close these issues.

Cannot Login Using Bitwarden Mobile Native