Add support for OAuth2 #189

jirijakes · 2023-05-14T09:56:10Z

Draft of OAuth2 support, which should utlimately resolve #187 and hopefully will allow me to /claim #187.

Current status:

Issues:

Works only on JVM with Java 11+
Requires additional dependency (ZIO JSON)

Showcase:

CLAassistant · 2023-05-14T09:56:17Z

All committers have signed the CLA.

jdegoes · 2023-05-14T21:46:06Z

@jirijakes Great to see this!

Indeed, we should support refresh tokens because it requires less interaction from the user, and leads to a better CLI experience.

For JS, the build can be split into JVM and JS source files, like ZIO itself and many other ZIO projects.

Let me know if you have any other questions!

jirijakes · 2023-05-14T23:17:18Z

we should support refresh tokens

Good!

Let me know if you have any other questions!

Thanks, John.

jdegoes · 2023-05-17T09:39:54Z

@jirijakes Do you know when you might get a chance to push this through to completion??

jirijakes · 2023-05-17T10:49:06Z

Do you know when you might get a chance to push this through to completion??

I am progressing very slowly during the week, will have more time Friday to Sunday. I believe on Sunday, it could be ready.

jdegoes · 2023-05-17T12:38:55Z

@jirijakes That's great, thanks for the update!

jirijakes · 2023-05-20T14:23:10Z

Progress report

added storing of access token into encrypted file
- password can by typed-in, provided by argument or provided by external command
- file name can be provided by argument
added Google support
added Facebook support
added refreshing of access tokens

To make refreshing of token work seemlessly, I had to make access to access token effectful:

trait OAuth2Token {
  def refreshTokenNow: Task[Unit]
  def accessToken: Task[AccessToken]
}

With this, whenever accessToken is called, either the existing/active access token is returned immediately or new one is obtained by refreshing it. refreshTokenNow gives users opportunity to refresh access token on demand (e. g. when access token becomes invalid).

JavaScript

The biggest question I have now is what to do with JavaScript. Is the OAuth2 supposed to work with JS? I do have no experience with ScalaJS and when looking around ZIO projects, I could not find one that uses HTTP requests and file system. I found both should be possible with ScalaJS, however the lack of examples in ZIO ecosystem scares me a bit. Could I get an advice on this?

Connected to this is a problem I had with splitting code into JS/JVM. Since Options is a sealed trait, I cannot put case class OAuth2Options extends Options into platform-specific file. Even if OAuth2 does not support JS for the moment, I don't know how to properly make it only available on JVM (I could think of some ugly ways).

What's missing

Besides the JavaScript issue, a bit more documentation has to be added, error handling improved, remove a few ??? and some tests should be written.

Please, let me know if this approach is acceptable and which parts could be done better.

jdegoes · 2023-05-20T14:41:53Z

The biggest question I have now is what to do with JavaScript. Is the OAuth2 supposed to work with JS?

I would not worry about that. Just make sure it compiles under both JVM and JS. if the OAuth is not available in JS, that's fine. Anyone interested in that feature could add it themselves down the road.

Connected to this is a problem I had with splitting code into JS/JVM. Since Options is a sealed trait, I cannot put case class OAuth2Options extends Options into platform-specific file.

Put it into the common trait, just as yet another Options, alongside the existing ones; but push the implementation to a "support trait" which is implemented differently on JVM and JS. The JS implementation will just fail with an error indicating that OAuth is not yet supported on JS.

To make refreshing of token work seemlessly, I had to make access to access token effectful:

This is fine. 👍

added storing of access token into encrypted file

This sounds like it will lead to a negative user experience. Whatever we do here, it should have the usability of, e.g. the git command-line tool.

Many users store keys in ~/.ssh, and an access token is much more secure than that. So I would suggest storing the access tokens into a folder inside the user's home directory is enough.

Thank you for your work on this feature! I think you are nearly there.

jirijakes · 2023-05-21T09:26:15Z

Ready for review.

Recent changes:

access token (together with refresh token, expiration time etc.) is stored in a plaintext file, location can be specified by --oauth2-file parameter. Default is current directory.
split into JS and JVM, JS not implemented
added documentation comments
tested all three providers, Google also with refresh token
error handling: the workflow is completely inside Task. It's mostly calling network and using filesystem plus some encoding/decoding of JSON. I could introduce some custom error types but at the end, I don't think it's worth since only an error message gets printed in case of problems. And that code is not supposed to be used as a library.

