search

TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
975 stars 306 forks source link

Fix for `WithSoftDelete` failing some specific operations #6573

Closed nicholaspcr closed 1 year ago

nicholaspcr commented 1 year ago

Summary

While looking into https://github.com/TheThingsIndustries/lorawan-stack-docs/issues/1165#issuecomment-1726416533 I noticed that when attempting to fetch deleted entities we are firstly trying to fetch accounts which trigger the _isSoftDelete conditional in the applyContext executed in most queries in the store pkg. Since DeletedOptions isdetermined in the registry level and that propagates to the membership section of the store pkg, it means that we are trying to find an deleted account by passing a valid user.

Steps to Reproduce

List an entity:

  1. Create an app
  2. Delete an app
  3. Run ttn-lw-cli app list --deleted
  4. You will get the account_not_found error.

Search for an entity:

  1. Login into a non admin user
  2. Create an app
  3. Delete an app
  4. Run ttn-lw-cli app search --deleted
  5. You will get the account_not_found error.

Current Result

We always attempt to search for an account with deleted_at IS NOT NULL but that is not the desired behavior

Expected Result

Get a valid user and then search for deleted entities/memberships.

Relevant Logs

No response

URL

No response

Deployment

The Things Stack Enterprise (self-hosted)

The Things Stack Version

3.27.2-dev

Client Name and Version

Development version

Other Information

No response

Proposed Fix

  1. Update methods in the membership_store.go to only search for non deleted users, overwriting the DeletedOptions on only the calls for getAccountModel.
  2. Add tests that validate this.

Contributing

Code of Conduct

KrishnaIyer commented 1 year ago

I don't understand the requirement. Why should non admins be able to list deleted entities? Admins need this to purge IDs.

nicholaspcr commented 1 year ago

Maybe from me trying to fit in both list and search under the same behavior, it causes the confusion.

  1. list fails even for admin users, the reason for that is trying to find the user as a deleted account due to the injection on DeletedOptions in the context.
  2. search only works for admins because fetching the memberships are skipped by setting member to nil in the search related methods (ref)
  3. Even if a user should be forbidden to see its own deleted entities due to the lack of admin privileges, the error should reflect that. Instead of informing that the account was not found. 
    error:pkg/identityserver/store:account_not_found (user account with id `admin` not found)
account_type=user
account_id=admin
correlation_id=8ca168ed8379438cbe6066d5cbcbe6dc
exit status 255

In addition of that I would argue that an user being able to see a list of its own deleted entities isn't problematic, one could even check before asking an admin to restore the entity. What you think @KrishnaIyer ?

KrishnaIyer commented 1 year ago

Before getting into the details of implementation, let's first discuss what we're trying to achieve.

In addition of that I would argue that an user being able to see a list of its own deleted entities isn't problematic, one could even check before asking an admin to restore the entity

Is this a customer requirement? What is the basis for this? I think that the current flow of only allowing admins to list purged entities works well. Users will get an ID taken or similar warning and then they can request an admin to purge.

nicholaspcr commented 1 year ago

A bit late on the reply here, the example that I gave is not a customer requirement. It was merely an example of something possible in a smaller scale where an user can contact an admin from outside of the stack, providing a bit more specific information. Please disregard as it is not intended as a suggestion for this issue.

The main point here is the following the API does already allow non admin users to list/search for deleted entities (with the exception of other users), while is not present on the Console the API does allow it. In addition to this statement the ability of performing both operations via the CLI returns an inappropriate error message instead of the expected result.

This is not a big change but merely a bug fix which involves in removing the DeletedOptions from the fetch account operation done on membership methods. In a bit I'll link a draft PR with the changes so its easier to have this discussion