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

feat: support Client Credentials Grant as authentication method #335

Merged
merged 17 commits into from
Jul 5, 2022

Conversation

mhagmajer
Copy link
Contributor

Uses getCCGClientForUser method added with box/box-node-sdk#709.

@mhagmajer mhagmajer requested review from antusus and mwwoda May 30, 2022 09:14
Copy link
Contributor

@antusus antusus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think CCG should require as-user to be set to login. I should be able to use CCG with service account.
I should be able to login as admin using subject "user". After that if I have privileges I can use as-user header but it is not required.
Why in this solution we are not using configuration command and allowing users to add new environment that will be using CCG? (https://github.com/box/boxcli/blob/main/docs/configure.md)

The Node SDK version that supports CCG is 2.1.0 you forgot to bump the version of the SDK in package.json

@mhagmajer
Copy link
Contributor Author

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
Copy link
Contributor

antusus commented Jun 9, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mhagmajer mhagmajer requested a review from antusus July 5, 2022 10:46
antusus
antusus previously approved these changes Jul 5, 2022
Copy link
Contributor

@antusus antusus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@antusus antusus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@antusus antusus merged commit 4649d8a into main Jul 5, 2022
@antusus antusus deleted the mh/SDK-1617 branch July 5, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants