Added ability to delete the default admin user #1131

eharris · 2019-06-15T10:06:51Z

This transitions to solely using Admin Roles rather than a hardcoded admin
account. This allows for deleting the initial admin account, which had
previously been a security weakness since the account name was not able to
be changed. Fixes #640.

eharris · 2019-06-15T10:15:49Z

I would recommend that #1129 is merged before this.

airsonic-main/src/main/java/org/airsonic/player/dao/UserDao.java

airsonic-main/src/main/java/org/airsonic/player/service/SecurityService.java

jvoisin · 2019-06-15T15:37:04Z

I'll test this one as soon as #1129 is merged :)

eharris · 2019-06-27T05:42:46Z

Due to changes in another PR, this will probably have merge conflicts, but I'll fix them after #1129 is merged.

airsonic-main/src/main/java/org/airsonic/player/controller/GeneralSettingsController.java

airsonic-main/src/main/java/org/airsonic/player/validator/UserSettingsValidator.java

airsonic-main/src/main/webapp/WEB-INF/jsp/generalSettings.jsp

muff1nman · 2019-07-13T19:26:00Z

This seems bizzare to me. What is the benefit of adding an AdminUser setting? IMO we should either go all the way to Roles where there can be multiple admin users or leave it as is.

eharris · 2019-07-14T02:03:17Z

This PR just makes it so that the "primary admin" user can be any account rather than being hardcoded as the user with the username of admin, and is aimed at the minimal change-set that will allow that.

Roles do already allow for multiple accounts to have "admin" privileges, but I think it still makes sense to have a special "primary" admin account that has special protections and actions, such as not being allowed to be deleted (even by other users with the admin role). For simplicity and a clean PR, this doesn't add anything additional, but I could see things like the primary admin account automatically having access to all media directories, whereas now individual accounts, even if they have the "admin" role do not. There may be other "special" things that would be useful as well.

But if you would prefer not to have anything past the more general "admin" role, I can probably rework the PR to do that.

eharris · 2019-08-08T22:59:51Z

@muff1nman Did you still want changes to this in order to merge it?

muff1nman · 2019-09-10T19:57:33Z

My gut feeling is that we should either leave it as is or go all the way to an admin role that can be assigned. Having a special admin seems like a bad idea in my mind. If we need further segmentation between lower and higher level admins, that probably means there should be more than one role.

eharris · 2019-09-11T08:20:41Z

I've updated this PR as @muff1nman has requested to not have a "special" admin account and instead use only roles for admin.

A side effect of this change is that automatic playlist imports do not work any longer, since there is no "primary" user to own them, and there is not (yet) any way to auto-import playlists for individual users. One way of resolving this is described in #233 (comment)

eharris · 2019-10-07T15:45:30Z

Fixed the merge conflicts from other merges.

airsonic-main/src/main/java/org/airsonic/player/controller/LoginController.java

airsonic-main/src/main/java/org/airsonic/player/controller/SubsonicRESTController.java

jvoisin · 2019-10-08T15:55:44Z

airsonic-main/src/main/java/org/airsonic/player/controller/SubsonicRESTController.java

