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

Request different name for 'Regular User' group #79

Closed
lbross opened this issue Oct 23, 2017 · 19 comments
Closed

Request different name for 'Regular User' group #79

lbross opened this issue Oct 23, 2017 · 19 comments

Comments

@lbross
Copy link

lbross commented Oct 23, 2017

I'm putting the groups into a VB enumeration and that structure doesn't deal well with spaces. Can this group name be changed to either Regular_User or RegularUser? Thanks

@jkeifer
Copy link
Member

jkeifer commented Oct 23, 2017

I'm not aware of a Regular User group... If a user is not NWCC_STAFF or NWCC_ADMIN then the group list would be empty. Here's an example response of the account/user endpoint for a user that has a role of staff but is not in any group:

{
    "username": "jkeifer_staff",
    "email": "[email protected]",
    "first_name": "",
    "last_name": "",
    "roles": [
        "ROLE_STAFF"
    ],
    "groups": []
}

@lbross
Copy link
Author

lbross commented Oct 23, 2017

Interesting. This is what I got when I just hit https://test.ebagis.
geog.pdx.edu/api/rest/account/user/ with user: testUser. Is this user not set-up correctly? Note the role in groups rather than roles.
{"username":"testUser",
"email":"[email protected]",
"first_name":"Test",
"last_name":"User",
"roles":[],
"groups":["NWCC_ADMIN"]}

Also, this is the list of roles provided by @jdduh. Not correct?
The roles are:

Guest: view the entry website
Regular User: download AOI
NWCC_STAFF: download and upload AOI
NWCC_ADMIN: download, upload, and delete AOI
Superuser: everything

@jkeifer
Copy link
Member

jkeifer commented Oct 23, 2017

Sorry, I probably should not have send the output of the previous user as that may have muddled things up.

As I mentioned in my email about creating users, the roles and groups have very different uses. Unfortunately, I mistakenly used roles to distinguish between different sets or permissions because it was easy. But roles has particular significance in django, as the roles control access to the admin console. When we weren't using the admin console it was fine to use the roles for different purposes, but now that we are using the admin console I realized that we could not use the roles to signify user permissions sets, as then we'd inadvertently be granting admin console access to any NWCC users. That is obviously not desired.

As such I had to add in groups. The two named groups at this point that you will see are NWCC_STAFF and NWCC_ADMIN. Regular users are those without any group affiliations.

The testUser account you show above is an example of an NWCC_ADMIN user. The jkeifer_staff user from my example is a user that does not have NWCC_STAFF or NWCC_ADMIN privileges, but does have access to the django admin console via the staff STAFF role. Honestly, that combination is a weird one and unlikely to be seen ever in actual use.

So, what should you do? I'd simply recommend looking at the groups for NWCC_STAFF or NWCC_ADMIN. If a user is NWCC_STAFF, they can use the standard functions in the PC BAGIS, downloading and uploading AOIs. If they have the NWCC_ADMIN role then they can also delete AOIs.

A super user will have the ROLE_ADMIN under roles, but I'd probably not worry about that for your purposes and expect they will also have NWCC_ADMIN in their groups.

Make sense? Can I clarify anything further?

@jdduh
Copy link

jdduh commented Oct 23, 2017

If a group name is required for regular users, then we can call it "DEFAULT." Use "ADMINISTRATORS" for super users.

@lbross
Copy link
Author

lbross commented Oct 23, 2017

@jkeifer: Your explanation makes sense and I concur that PC-BAGIS need not be concerned with the django roles.

@jdduh: Do you approve of the streamlined PC-BAGIS security model that Jarrett suggested? With this model the user will have to be logged in with a valid group (NWCC_STAFF or NWCC_ADMIN) to perform any actions in PC-BAGIS. Given your original request, we could add two additional groups if you think it's relevant for ArcMap users. This would require additional work on Jarrett's part to implement the security on the server.

  1. A GUEST group that can list the AOIs in the database but do nothing else. Not sure how useful this would be. @jkeifer: Do you have the concept of a guest group on the webapp or do all users have to have an account? Worded slightly differently, can users access any of the endpoints without a token?
  2. A DEFAULT group that can only list/download AOIs (read-only)

