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

identities authenticate fails without a desktop enviroment #76

Closed
tegefaulkes opened this issue Dec 8, 2023 · 11 comments · Fixed by #150
Closed

identities authenticate fails without a desktop enviroment #76

tegefaulkes opened this issue Dec 8, 2023 · 11 comments · Fixed by #150
Assignees
Labels
bug Something isn't working

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Dec 8, 2023

Describe the bug

Running polykey identities authenticate in an environment that is missing a desktop environment or a web browser fails.

I suppose this somewhat intended since the OAUTH procedure depends on a web browser. But the error throw is very low level in this case. That said I don't think we have a method of adding an identity without going through OAUTH. So how do we add identities to nodes in a server environment.

I think we can to the browser portion of the procedure on a different computer, so the only issue here is that the command is crashing while failing to spawn the browser.

To Reproduce

  1. In an enviroment that lacks the desktop such as an Ubuntu server, run the polykey identities authenticate command This will throw Error: spawn xdg-open ENOENT.

Expected behaviour

If we fail to spawn a web browser then just proceed as normal. Don't crash the command.

Screenshots

image

Platform (please complete the following information)

  • Device: Ubuntu VM running in an Unraid NAS
  • OS: Ubuntu 22.04
@tegefaulkes tegefaulkes added the bug Something isn't working label Dec 8, 2023
@tegefaulkes
Copy link
Contributor Author

I think all we really need here is better error handling for the authentication process. If we fail to open a browser, either due to the a browser is missing or even x-server is missing. we just need to ignore the problem and just print out the URL.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Dec 13, 2023

If a browser is not available, it is not possible for one to automatically open it up.

In that case the correct procedure is to just tell the user to visit the particular URL that we would have auto opened it to.

This should be similar to the tailscale login command. It does not even bother auto opening the url in the browser.

I believe the auto open might depend on an environment variable or something. We could catch the exception, identify if it is the right kind of exception, then show the correct URL and tell the user to open it.

The output format here though will need to be more explicit. It might be important to use a key value structure.

Currently our output formats don't have any human readable formats. We have lists, tables, dictionaries... Etc.

If we only show a url, there's no context for the end user. So a key value might provide more context.

But this entire process is also interactive. So it might need to be a series of key-values.

We generally want the CLI to be machine parseable as it is not a TUI.

@amydevs
Copy link
Member

amydevs commented Feb 22, 2024

I personally also think that the exec for should happen on the CLI client and not the CLI agent. This would make it so that it is the client's responsibility to engage with the OAuth flow, whilst the agent simply just passively waits for a response from the OAuth server. (In this case it's polling).

@CMCDragonkai
Copy link
Member

Would it work whether the client is remote to the agent? I think in that case, it would need to be client opening the browser.

@amydevs
Copy link
Member

amydevs commented Feb 22, 2024

My bad, I thought I remembered it as the agent opened the browser, the client does open the browser correctly

@tegefaulkes
Copy link
Contributor Author

Ok, so it seems that the indentitiesUtils has a browser function being called by the CLI. This is what handles spawing the browser. The placement seems weird that the function is defined in the Polykey lib but only used by the Polykey-CLI. It's going to make fixing it annoying too.

So I'm just going to transplant that function into the Polykey-CLI utils and fix it there.

@CMCDragonkai
Copy link
Member

Assign yourself the issues if you're working on it!

@CMCDragonkai
Copy link
Member

Did you check this? #76 (comment). If you are in a non desktop environment. You should just give the URL to the user to open up manually.

@CMCDragonkai
Copy link
Member

Check how tailscale does it too.

@tegefaulkes
Copy link
Contributor Author

The command already gives your the URL to go to. So all I had to ensure is that failing to open the browser doesn't cause the process to crash.

@CMCDragonkai
Copy link
Member

Great - so in that case, the user is expected to open it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

3 participants