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

Login directly to remote machines #5118

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
2 participants
@petervo
Copy link
Contributor

commented Oct 4, 2016

Still a WIP, specifically the known hosts aspect needs work, but figured i'd put this here now to get feedback on what's done so far.

@petervo petervo added the needswork label Oct 4, 2016

@petervo petervo force-pushed the petervo:direct-to-machine branch 3 times, most recently from 7f2111d to 54e8210 Oct 5, 2016

@stefwalter
Copy link
Contributor

left a comment

Nice. I especially like the fact that we're testing the compat. Added some comments.

@@ -250,14 +250,27 @@ function event_mixin(obj, handlers) {
});
}

function calculate_application() {

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

This needs comprehensive tests for all the cases.

@@ -1,7 +1,7 @@
{
"version": "@VERSION@",
"requires": {
"cockpit": "118"
"cockpit": "119.x"

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Needs a cockpit.spec change too.

This comment has been minimized.

Copy link
@petervo

petervo Oct 5, 2016

Author Contributor

I don't think dashboard ever has it's own package

@@ -1,7 +1,7 @@
{
"version": "@VERSION@",
"requires": {
"cockpit": "0.114"
"cockpit": "119.x"

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Needs a cockpit.spec change too, and a debian/control change?

@@ -891,7 +891,10 @@
return defer.promise;

var last, req, kubectl, loginOptions;
var loginData = window.localStorage.getItem('login-data');
var loginData = window.localStorage.getItem(cockpit.transport.application + ':login-data');

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Should we have a cockpit.js helper method for getting transport/application specific data?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Perhaps accept a storage so it can operate on either localStorage or sessionStorage?

var loginData = window.localStorage.getItem('login-data');
var loginData = window.localStorage.getItem(cockpit.transport.application + ':login-data');
if (!loginData)
loginData = window.localStorage.getItem('login-data');

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Should the above helper method incorporate this logic?

if (!memory)
memory = g_strdup (application);
return memory;
return auth_parse_application (path);

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Extra space.

if (g_str_has_prefix (path, "/=/") ||
g_str_has_prefix (path, "/@/") ||
g_str_has_prefix (path, "//"))
{

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Needs adapted tests.

/* Instead of clearing, purge anything that isn't prefixed with an application
* and anything prefixed with our application.
*/
for (i = 0; i < window.sessionStorage.length; i++) {

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Lets make this its own function.

}
if (path.indexOf("/=") === 0)

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Lets split this logic into its own function for readability.

if (url_root)
localStorage.setItem('url-root', url_root);
if (wanted && wanted != window.location.href)
window.location = wanted;
/* Make sure that the base1 version is new enough to handle

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 5, 2016

Contributor

Lets split this into its own function for readability.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2016

Can you update urls.md ... and add significant documentation about how and why? This wouldn't just be a couple line change ... but rather help anyone coming into this world to understand things.

@petervo petervo force-pushed the petervo:direct-to-machine branch 6 times, most recently from 82beedb to b0113c2 Oct 5, 2016

petervo added some commits Oct 4, 2016

base1: Separate browser storage in application
Expose the application that is being used, and use it
to prefix browser storage keys. This allows us applications
to keep their data seperate from each other.

@petervo petervo force-pushed the petervo:direct-to-machine branch 5 times, most recently from b350867 to f0e9475 Oct 5, 2016

@petervo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2016

Addressed review points and added unknown host handling.

petervo added some commits Oct 4, 2016

ws: Only connect to machines known machines
When connecting to a remote machine via an application
only connect to machines that are present in known_hosts.
Unless the remote peer logging is a local address or cockpit
has been configured to allow it.

@petervo petervo force-pushed the petervo:direct-to-machine branch from f0e9475 to e13f44c Oct 12, 2016

@stefwalter stefwalter removed the needswork label Oct 19, 2016

@stefwalter stefwalter changed the title WIP: Login directly to remote machines Login directly to remote machines Oct 19, 2016

stefwalter added a commit that referenced this pull request Oct 19, 2016

ws: Allow connecting to remote machines from login page
Closes #5118
Reviewed-by: Stef Walter <stefw@redhat.com>

stefwalter added a commit that referenced this pull request Oct 19, 2016

ws: Support prompts for unknown-hosts
Closes #5118
Reviewed-by: Stef Walter <stefw@redhat.com>

stefwalter added a commit that referenced this pull request Oct 19, 2016

ws: Only connect to machines known machines
When connecting to a remote machine via an application
only connect to machines that are present in known_hosts.
Unless the remote peer logging is a local address or cockpit
has been configured to allow it.

Closes #5118
Reviewed-by: Stef Walter <stefw@redhat.com>
@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

There are several follow ups necessary, that we can work through.

  • Clicking on the server name on the login page should allow changing it.
  • The layout of the fingerprint prompt needs to be tweaked.
  • Logging out when logged into another server takes us to an unfamiliar screen.
@petervo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

  • Clicking on the server name on the login page should allow changing it.
  • The layout of the fingerprint prompt needs to be tweaked.

We did talk about having a way to select a different machine on the
login screen. But i need some design help with that.

  • Logging out when logged into another server takes us to an
    unfamiliar screen.

I wasn't able to reproduce, what was the unfamiliar screen?

@petervo petervo deleted the petervo:direct-to-machine branch Oct 20, 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.