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

SELinux-Troubleshooting #3962

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@dperpeet
Copy link
Member

commented Mar 9, 2016

  • basic implementation using react
  • link to fixit (perform fix)
  • add option to view only alerts after a certain point in time
  • workaround: permanently dismiss alerts by adding journal entries
  • fix style
  • add jsx transformation to build step or commit js instead (significantly more difficult to read)
  • get integration tests to work

@dperpeet dperpeet added the needswork label Mar 9, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2016

for jsx you can download react-tools (e.g. npm -g install react-tools) and then transform the script via cd pkg/setroubleshoot && jsx -x jsx views views

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

BTW, IMO this the package should be called 'selinux' with the troubleshooting stuff being one page, and related javascript files, CSS etc.

But as far as react, there's quite a bit to discuss about JSX. In particular in browser JSX transformation (while it works) is being shunned upstream, and also doesn't work with Content-Security-Policy.

There are some ugly ways around this:

  • Abandon the concept of simple editing with just refreshing the browser (ie: no build step). Start requiring people to install Node.JS and run a build command before they can make a single change to Cockpit.
  • Following on from that ... Start using tools like Grunt during development to monitor files and skip the build command. This requires support in cockpit-bridge or similar.
  • Alternatively, Fork JSX transformer and hack our own version that works with CSP.
@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Obviously another approach is to turn off Content-Security-Policy (or at least the unsafe-eval) option during development (which one can do with an override.json). However this makes it easy to ignore issues during development that would not pass CSP.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

JSXTransformer is much easier to switch to CSP compatible than babel is. That's because it uses a script insert, which we can setup with an nonce parameter.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Abandon the concept of simple editing with just refreshing the browser (ie: no build step). Start requiring people to install Node.JS and run a build command before they can make a single change to Cockpit.

I'd be fine with this. Actually, I only work this way - make and install to see changes in the browser.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

I'd be fine with this. Actually, I only work this way - make and install to see changes in the browser.

Yes, but the point has been to lower the barrier to entry, not raise it. Requiring installing software to see changes is a big fail in 2016. There has to be a development mode that's easier to use.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

@andreasn This is a current screenshot with the 'if', 'then' and 'do' text for each possible solution:
screenshot from 2016-03-11 15-04-30

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

Thanks!

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

"SELinux is preventing..." <- Shouldn't this be "SELinux prevented..." since it happened in the past?

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

"SELinux is preventing..." <- Shouldn't this be "SELinux prevented..." since it happened in the past?

This text is the summary of the alert as passed to us by setroubleshootd: https://github.com/bachradsusi/setroubleshoot/wiki/DBUS#return-values-3

If we gather a few of these issues we can open a pull request upstream and discuss.

@dperpeet dperpeet force-pushed the dperpeet:selinux branch 4 times, most recently from 30d17f1 to b508950 Mar 14, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2016

@andreasn current screenshot
screenshot from 2016-03-17 09-21-33

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

"4 occurrences" should go under the delete icon in order to match up better with how these panels are used elsewhere (OSTree, Kubernetes, Registry)

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

I wonder if a more correct label for "Action:" would be "Solution". That would map better with "Apply this solution"

@dperpeet dperpeet force-pushed the dperpeet:selinux branch 2 times, most recently from faab7a9 to 7160e2f Mar 17, 2016

@dperpeet dperpeet added the bot label Mar 18, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

bot: Image refresh for fedora-24

@cockpituous

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

@cockpituous

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Image creation failed.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

bot: Image refresh for fedora-24

@cockpituous

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

@cockpituous

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Image creation failed.

@dperpeet dperpeet removed the bot label Mar 18, 2016

@dperpeet dperpeet force-pushed the dperpeet:selinux branch from 4ae7362 to 9112ca2 Mar 18, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

addressed issues, updated screenshot:
screenshot from 2016-03-18 15-23-09

@@ -131,6 +137,8 @@ CLEANFILES += \
pkg/base1/patterns.min.js \
pkg/base1/moment.min.js \
pkg/base1/mustache.min.js \
pkg/base1/react.min.js \
pkg/base1/react-dom.min.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

Indentation fix needed.

setroubleshoot_MODULES = \
$(PKGDIR)/setroubleshoot.js \
$(PKGDIR)/setroubleshoot-client.js \
$(PKGDIR)/views/setroubleshoot-view.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

