search

fablabbcn / smartcitizen-api

The Smart Citizen Engine
https://developer.smartcitizen.me
GNU Affero General Public License v3.0
10 stars 4 forks source link

Ensure /users/:id endpoint only lists private devices when the requesting user is authorized to see them #285

Closed timcowlishaw closed 7 months ago

timcowlishaw commented 8 months ago

If the current authenticated user is the user being requested, they are visible If the current authenticated user is an admin, they are visible If there is no current user, or the current user is a different citizen or researcher, they are not.

timcowlishaw commented 7 months ago

Hey @oscgonfer FYI I've added a commit to ensure that user tags and location are properly listed for devices on the user endpoint (this simplifies things too to just reuse the /devices representation as much as possible rather than using a completely different serialisation.

oscgonfer commented 7 months ago

Much nicer, thank you for also reusing the device representation. This approach makes it cleaner and more maintainable. It makes me think of the health check (that we have in the list for the future) and how we can try to reuse devices to make more informative views in the different front-end views (i.e. if one day we add the health to the device, I'd like that to propagate to the different views of the devices adapted obviously to each context, without having to rewrite it all each time).

As far as this one, I'll try to test in staging once I have time and merge if all goes well.

timcowlishaw commented 7 months ago

Yeah, i think that might be something to put on a general 'tech debt' refactoring list for when we have the kits stuff shipped - try and settle on a 'default representation' in JSON for each model type and reuse it across all the endpoints - there's a lot of duplication in those jbuilder files!

In fact, I might create a tech debt issue now with a list.

On Fri, 1 Dec 2023 at 10:23, oscgonfer @.***> wrote:

Much nicer, thank you for also reusing the device representation. This approach makes it cleaner and more maintainable. It makes me think of the health check (that we have in the list for the future) and how we can try to reuse devices to make more informative views in the different front-end views (i.e. if one day we add the health to the device, I'd like that to propagate to the different views of the devices adapted obviously to each context, without having to rewrite it all each time).

As far as this one, I'll try to test in staging once I have time and merge if all goes well.

— Reply to this email directly, view it on GitHub https://github.com/fablabbcn/smartcitizen-api/pull/285#issuecomment-1835839544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBNY4OU67YGKSWWSWE2LYHGV2VAVCNFSM6AAAAAA7Z3YCM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZVHAZTSNJUGQ . You are receiving this because you authored the thread.Message ID: @.***>

oscgonfer commented 7 months ago

That sounds great! Thank you so much

oscgonfer commented 7 months ago

@timcowlishaw

A minor detail... It seems there is also an (always null) owner property added now to the devices on the /user. I don't know if it can be removed, but I guess it's inherited (?) and could be potentially confusing? Otherwise, if you think it's unnecessary or too much of a hassle, I can merge.

timcowlishaw commented 7 months ago

aha good spot, thanks Oscar - that's now fixed!