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

replace inline checks with decorator #94

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

replace inline checks with decorator #94

wants to merge 25 commits into from

Conversation

samridhsharma
Copy link

No description provided.

yaron and others added 25 commits November 9, 2017 15:58
Implemented #57. This allows batch outside collaborator repo adding.
- This will change how DUO environment variables are sent in. Please see the updated README.
- This also adds support for hidden commands where help text is not specified. This is useful for
  commands that you have retired. This is so if users type in the retired command, they still
  get some output that you can control -- indicating to them that the command is retired and
  that another command should be used instead -- while not showing up in the help text.
Added multiple-domain DUO support
Added more threads to duo error messages
Small change to include a non-zero exit status if the docker build command failed for any reason.
Update for proper exit status
Properly removes all macOS "Smart Quotes".
@mikegrima
Copy link
Contributor

Wow! Lots of good stuff in there.

I will need a days to look it over.

Copy link
Contributor

@mikegrima mikegrima left a comment

Choose a reason for hiding this comment

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

Awesome PR!

Please address the few items and I'll merge it in and make a new release.

raise NoSecretsProvidedError("Must provide secrets to enable authentication.")
for variable, secret in secrets.items():
if "DUO_" in variable:
domain, host, ikey, skey = secret.split(",")
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems incorrect. All 3 of the variables must be provided for DUO to work.

I would suggest keeping the old code here.

"user_data_required": True,
"help": "Sets team permissions to a specific repository in a specific GitHub organization.",
"permitted_permissions": ["push", "pull"], # To grant admin, add this to the config for
"enabled": True # this command in the config.py.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make disabled by default.

"DUO-HOST": "xxxxxxxx.duosecurity.com",
"DUO-IKEY": "The IKEY for Duo",
"DUO-SKEY": "The SKEY for Duo"
"DUO_YOUR_DOMAIN": "your-domain-here.com,xxxxxxxx.duosecurity.com,THEDUOIKEY,THEDUOSKEY"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer to keep this as is -- it won't break users who upgrade.

Unless you can convince me that this way is better.

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.

None yet

3 participants