box / boxcli

A command line interface for interacting with the Box API.
https://developer.box.com
Apache License 2.0
229 stars 59 forks source link

feat: support Client Credentials Grant as authentication method #335

Closed mhagmajer closed 2 years ago

mhagmajer commented 2 years ago

Uses getCCGClientForUser method added with https://github.com/box/box-node-sdk/pull/709.

mhagmajer commented 2 years ago

Thank you @antusus for this review!

I've bumped the box-node-sdk dependency to 2.3.0. I made sure to follow https://github.com/box/box-node-sdk/blob/main/docs/authentication.md#client-credentials-grant-authentication in order to allow both enterprise and as-user authentication. You actually need to setup you default environment with configure:environments so that that CCG can pull your clientId and clientSecret from there.

antusus commented 2 years ago

@mhagmajer Most critical issue is that this is still not working. When adding new environment the clientSecret property is not stored in the environment by the configure:environments:add method. When you want to use ccg-auth then clientSecret is empty and it fails. I think you would need lo load config file like JWT is doing configObj = JSON.parse(fs.readFileSync(environment.boxConfigFilePath));

The configure:environments:add always assumes that you are adding JWT configuration and it requires you to put JWT appAuth section with key ID, it's passphrase and private key. So we would ask customers to put this configuration with some dummy data. We have to change that and while adding determine if user wants to add JWT or CCG auth and build our internal configuration and validate configuration object. I would change configure:environments:add method so that is could determine if there is appAuth section - then someone is configuring JWT or does not have it and clientSecret is present then CCG metod is selected by the user and I would remove the ccg-auth flag.

We still are not allowing admin user to use CCG - we should allow users to put userId in the configuration file while defining CCG authentication method. Then we should create client configured for admin user. Now we are using asUser configuration entry. I think we should separate that. I could log in using CCG as an Admin and I would like to provide my userId, then when I want to work on some other user i would use as-user to set this up.

mhagmajer commented 2 years ago

Something weird is going on with the tests. Doesn't seem related to this PR

  1) Sign requests
       sign-requests:get
         should get a sign request by id:
     Error: Timeout of 9000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/boxcli/boxcli/test/box-command.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)
  2) Sign requests
       sign-requests:cancel
         should cancel a sign request by id:
     Error: Timeout of 9000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/boxcli/boxcli/test/box-command.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)
  3) Sign requests
       sign-requests:resend
         should resend a sign request by id:
     Error: Timeout of 9000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/boxcli/boxcli/test/box-command.test.js)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

I've already increased the timeout from 5s to 9s, however, it didn't help

mhagmajer commented 2 years ago

All req changes applied during call with @antusus. Thanks!