Indentation.

$(NULL)
setroubleshoot_DATA = \
$(PKGDIR)/manifest.json \
$(PKGDIR)/setroubleshoot-app.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

indentation.

$(PKGDIR)/setroubleshoot.min.css \
$(PKGDIR)/setroubleshoot.min.html \
$(PKGDIR)/bundle.min.js \
$(PKGDIR)/setroubleshoot-app.min.js \

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

indentation

"order": 8
}
},
"content-security-policy": "default-src 'self'"

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

This line isn't necessary.

This comment has been minimized.

Copy link
@dperpeet

dperpeet Mar 24, 2016

Author Member

it broke for me without that line, I'll test again

This comment has been minimized.

Copy link
@dperpeet

dperpeet Mar 30, 2016

Author Member

it broke for me without that line, I'll test again

it doesn't work for me without the line

function(troubleshoot) {
const app = document.getElementById('app');
troubleshoot.init(app);
});

This comment has been minimized.

Copy link
@stefwalter

stefwalter Mar 24, 2016

Contributor

Seems like this file should be merged with setroubleshoot.js ... that's what I've been doing with all the other places where we're moving code out of the HTML due to CSP.

This comment has been minimized.

Copy link
@dperpeet

dperpeet Mar 24, 2016

Author Member

Seems like this file should be merged with setroubleshoot.js ... that's what I've been doing with all the other places where we're moving code out of the HTML due to CSP.

On the other hand, this way we have cleaner separation. I noticed I haven't removed the jquery dependency in setroubleshoot.js, but I could. That way it could be included from anywhere, with any top level html element.
I guess the question is whether to keep this glue between the actual component and the rendering target or merge them like you did before. I like the separation, but for the sake of having fewer files I could go with the merged variant.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

The way "solution details" jumps a bit when clicked is slightly jarring.
Setting a width of 8px on the i-tag with fa seems to help.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

It breaks the expansion behavior that the green banner shows up in the middle between the expander and the block it's expanding.

screenshot from 2016-03-24 11-50-43

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

It breaks the expansion behavior that the green banner shows up in the middle between the expander and the block it's expanding.

what do you suggest? beneath the details could mean that the result is hidden (because the details can be very, very long)

Moving it up means that the green box can collide with the button.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

what do you suggest? beneath the details could mean that the result is hidden (because the details can be very, very long)

But only if the details are expanded, right?
I think that's ok behavior in that case.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

But only if the details are expanded, right?
I think that's ok behavior in that case.

Wouldn't standard behavior be to look at the details and then press "apply"? In that case I wouldn't see the result of the operation without scrolling. I can also auto-collapse when the user hits apply, that would solve the issue.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

I can also auto-collapse when the user hits apply, that would solve the issue.

That sounds like a neat solution!

@dperpeet dperpeet force-pushed the dperpeet:selinux branch from 24e711b to 220121e Mar 30, 2016

@dperpeet dperpeet force-pushed the dperpeet:selinux branch from 220121e to f405d43 Mar 30, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2016

I can also auto-collapse when the user hits apply, that would solve the issue.

That sounds like a neat solution!

This is how it works now.

@dperpeet dperpeet added the bot label Mar 30, 2016

@dperpeet dperpeet force-pushed the dperpeet:selinux branch from f405d43 to da6ee91 Mar 30, 2016

@dperpeet dperpeet added bot and removed bot labels Mar 30, 2016

dperpeet added some commits Jan 23, 2016

selinux: Add ui for troubleshooting
Add a new cockpit package to troubleshoot SELinux alerts.
The underlying tool used is setroubleshoot, which exposes SELinux
alerts via its DBUS API.
Alerts on the system and possible solutions can be browsed in Cockpit,
while some solutions can be applied at the press of a button.
There is currently no way in Cockpit to dismiss any alerts, since the
underlying API doesn't expose this functionality yet.
base1: Remove alert style override for listings
This is an override that's not suitable for all use cases and shouldn't be global.
The override moved to shell.css where it's used in the credentials dialog.

@dperpeet dperpeet force-pushed the dperpeet:selinux branch from da6ee91 to 576c975 Mar 30, 2016

@dperpeet dperpeet removed the bot label Mar 30, 2016

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2016

issues addressed, tests passed on fedora-24

@stefwalter stefwalter self-assigned this Mar 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.