-
Notifications
You must be signed in to change notification settings - Fork 678
SMQ-3222 - Add OAuth support to CLI #3255
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: main
Are you sure you want to change the base?
Conversation
4d0c18c to
d89aabc
Compare
db6fbe7 to
b0c2226
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3255 +/- ##
==========================================
+ Coverage 35.55% 39.85% +4.29%
==========================================
Files 307 64 -243
Lines 39314 15077 -24237
==========================================
- Hits 13979 6009 -7970
+ Misses 24477 8674 -15803
+ Partials 858 394 -464 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cli/oauth.go
Outdated
| shutdownTimeout = 5 * time.Second | ||
| ) | ||
|
|
||
| const successHTML = `<!DOCTYPE html> |
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.
Since we have the preview.html files can we remove this constants and use embed library
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.
Removed preview files and move strings to a separate go file. We are trying to use approach as simple as possible, and also standalone since this is something that will be running on Users end - i.e. we start a small server used for redirection. We are not even using it as a template, as you can see, but just serve it as a string with HTML content type.
nyagamunene
left a comment
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.
LGTM
|
@dborovcanin We can take reference flow from Digital ocean doctl cli tool and Github CLI tool |
|
@arvindh123 What we can do is use Device Auth Flow instead https://datatracker.ietf.org/doc/html/rfc8628. I have a draft prepared, but haven't sent it yet. |
|
@arvindh123 The reason is simple - there is no reason to assume device using CLI has broweser installed. For example, you could and are likely to use CLI when SSH-ed to the cloud component. |
f0da1eb to
839f9d2
Compare
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
839f9d2 to
fee4618
Compare
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
Signed-off-by: dusan <[email protected]>
What type of PR is this?
This is a feature.
What does this do?
It adds an OAuth support to SMQ CLI.
Which issue(s) does this PR fix/relate to?
Resolves #3222.
Have you included tests for your changes?
Yes.
Did you document any new/modified feature?
Yes.
Notes
N/A