-
Couldn't load subscription status.
- Fork 281
Fix detection of plugdev group for Linux users #1214
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
base: development
Are you sure you want to change the base?
Conversation
- Add method isInElevatedGroup - check if the linux user is in plugdev - add logic for admin error check
|
Can I get some response to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, work on GUI v7 has begun, and I would like to include this PR.
Let's keep this change isolated to only check on Linux and separated from other admin privileges checks.
| */ | ||
| public boolean isInElevatedGroup() { | ||
| boolean result = true; | ||
| if (isLinux()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverse if and do if(!isLinux) return at the beginning of this method.
|
|
||
| if (!isAdminUser() || isElevationNeeded()) { | ||
| outputError("OpenBCI_GUI: This application is not being run with Administrator access. This could limit the ability to connect to devices or read/write files."); | ||
| if (!isInElevatedGroup() && (!isAdminUser() || isElevationNeeded())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please separate this out to new if statement and say if (isLinux() && !isInElevatedGroupLinux()) since this only applies to one OS.
| * | ||
| * @return <code>true</code> if user is in group plugdev, <code>false</code> otherwise. | ||
| */ | ||
| public boolean isInElevatedGroup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename isInElevatedGroupLinux()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you please confirm this is a positive fix for all Linux users?
What do you mean by all Linux users? Do you talking about adding unit tests? If that's the case, I am wiling to contribute some as well, but I am not sure how to simulate/mock the scenario. |
I mean all Linux users, not just for your use case. |
I just realized, that's mostly just a debian/ubuntu thing these days, to control usb access via static plugdev group. Further, it seems most other Distributions (Fedora, OpenSuse, Arch, even Gentoo) relay on We also only tested for Cyton Biosensing Board Dongle, based on our availability. In General, I would say yes, every Linux user is getting profit from changes in this area, since running GUI Applications in with sudo is in general a bad idea. Specific for us: We are dividing user and admin strictly on the systems, which makes sudo not accessible for the users. An even better solution, would be proper packaging of the software, which also provide the specific udev rule with access to the usb dongle via uaccess. |
|
So how does what you've described overlap/interact with an existing fix? Did you try the existing fix and it didn't work? Found here: https://docs.openbci.com/Software/OpenBCISoftware/GUIDocs/#linux-users-serial-port-permissions Side Note: We can realistically only target the most popular Linux distributions. |
|
I just checked https://wiki.debian.org/SystemGroups And I can no longer remember, why we ended with the
Absolutly. I was just surprised, that only debian based distributions are the only one which are focusing on the group approach to apply udev rules. The uaccess approach seems to be the new way and debian is just the last horse. I see two solution for the MR:
Or a combination of both. Or (don't prefered by me) close it and rely on the docs. |
|
Moving forward on this ticket, it should be separated out for Linux users and not combined with the Windows methods. See existing feedback. |
Our users are always for root password. This is no Option.
We only use Cyton Dongle and this Improvment aims for Dongle usage.
To use Dongle under linux users must be in plugdev group.