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

Give siteadmin user the ability to manage domains as a domain manager #230

Merged
merged 6 commits into from
May 1, 2017

Conversation

nklatt
Copy link
Contributor

@nklatt nklatt commented Nov 9, 2016

For your consideration, this branch has the following changes:

Primarily, it (optionally) grants the siteadmin user the capability to manage each domain as though they were the domain manager. This is accessed via the domains table at site.php using the added "manage" link. Clicking "manage" transfers the siteadmin to the admin.php page with the powers of a domain manager for the chosen site. When the siteadmin returns to a siteadmin page (i.e., a page that includes authsite.php, they are "logged out" of the domain and return to being the siteadmin user.

Secondarily, it has a few other changes in it:

  1. Fixed a warning message when password_strengthcheck is called with no username in $_POST, e.g. from adminuseradd.php.
  2. Added some zebra striping to the domains table at site.php.
  3. Some trailing whitespace is removed. (As a reminder, append ?w=1 to the compare url to see a diff with the whitespace changes ignored.)
  4. Some final closing php tags are removed. (Per https://secure.php.net/manual/en/language.basic-syntax.phptags.php)

@rimas-kudelis
Copy link
Collaborator

I was hoping to leave this feature for 3.0, but since we have code involved... @Udera, would you like to review the PR?

@Udera
Copy link
Collaborator

Udera commented Dec 29, 2016

Sorry for my late reply. I'm ok with all the code format improvements.

The new feature is nice, however at some point we need to stop to implement stuff here in order to get a version 3.0 started. However, we should somehow get 3.0 going, if not we will restart fixing 2.3 at some later point. Can we schedule a release plan?

@rimas-kudelis
Copy link
Collaborator

We are fixing 2.3, I just don't feel like we should spend time adding new features to it. But then, this feature is handled to us on a plate AND it doesn't touch the database, so I think it's OK to use it. We could even tag 2.4 at some point, considering that we've already had other improvements as well.

Regarding 3.0, I haven't been moving in that direction lately. I was going to try out some minimal PHP frameworks, but then moved on to other stuff. Considering the discussion we had at the end of 2015, I'd say the framework being minimal desired, but not really required, so I guess we could as well just use one of the popular/trending frameworks for that, even if it's a bit larger. But this isn't the right place for that discussion – we could either discuss it on our mailing list, or create a separate issue with "Version 3.0" milestone to discuss frameworks.

@Udera
Copy link
Collaborator

Udera commented Apr 3, 2017

I will try to review this soon.

@Udera
Copy link
Collaborator

Udera commented Apr 16, 2017

This feature seems to work. I tested it a bit (not the full feature list). Code review is a bit difficult as many other things were changed as well.

@rimas-kudelis Do you want to have a look at the authentication stuff as well?

die();
}

if ($_SESSION['username'] != "siteadmin"){
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see what changed here. But once your are changing it, you can use !== instead of != as well.

@@ -105,4 +114,6 @@
}
if (isset($_GET['login']) && ($_GET['login'] == "failed")) { print _("Login failed"); }

if ($validateAsSiteadmin) print '<a href="site.php" style="float:right; margin-right:20px;">siteadmin home</a>';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you create a new class in the style.css? I started to remove inline css-code to be able to use content security policy properly.

@Udera
Copy link
Collaborator

Udera commented May 1, 2017

Ok, thanks for the quick changes @nklatt.
I'm merging this in, then we can test all the changes together before the release.

@Udera Udera merged commit 62cfb53 into vexim:master May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants