-
Notifications
You must be signed in to change notification settings - Fork 163
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: BoxCCGAuth
add User and Admin clients factory methods without initial token
#883
feat: BoxCCGAuth
add User and Admin clients factory methods without initial token
#883
Conversation
I can update (simplify) the documentation if the code looks OK. |
Really cool to see your contribution!
If you don't mind go ahead. The code looks fine to me. Just one missing thing I mentioned in another comment |
I'm not the best at writing docu, so hopefully, it makes sense. There were invalid links, so I've updated them. You can review it now. Do I have to fix something myself to make integration tests pass? based on CI log it seems like it's not configured. |
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.
Nice catch with old links! Just a small typo and we are ready to go 🚀
The integration tests use some preconfigured account that calls the Box API directly. Since this account needs credentials which are stored as secrets, this workflow is restricted to internal collaborators to prevent malicious code that could be triggered by the fork (like leaking the secrets). Tests will be run on the main branch when we merge this PR. |
Thanks @JanVargovsky once again for your input. It's always nice to see contributions from the community. I'll try to release a new package to nuget with these changes early next week to make it easier to access this feature. Have a great weekend! |
I get the security perspective, I just thought that it would pass, you just have to run it manually. Thanks for the review :) |
Fixes #879