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

Initial functionality #1

Merged
merged 34 commits into from
Jul 31, 2023
Merged

Initial functionality #1

merged 34 commits into from
Jul 31, 2023

Conversation

cbartz
Copy link
Collaborator

@cbartz cbartz commented Jul 28, 2023

This PR adds the basic functionality to support the SAML group attributes using Flask Multipass within Indico.
The current SAML Identity Provider in Flask Multipass does not support the SAML group attributes, see this issue for further explanation.

Implementation notes:

  • Groups are stored/cached in the Indico provided database, as Indico does not do this for external auth providers (see this issue), and group membership information would otherwise be lost on a server restart/crash.
  • Group membership information is updated on every user login.
  • Currently the package is designed as an Indico plugin and the code directly uses the SQLAlchemy instance from the Indico package, but this could be extended to be independent of Indico to be used for other services.
  • The code has only been tested locally and not yet with a real SAML IdP service, so it should be considered experimental. I have added a warning to the README.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 36 files.

Valid Invalid Ignored Fixed
24 1 11 0
Click to see the invalid file list
  • flask_multipass_saml_groups/migrations/20230725_1640_ae387f5fc14a_initial_migration.py

@cbartz cbartz marked this pull request as ready for review July 28, 2023 12:40
@cbartz cbartz requested a review from a team as a code owner July 28, 2023 12:40
pytest.ini Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
arturo-seijas
arturo-seijas previously approved these changes Jul 28, 2023
Copy link
Contributor

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM!

setup.cfg Outdated Show resolved Hide resolved
install-libs.sh Outdated Show resolved Hide resolved
arturo-seijas
arturo-seijas previously approved these changes Jul 31, 2023
Copy link

@mthaddon mthaddon left a comment

Choose a reason for hiding this comment

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

We're missing docstrings for a few methods - are there plans to add those later or is that something that could be addressed now?

@cbartz
Copy link
Collaborator Author

cbartz commented Jul 31, 2023

We're missing docstrings for a few methods - are there plans to add those later or is that something that could be addressed now?

Any missing docstrings should be docstrings equivalent to the base class docstring. I avoided copy & paste there.
I always mention it on the first disable rule # noqa: D102,DCO010 docstring in base class, but did not repeat the reason "docstring in base class" everywhere.

Copy link

@mthaddon mthaddon 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, thanks. And thanks for the thorough PR description.

@cbartz cbartz merged commit 8941740 into main Jul 31, 2023
@cbartz cbartz deleted the initial branch September 6, 2023 07:48
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.

5 participants