Skip to content

Group support & permissions#137

Open
richard-underwood wants to merge 29 commits into
tuxis-ie:masterfrom
richard-underwood:issue-68
Open

Group support & permissions#137
richard-underwood wants to merge 29 commits into
tuxis-ie:masterfrom
richard-underwood:issue-68

Conversation

@richard-underwood
Copy link
Copy Markdown
Contributor

@richard-underwood richard-underwood commented Jan 9, 2017

Hi, this is a pretty big set of changes, so it would be worth getting some early feedback. It's designed to fix the following:

Issue 68 - group support
Issue 17 - forbid users from editing administrative records

Unfortunately, there's no point in having groups without some sort of permissions system, so that's included as well.

As the DB schema also needed updating for this, I've put a framework in place for schema versioning.

@tuxis-ie
Copy link
Copy Markdown
Owner

tuxis-ie commented Jan 9, 2017

Holy moly! I'll try to review this soon and await the rest of the patches. But good work!

@richard-underwood
Copy link
Copy Markdown
Contributor Author

Thanks, one thing I should mention is that the interface is using j-query autocomplete rather than drop-downs for the user and group selection boxes as the number of users could be large. I hope this is OK.

@tuxis-ie
Copy link
Copy Markdown
Owner

tuxis-ie commented Jan 9, 2017

Yeah, I'd think that's ok.

@righter83
Copy link
Copy Markdown

Hi are there any news to commit this into master?
We can use that group feature also.
thanks

@pasikarkkainen
Copy link
Copy Markdown

Ping? Any plans to get this PR reviewed and merged?

@tuxis-ie
Copy link
Copy Markdown
Owner

tuxis-ie commented Nov 7, 2017

@richard-underwood Was it ready?

@richard-underwood
Copy link
Copy Markdown
Contributor Author

Apologies - other tasks got in the way and it stalled. It's not finished, and will need re-merging now. I'll try and get it up to date later this week and see what the state of play is.

@tuxis-ie
Copy link
Copy Markdown
Owner

tuxis-ie commented Nov 7, 2017

Not a problem. Thanks.

@richard-underwood
Copy link
Copy Markdown
Contributor Author

I've merged the upstream changes, which looked OK, but not fully tested everything yet.

This is a large change, I'll try and list the changes below:

  • Added "groups" tab similar to the users & admin for group membership
  • Set up a permissions scheme (including restricted editing)
  • Changed DB schema to allow permissions set per zone
  • Implememnted database versioning
  • Moved the records cascade out to its own button
  • Jquery autocomplete.js & menu.js enabled - user lookups are asynchronous as potentially this could be a large list (e.g. with LDAP integration)

Essentially, each domain still has an owner who is always admin, but other users or groups can be assigned to view, update or admin a zone. There's a config option to restrict editing (e.g. to stop SOA and NS records being updated).

I'm certain there's more to do, but it'll take me a while to remember exactly what. In particular, I have a feeling there are issues editing zones which have bene created, but not in the database. Hopefully I'll have a chance to work on this over the next week.

If anyone who is interested in this pull request could have a look ON A TEST SYSTEM and see if it works for you, if there's anything fundamentally missing, if there are any security issues, etc. then it'd be appreciated. Bear in mind that the database schema will be changed, so take a backup of this if it's important to you.

…that this will create users if they don't exist also.
Comment thread README.md
============
Multiple users are supported. A user can be an admin or a normal user. You can
configure wheter or not a normal user is allowed to add new zones.
configure whether or not a normal user is allowed to add new zones.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#159 should cover this

Comment thread permissions.php Outdated

if (!is_adminuser()) {
header('Status: 403');
jtable_respond(null, 'error', "You need adminprivileges to get here");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that adminprivileges should be two words (x2) as there doesn't appear to be a token by this name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, thanks.

@tuxis-ie
Copy link
Copy Markdown
Owner

@righter83 @pasikarkkainen ? Are you able to test this ON A TEST SYSTEM ? I'm currently not able to test this extensively.

@righter83
Copy link
Copy Markdown

Hi, thanks for investing more time into this feature!

I've pulled it into my test system and almost everything works.
I've tested the main stuff and not everthing.
The only bug I found:
Zone Clone: if you clone a zone and set a specific owner user: owner is admin afterwards instead of the specifiic user.

just my 2c on this feature.
We needed a superadmin, and medium admins for our reseller structure. That means a medium-admin is also granted to create new zones -> Everyone in his group will be granted to edit that zone.
But the medium admin sees only the zones which are granted to them. the superadmin sees then all zones.

My programming is not that fine, therefore I haven't mad a pull request.
But if others are interested in this model i could try to invest more time into it as soon as this pull will be merged into the master.

thanks

@richard-underwood
Copy link
Copy Markdown
Contributor Author

righter83 - thanks for looking at it. I didn't think I'd made any changes to zone cloning, so I suspect that that had always happened (but will check). However, the permissions will not be copied on zone clone, which I'm guessing they should be - so will need to add that.

Thanks.

@richard-underwood
Copy link
Copy Markdown
Contributor Author

I've raised issue #160 for the owner change on cloning - it looks like the "kind" field is also ignored.

@richard-underwood richard-underwood changed the title WIP: Group support & permissions Group support & permissions Nov 21, 2017
@richard-underwood
Copy link
Copy Markdown
Contributor Author

I'm happy for this to be merged, if everyone else is - although I think it could still do with some more review or testing. In particular, @tuxis-ie, are you happy with the database update scripts?

@metrax
Copy link
Copy Markdown

metrax commented Dec 23, 2017

+1 for merging this feature soon :-)

@ghost
Copy link
Copy Markdown

ghost commented Mar 9, 2018

@tuxis-ie any news about this? If you need some more testing, I'd be available too (have a test system up and running)

@ruben-herold
Copy link
Copy Markdown

+1 for merging that will lift nsedit to a new level

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

Successfully merging this pull request may close these issues.

7 participants