+            error(request, response, ErrorCode.NOT_FOUND, "No such user: " + username);
+            return;
+        } else if (user.getUsername().equals(username)) {
+            error(request, response, ErrorCode.NOT_AUTHORIZED, "Not allowed to delete own user");


What about "You're not allowed to delete your own user" instead?

Since these messages are in the REST API, I kept them very terse, as no one should ever see them except developers.

airsonic-main/src/main/java/org/airsonic/player/service/PlaylistService.java

jvoisin · 2019-10-08T15:59:28Z

airsonic-main/src/main/java/org/airsonic/player/service/PlaylistService.java

@@ -296,8 +295,11 @@ private void importPlaylistIfUpdated(File file, List<Playlist> allPlaylists) thr
        }
        InputStream in = new FileInputStream(file);
        try {
-            importPlaylist(User.USERNAME_ADMIN, FilenameUtils.getBaseName(fileName), fileName, in, existingPlaylist);
-            LOG.info("Auto-imported playlist " + file);
+            // FIXME - With the transition away from a hardcoded admin account to Admin Roles, there is no longer


Should this be fixed before merging this PR?

No one has suggested a good solution to this issue, and I don't want fixing this security issue to wait on one. This side effect has only become a problem because @muff1nman wanted not to keep a "primary" admin user, so I've changed the PR to comply with that request. @muff1nman which would you prefer, keeping this version with this side effect, or going back to the original version of this PR that kept a "primary" admin user?

I'm probably going to work on the solution I proposed in #233 (comment) at some point which would resolve this playlist issue as well as many others, but that is almost certainly going to be a low priority, and again I don't want to wait on something like that for this to get merged.

For now could you chose the first admin user found? Really I'd like to see the playlist owner field be nullable but that will probably take some work todo.

I'd rather not put in a half-baked hack, since it seems unlikely that playlist importing is all that usable in its current form anyway(*). I'd rather get the security regarding the hardcoded admin account fixed and merged, and then later get some focus on improving/fixing the playlist functionality.

(*) Is there even any documentation on how the playlist importing stuff is supposed to work that would help anyone actually use it? For instance, how are files in the playlist supposed to have their paths specified? How does it pick which media folder is used as the base? etc...

I used it a year ago. I don't think the negligible security gained here is worth losing the feature over.

@muff1nman So then would you prefer that I revert back to the original version of this PR that kept a "primary" admin user, in order to preserve the playlist functionality? I don't want to put playlist overhaul in as part of this PR.

Nope I'd prefer you kept it as is. For the playlist user you can:

Use the first admin user

Allow the playlist import user to be configurable

Change the playlist to allow a nullable user field

Although I still think 3 would be ideal/workable, 1 or 2 would be considerably easier and be much better than straight out removing this functionality.

As requested, PR has been modified to use the first admin account for playlist imports.

This transitions to solely using Admin Roles rather than a hardcoded admin account. This allows for deleting the initial admin account, which had previously been a security weakness since the account name was not able to be changed. Fixes airsonic#640.

muff1nman · 2019-10-18T07:32:00Z

airsonic-main/src/main/java/org/airsonic/player/service/SecurityService.java

+                return user.getUsername();
+            }
+        }
+        return null;


I'd throw an exception here instead of returning null.

I don't really like returning exceptions from simple lookup functions, especially when returning null for "not found" is such a universally accepted (and expected) pattern. I think it restricts potential use cases unnecessarily and violates separation of concerns. Why should this function need to enforce that the higher level system thinks not having a user with a particular role is an exception-worthy problem?

In any case, I believe that when this function returns null, the calling site will still generate an exception when it tries to use the null value, and does so without this function needing to have any concept of higher level user permissions concerns.

I also didn't want to rule out the possibility that someone might want to completely lock down their installation by disabling all admin accounts (even though doing so would require directly changing the db).

Returning null also allows for if/when someone changes the code to implement your suggestion number 3 above (making the owner of imported playlists nullable), although I admit that at that point this function might just be removed entirely, but who knows?

@muff1nman If throwing an exception is a requirement to get your approval, say so and I will change.

I don't really like returning exceptions from simple lookup functions, especially when returning null for "not found" is such a universally accepted (and expected) pattern.

In Java land its not universally accepted. In other languages, go, c/c++, python, yes I would agree.

Why should this function need to enforce that the higher level system thinks not having a user with a particular role is an exception-worthy problem?

Its in the SecurityService and warrants a bit more checking around assumptions to ensure we fail fast. With this change we still have an assumption that there should always be at least one admin user. When that assumption changes, we should get this exception and we can change the code to deal with the new assumptions then.

I also didn't want to rule out the possibility that someone might want to completely lock down their installation by disabling all admin accounts (even though doing so would require directly changing the db)

In the case the user does this anything that uses this function should rightly be broken. If the calling code can be robust in the scenario of no admin users, its on the function to handle the exception.

Returning null also allows for if/when someone changes the code to implement your suggestion number 3 above (making the owner of imported playlists nullable), although I admit that at that point this function might just be removed entirely, but who knows?

And when that happens we can change/remove this code too :)

It should be noted that the other user lookup functions already in SecurityService (e.g. getUserByName(), getUserByEmail()) also use returning null to indicate "not found", so throwing an exception rather than returning null would be inconsistent with those.

fxthomas · 2020-04-05T09:35:47Z

airsonic-main/src/main/java/org/airsonic/player/service/SecurityService.java