Not implemented:

tests; turns out there is not much to unit test without going full-blown ZIO (e. g. HTTP client) but I feel that would be quite overkill. Mostly we call some remote server and get response. If there are some suggestions, I will have a look.

jdegoes · 2023-05-21T10:31:58Z

Looks like there's a compiler crasher. Can you investigate and try to find a workaround? Usually working around Scala compiler's bugs is a matter of identifying the overall location and then experimenting until you find something that fixes it.

To switch SBT to the version that's crashing in the CI, do ++ <version> in the SBT console. Then you should be able to reproduce locally.

zio-cli/js/src/main/scala/zio/cli/oauth2/OAuth2PlatformSpecific.scala

zio-cli/jvm/src/main/scala/zio/cli/OptionsPlatformSpecific.scala

jdegoes · 2023-05-21T10:45:24Z

zio-cli/shared/src/main/scala/zio/cli/oauth2/OAuth2.scala

+   * @param expiresIn maximum period
+   * @return the last response received before terminating
+   */
+  def pollingSchedule(


For things like this that we probably don't want to expose in the public API, be sure to mark private[cli].

I hid the whole class, it's not supposed to be accessible from outside.

jdegoes · 2023-05-21T10:53:18Z

zio-cli/shared/src/main/scala/zio/cli/oauth2/OAuth2.scala

+      val expiresAt     = time.plus(response.expiresIn).withNano(0).toLocalTime()
+
+      s"""| >>
+          | >>  Application requests to perform OAuth2


Really good help docs!

jdegoes · 2023-05-21T10:55:42Z

zio-cli/shared/src/main/scala/zio/cli/Options.scala

+    scope: List[String],
+    auxiliaryOptions: Options[OAuth2AuxiliaryOptions]
+  ) extends Options[OAuth2Token] {
+    override def helpDoc: HelpDoc                = auxiliaryOptions.helpDoc


Rather than delegating everything to auxiliaryOptions, it may make sense to provide a bit of help here. But I suppose that depends on how it's rendered and if there is a good place to put help for a phantom options.

zio-cli/shared/src/main/scala/zio/cli/oauth2/OAuth2Provider.scala

jdegoes

Relatively minor changes, overall looks amazing!

I think we should look at improving help doc a bit: at the top-level help page, we should discuss the OAuth2 support. That will require traversing the options in the CLIApp and seeieng if OAuth2Options is somewhere there, and if so, documenting the feature.

jirijakes · 2023-05-21T12:05:11Z

Thanks for the review, John! I'll make the suggested changes.

Looks like there's a compiler crasher

It seems the cause is HttpClient, which is missing on Java 8 (it was added in Java 11).

So in order to make this work in Java 8, there are two options, as I can see: either use built-in, archaic HttpURLConnection, or a new external dependency.

Can you please advise, what would be best in line with the project?

I think we should look at improving help doc a bit: at the top-level help page, we should discuss the OAuth2 support.

Yes, this makes sense. I will see what I can do.

jdegoes · 2023-05-21T12:26:25Z

Let's bump JVM to 11

jirijakes · 2023-05-21T14:28:50Z

Reflected John's comments.

I added help doc which appears when OAuth2 is used. See screenshot. I am not sure about wording and implementation (misusing p a bit but without me manually wrapping, it did not look good).

Let's bump JVM to 11

Shall I add it to this PR? Does it mean to remove Java 8 from ci.yml?

zio-cli/.github/workflows/ci.yml

Lines 59 to 62 in aadf2a4

    
           matrix: 
        
             java: [ '8', '11', '17' ] 
        
             scala: [ '2.12.17', '2.13.10', '3.2.2' ] 
        
             platform: [ 'JVM', 'JS', 'Native' ]

jdegoes · 2023-05-21T14:33:30Z

I added help doc which appears when OAuth2 is used. See screenshot. I am not sure about wording and implementation (misusing p a bit but without me manually wrapping, it did not look good).

Be sure that when --help is invoked, that no OAuth takes place, so that the user can see the help.

Shall I add it to this PR? Does it mean to remove Java 8 from ci.yml?

Yes, you modify the CI by making changes to the Github workflow. Instead of 8 we can just use 11.

jdegoes · 2023-05-21T14:35:41Z

@jirijakes

I think we should try to store the token in user home directory. Rather than the current directory. The reason is that the user may not have permission to modify the current directory, and also the user's current directory will keep changing all the time. You can get the home directory from Java properties:

System.getProperty("user.home")

jirijakes · 2023-05-21T14:45:47Z

Be sure that when --help is invoked, that no OAuth takes place, so that the user can see the help.

Yup, verified.

Yes, you modify the CI by making changes to the Github workflow. Instead of 8 we can just use 11.

Done.

I think we should try to store the token in user home directory. Rather than the current directory.

Good idea. Done.

jdegoes · 2023-05-22T09:17:36Z

Thank you for your work on this!

algora-pbc bot mentioned this pull request May 14, 2023

Integrate support for OAuth2 authentication & authorization #187

Closed

jirijakes force-pushed the oauth2 branch from 7c25ba9 to bb983ef Compare May 20, 2023 13:56

Add support for OAuth2

286aedc

jirijakes force-pushed the oauth2 branch from bb983ef to 286aedc Compare May 21, 2023 09:14

jirijakes marked this pull request as ready for review May 21, 2023 09:26

jdegoes reviewed May 21, 2023

View reviewed changes

zio-cli/js/src/main/scala/zio/cli/oauth2/OAuth2PlatformSpecific.scala Outdated Show resolved Hide resolved

jdegoes reviewed May 21, 2023

View reviewed changes

zio-cli/js/src/main/scala/zio/cli/oauth2/OAuth2PlatformSpecific.scala Outdated Show resolved Hide resolved

jdegoes reviewed May 21, 2023

View reviewed changes

zio-cli/jvm/src/main/scala/zio/cli/OptionsPlatformSpecific.scala Outdated Show resolved Hide resolved

jdegoes reviewed May 21, 2023

View reviewed changes

zio-cli/shared/src/main/scala/zio/cli/oauth2/OAuth2Provider.scala Outdated Show resolved Hide resolved

jdegoes requested changes May 21, 2023

View reviewed changes

Add help doc, hide sensitive types

ac65a29

ci: Do not build on Java 8

037e1c9

Store access token to home dir by default

469519e

Fix broken links in scaladoc

6aa61b8

jdegoes approved these changes May 22, 2023

View reviewed changes

jdegoes merged commit 803022d into zio:master May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OAuth2 #189

Add support for OAuth2 #189

jirijakes commented May 14, 2023 •

edited

Loading

CLAassistant commented May 14, 2023 •

edited

Loading

jdegoes commented May 14, 2023

jirijakes commented May 14, 2023 •

edited

Loading

jdegoes commented May 17, 2023

jirijakes commented May 17, 2023 •

edited

Loading

jdegoes commented May 17, 2023

jirijakes commented May 20, 2023

jdegoes commented May 20, 2023

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jdegoes May 21, 2023

jirijakes May 21, 2023

jdegoes May 21, 2023

jdegoes May 21, 2023

jdegoes left a comment

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jdegoes commented May 21, 2023

jirijakes commented May 21, 2023

jdegoes commented May 22, 2023

Add support for OAuth2 #189

Add support for OAuth2 #189

Conversation

jirijakes commented May 14, 2023 • edited Loading

CLAassistant commented May 14, 2023 • edited Loading

jdegoes commented May 14, 2023

jirijakes commented May 14, 2023 • edited Loading

jdegoes commented May 17, 2023

jirijakes commented May 17, 2023 • edited Loading

jdegoes commented May 17, 2023

jirijakes commented May 20, 2023

Progress report

JavaScript

What's missing

jdegoes commented May 20, 2023

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jdegoes May 21, 2023

Choose a reason for hiding this comment

jirijakes May 21, 2023

Choose a reason for hiding this comment

jdegoes May 21, 2023

Choose a reason for hiding this comment

jdegoes May 21, 2023

Choose a reason for hiding this comment

jdegoes left a comment

Choose a reason for hiding this comment

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jirijakes commented May 21, 2023

jdegoes commented May 21, 2023

jdegoes commented May 21, 2023

jirijakes commented May 21, 2023

jdegoes commented May 22, 2023

jirijakes commented May 14, 2023 •

edited

Loading

CLAassistant commented May 14, 2023 •

edited

Loading

jirijakes commented May 14, 2023 •

edited

Loading

jirijakes commented May 17, 2023 •

edited

Loading