[EC-584] Removed ListResponseModel from OrganizationExportResponseModel #2316

r-tome · 2022-09-29T17:01:57Z

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

There is an issue on exporting an organization vault where the nested data from the API response was not being parsed.
These changes revert the behavior back to what it was to previously before the unintended change by removing the wrap with ListResponseModel.

Code changes

Related client changes on this PR.

src/Api/Controllers/OrganizationExportController.cs: Removed the creation of ListResponseModels
src/Api/Models/Response/OrganizationExportResponseModel.cs: Removed the ListResponseModel wrap from the properties

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

…el properties

Hinton

Looks good, thanks @r-tome

Hinton · 2022-10-07T10:35:24Z

@r-tome We need to add a version check here since the CLI uses this endpoint. The clients sends a Bitwarden-Client-Version header we can use to check, if it's 2022.9.0 we should return the old wrapped response. And add a tech debt task to remove it in 9 months.

…-sensitive-in-regards-to-the-server-response-with-ciphers-and-collections

r-tome · 2022-10-07T14:26:49Z

@Hinton thanks for the heads up, that is well spotted! I've included a basic version check on the controller and created the tech debt ticket.

src/Api/Controllers/OrganizationExportController.cs

BlackDex · 2022-10-12T13:54:59Z

This now probably also needs to be checked for the 2022.10.0 version i guess?

r-tome · 2022-10-12T14:42:32Z

This now probably also needs to be checked for the 2022.10.0 version i guess?

Correct! I've updated the branch with the new version.
Thank you for your question.

Hinton · 2022-10-12T16:18:29Z

