[api] Implement multitenancy in API #61

[purhan]

Closes #61

[nemesifier]

I suggest you to write some tests first, you can take a look in other modules (eg: firmware-upgrader, openwisp-users).

[coveralls]

Coverage increased (+0.09%) to 99.432% when pulling f88f335 on Purhan:issues/61-implement-multitenant-api into b130a44 on openwisp:master.

[atb00ker]

Okay, I started to check manually to see the reason why you needed the mixin but it doesn't seem to work.
Try this out:

1. Create a subnet with org1
2. Create an org1 user who is not a superuser, only staff operator user with all subnet permissions.
3. Login from this user
4. Go to subnets, your device would be missing!

Have you tested this manually as well? Do you need help with testing manually? 😄

[purhan]

Okay, I started to check manually to see the reason why you needed the mixin but it doesn't seem to work.
Try this out:

1. Create a subnet with org1

2. Create an org1 user who is not a superuser, only staff operator user with all subnet permissions.

3. Login from this user

4. Go to subnets, your device would be missing!

Have you tested this manually as well? Do you need help with testing manually? smile

@atb00ker I did some testing manually but didn't notice this. This is the bug on the master branch. I found a similar bug here: #77

[purhan]

Also I'd like to mention, this is only a problem in the admin interface. The api still shows all subnets in http://127.0.0.1:8000/api/v1/subnet/
@atb00ker can you confirm this?

[atb00ker]

@atb00ker I did some testing manually but didn't notice this. This is the bug on the master branch. I found a similar bug here: #77

That's odd, I'll have to dedicate more time to this in that case, please investigate further if possible, thanks! 😄

Also I'd like to mention, this is only a problem in the admin interface. The api still shows all subnets in http://127.0.0.1:8000/api/v1/subnet/

You are talking about testing this in master, right?

[purhan]

@atb00ker I did some testing manually but didn't notice this. This is the bug on the master branch. I found a similar bug here: #77

That's odd, I'll have to dedicate more time to this in that case, please investigate further if possible, thanks! smile

Sure 😄

Also I'd like to mention, this is only a problem in the admin interface. The api still shows all subnets in http://127.0.0.1:8000/api/v1/subnet/

You are talking about testing this in master, right?

@atb00ker Yes, the bugs that I mentioned are on master.

[nemesifier]

Pay attention, if I'm not mistaken, most of the API action should only be available to managers (not members).

The difference is that a member could be an user who signs up for public wifi or another derivative service offered via openwisp to end users, while a manager is a user who manages their network via the openwisp administration interface but is not a super user.

See also my comments below.

[atb00ker]

Sorry, I am not able to test it because of the bug mentioned above, I'll need to find time to checkout that issue, it's 4th on my list presently, so I would take a while for me to come here! 😄
Meanwhile, rebase with master is required! 😄

[purhan]

@atb00ker by all means, take your time. BTW, please use tools like postman other than the admin interface, to test the API, as the admin interface can behave differently! (The bugs mentioned earlier, I found these in the admin interface only and not on the API itself)

[atb00ker]

please use tools like postman

At least for openwisp, you shouldn't need postman, if you open API links on browser itself, the DRF page should work just fine, if it doesn't it's a bug (because we are not following the DRF docs in that case)! 😄

[purhan]

@nemesisdesign I have made many changes in the last 4 commits.

go to http://localhost:8001/api/v1/subnet/, results in the API are correct, but if you scroll below to the browsable API form and click on "organization" and "master subnet", you will see also items from other organizations, while the user should only see items of the organizations that belong to them, I had not considered this!

This was fixed. ☝️ (c000861)

wait, why are you doing these changes here?
I am not sure this is the right place to do this check, why is this needed at all, can you explain?

Now that I look at it, this is not a good way to implement what I tried to do. Basically, a user could still access subnets from other organizations by importing/exporting CSV files (which have an organization field in them) i made these changes to prevent that from happening. I will try to make a reusable class for this too.

This was also fixed ☝️ (8632f20)

[nemesifier]

Great work @purhan! Did a quick test and I'm delighted to see that the DRF browsable API issue is solved. We'll be reusing that solution also in other modules! Added a note to openwisp/openwisp-users#210.

I left some comments below, I will need more time to test it deeper, but it's almost ready! 👍 👍

[nemesifier]

Looks good @purhan! Thank you so much! 👍 👍

Before merging, can you move the filter mixins to openwisp-users as initially discussed please?
openwisp/openwisp-users#210

[nemesifier]

There's a conflict to resolve.

Can you make the openwisp-user dependency point to your branch here?
openwisp/openwisp-users#217 (here's how we do it for openwisp-radius: https://github.com/openwisp/openwisp-radius/blob/ee5220f9aebc81508582fb1d6fa5f5338dd51c90/requirements.txt#L7-L9)

And change the code so use the openwisp-users mixins please?

[purhan]

@nemesisdesign Done, resolved the conflicts and replaced the code to use mixins from openwisp/openwisp-users#217 . Please take a look.

[nemesifier]

Almost ready to merge @purhan, I took a closer look at the tests and noticed something, read my comment below.

[purhan]

Ready for review 👍

[nemesifier]

Great work @purhan! 👍 👍