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.

Sign up for GitHub

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

Jump to bottom

Updates for ALDAP v2.0 #16

Merged
merged 3 commits into from
May 26, 2021
Merged

Updates for ALDAP v2.0 #16

merged 3 commits into from
May 26, 2021

Conversation

dignajar
Copy link
Owner

@dignajar dignajar commented May 24, 2021 •
edited
Loading

  • Include requirements.txt with fixed versions
  • Update to Python 3.9.5
  • Run container as non-root
  • Support for matching users or matching groups, allows service account that doesn't belong to the groups
  • Cache supports matching groups
  • Session encryption key via parameters
  • Update K8S manifests for support HTTPS

marianobilli
marianobilli approved these changes May 25, 2021
View reviewed changes
Copy link
Collaborator

@marianobilli marianobilli left a comment

Choose a reason for hiding this comment

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

Just a small suggestion to wrap initialization of parameters inside function for main program

files/main.py
LDAP_REQUIRED_GROUPS = request.headers["Ldap-Required-Groups"]
elif "LDAP_REQUIRED_GROUPS" in environ:
LDAP_REQUIRED_GROUPS = environ["LDAP_REQUIRED_GROUPS"]
# List of groups separated by comma
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have all the retrieval of parameters/headers inside a function. Otherwise the main its a bit too long until you get to the part of the validation logic

Copy link
Owner Author

Choose a reason for hiding this comment

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

The object request.headers exists only in some context of the application, also I would like to set these parameters outside in one place..

rodrigoechaide
rodrigoechaide approved these changes May 25, 2021
View reviewed changes
Copy link
Collaborator

@rodrigoechaide rodrigoechaide left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@dignajar
Copy link
Owner Author

I changed matching-groups for allowed-groups and allowed-users

@dignajar dignajar merged commit 6f9ca86 into master May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marianobilli marianobilli marianobilli approved these changes

@rodrigoechaide rodrigoechaide rodrigoechaide approved these changes

@GlycineASoya GlycineASoya Awaiting requested review from GlycineASoya

@leandro-loos leandro-loos Awaiting requested review from leandro-loos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dignajar @marianobilli @rodrigoechaide