src/Api/Controllers/OrganizationExportController.cs

    {
        var userId = _userService.GetProperUserId(User).Value;

        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollections(organizationId);
        (IEnumerable<CipherOrganizationDetails> orgCiphers, Dictionary<Guid, IGrouping<Guid, CollectionCipher>> collectionCiphersGroupDict) = await _cipherService.GetOrganizationCiphers(userId, organizationId);

-        var result = new OrganizationExportResponseModel
+        // Backward compatibility with versions 2022.9.0 and 2022.10.0 that use ListResponseModel
+        if (new[] { "2022.9.0", "2022.10.0" }.Contains(_currentContext.ClientVersion))


Would be nice to have a parser for version and just mark it as _currentContext.ClientVersion.GreaterThan("2022.10.0") or _currentContext.ClientVersion > ClientVersion(2022, 10, 0). But that seems somewhat out of scope

Or not, we need to include web 9.1 and 9.2 as well, at which point this feels like a pain.

I change the ICurrentContext.ClientVersion property to be an instance of the Version class and this ables us to compare versions.
I'm assuming this fix will go out on version 2022.11.0, is that correct?

Ideally, depends when QA can test it.

src/Api/Controllers/OrganizationExportController.cs

…t use this endpoint

Hinton

Looks good, thanks for iterating on this!

Hinton · 2022-11-04T10:30:05Z

src/Api/Controllers/OrganizationExportController.cs

    {
        var userId = _userService.GetProperUserId(User).Value;

        IEnumerable<Collection> orgCollections = await _collectionService.GetOrganizationCollections(organizationId);
        (IEnumerable<CipherOrganizationDetails> orgCiphers, Dictionary<Guid, IGrouping<Guid, CollectionCipher>> collectionCiphersGroupDict) = await _cipherService.GetOrganizationCiphers(userId, organizationId);

-        var result = new OrganizationExportResponseModel
+        // Backward compatibility with versions before 2022.11.0 that use ListResponseModel
+        if (_currentContext.ClientVersion < new Version("2022.11.0"))


We need to bump this right?

Not needed, this will be cherry-picked to rc and will be on version 2022.11.

…el (#2316) * [EC-584] Removed ListResponseModel from OrganizationExportResponseModel properties * [EC-584] Added backwards compatibility for client version 2022.9.0 * [EC-584] Added property 'ClientVersion' to ICurrentContext * [EC-584] Added backwards compatibility for version 2022.10.0 * [EC-584] Change ICurrentContext.ClientVersion from string to Version * [EC-584] Remove check for versions before 2022.9.0 because they do not use this endpoint (cherry picked from commit 8a6f780)

Since v2022.9.x the org export uses a different endpoint. But, since v2022.11.x this endpoint will return a different format. See: bitwarden/clients#3641 and bitwarden/server#2316 To support both version in the case of users having an older client either web-vault or cli this PR checks the version and responds using the correct format. If no version can be determined it will use the new format as a default.

…ponseModel (#2316)" This reverts commit 656eadd.

…el (#2316) * [EC-584] Removed ListResponseModel from OrganizationExportResponseModel properties * [EC-584] Added backwards compatibility for client version 2022.9.0 * [EC-584] Added property 'ClientVersion' to ICurrentContext * [EC-584] Added backwards compatibility for version 2022.10.0 * [EC-584] Change ICurrentContext.ClientVersion from string to Version * [EC-584] Remove check for versions before 2022.9.0 because they do not use this endpoint (cherry picked from commit 8a6f780)

[EC-584] Removed ListResponseModel from OrganizationExportResponseMod…

29bd346

…el properties

r-tome requested a review from Hinton September 29, 2022 17:01

r-tome mentioned this pull request Sep 29, 2022

[EC-584] Fixed OrganizationExportResponse to correctly parse data bitwarden/clients#3641

Merged

Hinton previously approved these changes Sep 29, 2022

View reviewed changes

r-tome added 2 commits October 7, 2022 14:34

Merge branch 'master' into EC-584-the-new-organization-export-is-case…

6405afc

…-sensitive-in-regards-to-the-server-response-with-ciphers-and-collections

[EC-584] Added backwards compatibility for client version 2022.9.0

e0655ce

r-tome dismissed Hinton’s stale review via e0655ce October 7, 2022 14:17

Hinton reviewed Oct 7, 2022

View reviewed changes

src/Api/Controllers/OrganizationExportController.cs Outdated Show resolved Hide resolved

[EC-584] Added property 'ClientVersion' to ICurrentContext

8ef40a7

[EC-584] Added backwards compatibility for version 2022.10.0

45abb2e

Hinton reviewed Oct 12, 2022

View reviewed changes

[EC-584] Change ICurrentContext.ClientVersion from string to Version

ca27a40

r-tome requested a review from Hinton October 13, 2022 16:51

Hinton reviewed Oct 14, 2022

View reviewed changes

src/Api/Controllers/OrganizationExportController.cs Outdated Show resolved Hide resolved

[EC-584] Remove check for versions before 2022.9.0 because they do no…

8523f91

…t use this endpoint

Hinton approved these changes Oct 17, 2022

View reviewed changes

r-tome added needs-qa and removed needs-qa labels Nov 2, 2022

Hinton reviewed Nov 4, 2022

View reviewed changes

r-tome merged commit 8a6f780 into master Nov 7, 2022

r-tome deleted the EC-584-the-new-organization-export-is-case-sensitive-in-regards-to-the-server-response-with-ciphers-and-collections branch November 7, 2022 12:01

BlackDex mentioned this pull request Nov 7, 2022

Support Org Export for v2022.11 clients dani-garcia/vaultwarden#2899

Merged

r-tome added a commit that referenced this pull request Nov 8, 2022

Revert "[EC-584] Removed ListResponseModel from OrganizationExportRes…

4f95b98

…ponseModel (#2316)" This reverts commit 656eadd.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EC-584] Removed ListResponseModel from OrganizationExportResponseModel #2316

[EC-584] Removed ListResponseModel from OrganizationExportResponseModel #2316

r-tome commented Sep 29, 2022 •

edited

Loading

Hinton left a comment

Hinton commented Oct 7, 2022

r-tome commented Oct 7, 2022

BlackDex commented Oct 12, 2022

r-tome commented Oct 12, 2022

Hinton Oct 12, 2022 •

edited

Loading

Hinton Oct 12, 2022 •

edited

Loading

r-tome Oct 13, 2022

Hinton Oct 14, 2022

Hinton left a comment

Hinton Nov 4, 2022

r-tome Nov 4, 2022

[EC-584] Removed ListResponseModel from OrganizationExportResponseModel #2316

[EC-584] Removed ListResponseModel from OrganizationExportResponseModel #2316

Conversation

r-tome commented Sep 29, 2022 • edited Loading

Type of change

Objective

Code changes

Before you submit

Hinton left a comment

Choose a reason for hiding this comment

Hinton commented Oct 7, 2022

r-tome commented Oct 7, 2022

BlackDex commented Oct 12, 2022

r-tome commented Oct 12, 2022

Hinton Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Hinton Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

r-tome Oct 13, 2022

Choose a reason for hiding this comment

Hinton Oct 14, 2022

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

Hinton Nov 4, 2022

Choose a reason for hiding this comment

r-tome Nov 4, 2022

Choose a reason for hiding this comment

r-tome commented Sep 29, 2022 •

edited

Loading

Hinton Oct 12, 2022 •

edited

Loading

Hinton Oct 12, 2022 •

edited

Loading