+     * @return username of admin user
+     */
+    public String getAdminUsername() {
+        for (User user : userDao.getAllUsers()) {


Minor thing: we should probably make sure the order is consistent (order by id or user name), I'm not sure of the behavior of all databases, and this might be confusing if the order somehow changes between runs.

fxthomas · 2020-04-05T12:30:05Z

I'm seeing no issue for now in my instance after removing the admin user. Let's see if it stays this way for a few days and I think we can merge this.

fxthomas · 2020-04-07T20:27:53Z

I haven't see any issue, this sounds good to go!

fxthomas · 2020-04-08T20:59:05Z

Let's go ahead and merge it. Thanks for the lot of work on this y'al!

jvoisin self-requested a review June 15, 2019 11:13

jvoisin reviewed Jun 15, 2019

View reviewed changes

airsonic-main/src/main/java/org/airsonic/player/dao/UserDao.java Outdated Show resolved Hide resolved

airsonic-main/src/main/java/org/airsonic/player/service/SecurityService.java Outdated Show resolved Hide resolved

eharris force-pushed the allow-changing-admin branch 2 times, most recently from fbbab4f to f166818 Compare July 2, 2019 10:33

jvoisin suggested changes Jul 2, 2019

View reviewed changes

eharris force-pushed the allow-changing-admin branch from f166818 to 8f84733 Compare July 2, 2019 22:18

jvoisin approved these changes Jul 2, 2019

View reviewed changes

eharris mentioned this pull request Aug 11, 2019

Share link disappeared #1191

Open

eharris assigned muff1nman Sep 3, 2019

eharris assigned eharris and unassigned muff1nman Sep 10, 2019

eharris force-pushed the allow-changing-admin branch 4 times, most recently from 179e3e4 to b535324 Compare September 11, 2019 04:45

eharris assigned muff1nman and unassigned eharris Sep 11, 2019

eharris mentioned this pull request Sep 11, 2019

Admin users now always get access to all media folders #1230

Open

eharris added status: pending-design-work Needs design work before any code can be developed. security labels Sep 15, 2019

eharris added this to the 10.5.0 milestone Sep 16, 2019

eharris force-pushed the allow-changing-admin branch from b535324 to c3a571a Compare October 7, 2019 15:12

eharris requested a review from muff1nman October 7, 2019 15:59

eharris force-pushed the allow-changing-admin branch from c3a571a to 811a46e Compare October 8, 2019 11:40

eharris requested a review from jvoisin October 8, 2019 11:49

jvoisin suggested changes Oct 8, 2019

View reviewed changes

eharris requested a review from jvoisin October 9, 2019 12:27

muff1nman removed the status: pending-design-work Needs design work before any code can be developed. label Oct 16, 2019

eharris force-pushed the allow-changing-admin branch from 811a46e to 3761340 Compare October 16, 2019 06:31

eharris force-pushed the allow-changing-admin branch from 3761340 to aa81d26 Compare October 16, 2019 06:35

muff1nman reviewed Oct 18, 2019

View reviewed changes

fxthomas removed this from the 10.5.0 milestone Dec 23, 2019

jvoisin force-pushed the master branch from c937007 to 6599e6a Compare February 16, 2020 10:19

eharris mentioned this pull request Mar 2, 2020

Airsonic 10.6.0 Release Plan #1438

Closed

13 tasks

eharris requested a review from muff1nman March 3, 2020 07:20

fxthomas reviewed Apr 5, 2020

View reviewed changes

fxthomas added this to the 10.6.0 milestone Apr 5, 2020

fxthomas approved these changes Apr 5, 2020

View reviewed changes

fxthomas merged commit 4394677 into airsonic:master Apr 8, 2020

eharris deleted the allow-changing-admin branch April 17, 2020 18:55

tesshucom mentioned this pull request May 25, 2020

settings gets redirected to personalSettings. cant access generalSettings #1722

Open

Added ability to delete the default admin user #1131

Added ability to delete the default admin user #1131

Uh oh!

Conversation

eharris commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eharris commented Jun 15, 2019

Uh oh!

Uh oh!

Uh oh!

jvoisin commented Jun 15, 2019

Uh oh!

eharris commented Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muff1nman commented Jul 13, 2019

Uh oh!

eharris commented Jul 14, 2019

Uh oh!

eharris commented Aug 8, 2019

Uh oh!

muff1nman commented Sep 10, 2019

Uh oh!

eharris commented Sep 11, 2019

Uh oh!

eharris commented Oct 7, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eharris Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eharris Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fxthomas commented Apr 5, 2020

Uh oh!

fxthomas commented Apr 7, 2020

Uh oh!

fxthomas commented Apr 8, 2020

Uh oh!

Uh oh!

eharris commented Jun 15, 2019 •

edited

Loading

eharris commented Jun 27, 2019 •

edited

Loading

eharris Oct 9, 2019 •

edited

Loading

eharris Oct 15, 2019 •

edited

Loading