@jkeifer
Copy link
Member

jkeifer commented Oct 23, 2017

@lbross

  1. A guest, or unauthenticated user, can list AOIs/files and their attributes but cannot download them. I would not be able to create a GUEST group as an unauthenticated user does not have a user account to which the GUEST group could be added.

  2. All authenticated users could be added to a DEFAULT group upon account creation, if we do want to allow download use only in PC BAGIS (I also question the utility of allowing this). However, in my mind, if we're doing this it seems reasonable to give all authenticated users download privileges in PC BAGIS as that is what they get in eBAGIS.

    Therefore, the DEFAULT group would be represented simply by the absence of any overriding group membership, i.e., all users can download so they don't get more privileges without specific group memberships. The addition of upload operations in the PC BAGIS UI would be restricted to only users with NWCC_STAFF or NWCC_ADMIN group membership. Likewise, delete operations would only be available to NWCC_ADMIN group members.

@lbross
Copy link
Author

lbross commented Oct 23, 2017

@jkeifer

This all works for me. Did not realize you could successfully hit aois endpoint without a token. This approach requires no action on your part and I hadn't implemented group-level security on PC-BAGIS so no re-work for me. Thanks!

@lbross
Copy link
Author

lbross commented Oct 23, 2017

We talked about allowing all users to be able to delete their own uploads. Is this baked into the system?

@lbross
Copy link
Author

lbross commented Oct 24, 2017

@jkeifer

Just trying to understand how all this works. I just deleted an aoi logged in as lbross that lbross did not upload. lbross is also not in the either of the NWCC_ groups. It's my understanding that lbross shouldn't have been able to delete that AOI. Or, is the security enforced as part of the web ui so I need to make sure that I am also enforcing it in PC-BAGIS.

I was trying to trigger a permissions error from the endpoint so that I could see what that looks like and write code to catch it in case something sneaks through.

@jkeifer
Copy link
Member

jkeifer commented Nov 4, 2017

Sorry not to respond more quickly to this. I have a huge project I have to complete for delivery Thursday, which has included 4 late-night maintenance windows to rebuild our entire network...so I've been busy.

You are correct, if lbross was not an NWCC_ADMIN I don't think that user should have been able to delete an AOI. That should be enforced on the backend, not in the web UI or in PC-BAGIS (that being said, you still may want to change your UI depending on the user permissions, and not display a delete button if they can't use it).

I'd like to say I'll dig into it right away, but I'll probably have to wait another week before I have time to work on this. But if I find some time I'll see what I can do.

@jkeifer
Copy link
Member

jkeifer commented Nov 14, 2017

@lbross A progress update: Digging into your report, I realized I did a bad job the last time I worked on permissions. Obviously, as an operation that should never have been allowed--your user deleting an AOI--succeeded without issue.

It has been slow going, but I've been working my way though the API and thoroughly checking things over. I don't know if I mistakenly thought I finished permissions before but didn't or what exactly happened, but permissions have been really wrong. Thankfully, I can say the last work I did set me up to do permissions correctly, I just didn't apply them correctly. So it has been fairly straightforward to correct the problems, I just have to work out the right blend of checks needed to establish user authorization for each particular scenario.

Assuming I have time to work on this as I anticipate I will, I hope to have this work completed and pushed to the beta test site tomorrow evening.

jkeifer added a commit that referenced this issue Nov 14, 2017
@jkeifer
Copy link
Member

jkeifer commented Nov 14, 2017

@lbross I just deployed some changes to the test beta instance. I have tested things like uploads and downloads, but I haven't gotten to deletes yet. Even so, I think the changes I've made will address the issue you found. I am working on another set of changes regarding deletes (i.e., don't actually delete records but archive them instead) that I want to implement before I go spending time testing deletes. However, feel free to give it a go if you need to test your end.

If you find any further issues please let me know. I'm hopeful everything is setup correctly now, but I wouldn't be surprised if I've missed a couple things.

@lbross
Copy link
Author

lbross commented Nov 14, 2017

@jkeifer Thanks for the update. I'm currently working on BAGIS-P but hope to get back to this soon. For clarification, the url for the "test beta instance" is test.ebagis.geog.pdx.edu?

It sounds like you are not 100% that the security for deletes is in place? If so, I will wait for you to finish work on the soft-delete before I try to test it. Unless you want me to test uploads/downloads?

@jkeifer
Copy link
Member

jkeifer commented Nov 14, 2017

@lbross Yes, the beta is test.ebagis.geog.pdx.edu.

I think the delete security fixes should be done, but I have not tested the deletes on my end, so by that token I'm not 100%. I figured I should wait to test the deletes until I'm done with the soft delete so I only have to test once instead of twice. You can test whatever you'd like though, as the soft deletes shouldn't really change what you see.

Regardless, I hope to have the soft delete stuff completed before the end of the week, assuming I don't run into any issues. So if you're busy with BAGIS-P, I might be done before you can shift focus anyway.

@jkeifer
Copy link
Member

jkeifer commented Nov 21, 2017

@lbross I am beyond ready to be done with the soft delete work. It turned into a huge hassle and I had to rework vastly more code than I was anticipating. I've deployed it to beta test.ebagis.geog.pdx.edu instance and I have not had any issues in testing. However, I have done very little testing on the beta instance. Even so, I needed to get it pushed out so I can shift my focus to other tasks.

As always, please don't hesitate to let me know if you have any issues. I think I tested thoroughly, but as always I feel like I haven't done enough testing for this magnitude of change.

@lbross
Copy link
Author

lbross commented Nov 21, 2017

@jkeifer You'll be pleased to know that for the most part, the permissions are working as expected. The one exception is that in my testing, a user cannot delete their own uploads if they aren't in the NWCC_ADMIN group. I think we requested this earlier and it would be beneficial for workflow for all users to be able to pull their own uploads back so they don't have to track down an admin.

@lbross
Copy link
Author

lbross commented Nov 21, 2017

@jkeifer Spoke too soon :-( Just inadvertently triggered an edge case. If you delete an AOI, you can't upload one with the same name again or it will trigger the attached error.
uploaderror

@jkeifer
Copy link
Member

jkeifer commented Nov 22, 2017

@lborss That's an expected error--deleting an AOI is a two-stage process:

  1. removal, which sets a removed_at date and makes it look non-existent for most queries
  2. "deactivation", which is when the actual files are deleted, and the record is no longer active (it is retained for archival purposes)

The deactivation is expected to be an automated process occurring after removal plus some time buffer. Doing it this way means we have a window in which we can "un-delete" something by simply unsetting the removed_at field.

In other words, until the "removed" record is "deactivated", its name is still taken. Where a unique name is enforced (e.g., AOIs), then adding a new one of the same name fails until a removed record is deactivated. #51 is a related ticket, which once addressed should have uploads error out on upload, rather than during processing. I'd also be open to an endpoint that can be used for AOI name validation. If you'd find that useful let me know or open a ticket for that and I will get it on the list.

As to the question of users deleting their own uploads, I think that warrants a separate discussion, so I've created another ticket for that: #85.

@lbross
Copy link
Author

lbross commented Nov 22, 2017

@jkeifer Thanks for the explanation of the logic behind deleting an AOI. I currently check the aois/ endpoint to see if the AOI exists. Obviously the "deactivated" record is not included in these results.

I don't want to submit an AOI that will return a stacktrace. If the completion of #51 means I will get an error I can handle, similar to a permissions error, that is one way to go. An endpoint that can be used for AOI name validation is obviously more efficient since I won't try to send the duplicate but it's not a high priority. I will wait until you release a solution for #51 and see if that works.

Closing because I think we have resolved the original issue. Related issues have their own tickets.

@lbross lbross closed this as completed Nov 22, 2017
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

No branches or pull requests

3 participants