-
Notifications
You must be signed in to change notification settings - Fork 86
Add OIDC login with Authlib (Google, GitHub, Microsoft) – closes #355 #654
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
base: master
Are you sure you want to change the base?
Conversation
Hi, thanks for the PR! Please look at my comments and check the CI errors. What's completely missing so far is the integration with the existing user system. If I understand your implementation correctly, you are storing the tokens in the session. But |
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.
Pull Request Overview
This PR adds backend-only OIDC login support to the Gramps Web API using Authlib, allowing users to sign in with Google, GitHub, or Microsoft accounts. Key changes include introducing a new OIDC blueprint with provider registrations, integrating the OIDC flow into the Flask app with new login and callback endpoints, and updating environment variable examples.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
gramps_webapi/auth/oidc.py | Adds Authlib OAuth provider registrations and defines login/callback endpoints. |
gramps_webapi/app.py | Integrates the OIDC blueprint into the app and sets up session and user creation logic in a callback endpoint. |
.env.example | Provides required environment variables for OIDC configuration. |
Comments suppressed due to low confidence (2)
gramps_webapi/app.py:214
- There is a duplicate callback route defined in app.py which conflicts with the one already declared in auth/oidc.py. Consider consolidating the callback logic into a single endpoint to avoid unexpected behavior.
@oidc_bp.route("/callback/<provider>")
gramps_webapi/app.py:216
- The variable 'oauth' is not defined in app.py since it is declared in auth/oidc.py but not imported. To resolve this, import 'oauth' from auth/oidc or reuse the already registered client via oidc_bp.
client = oauth.create_client(provider)
Thanks for the work on this. What about support for OIDC providers such as Authentik, Keycloak, Authelia, etc? These can allow logins from their own internal users, or be configured from external sources (e.g. from Google, GitHub, or Microsoft accounts). Ideally we should support both scenarios, as not everyone would want to login from these Google etc accounts. |
I agree, we should definitely add this before shipping this feature, but it might be out of scope of this PR. This should be an MVP demonstrating integration of OIDC with the current authorization system. |
The latest commit contains these key changes:
|
Sorry, but you haven't addressed/responded to my comments and the new code simply creating an owner account is insecure. |
gramps_webapi/app.py
Outdated
@@ -42,6 +44,7 @@ | |||
from .dbmanager import WebDbManager | |||
from .util.celery import create_celery | |||
|
|||
load_dotenv() |
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.
Please do not add dotenv! The place to document needed environment variables is in the documentation repository.
gramps_webapi/app.py
Outdated
"""Flask application factory.""" | ||
app = Flask(__name__) | ||
|
||
app.secret_key = os.getenv("SECRET_KEY") |
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.
Please do not read SECRET_KEY
from the environment! There are already several (documented) ways to set the secret key, with prefixed env or config file.
gramps_webapi/app.py
Outdated
@@ -93,8 +96,7 @@ def create_app(config: Optional[Dict[str, Any]] = None, config_from_env: bool = | |||
deprecated_config_from_env(app) | |||
|
|||
# use prefixed environment variables if exist | |||
if config_from_env: | |||
app.config.from_prefixed_env(prefix="GRAMPSWEB") | |||
app.config.from_prefixed_env(prefix="GRAMPSWEB") |
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.
Please rebase.
gramps_webapi/app.py
Outdated
@@ -207,4 +211,47 @@ def close_user_db_connection(exception) -> None: | |||
def ready(): | |||
return {"status": "ready"}, 200 | |||
|
|||
return app | |||
@oidc_bp.route("/callback/<provider>") |
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.
Please move this to the OIDC module.
@@ -0,0 +1,15 @@ | |||
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.
Can't this be handled as part of the flask configuration? https://docs.authlib.org/en/latest/client/flask.html#configuration
Authlib Flask OAuth registry can load the configuration from Flask app.config automatically.
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.
So this file is not used anymore?
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.
yes
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.
Then why is it still here?
gramps_webapi/auth/oidc.py
Outdated
def configure_oauth(app): | ||
oauth.init_app(app) | ||
|
||
oauth.register( |
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.
Are you actually using OIDC here or just OAuth?
https://docs.authlib.org/en/latest/client/flask.html#flask-openid-connect-client
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.
Note that OIDC is just a defined way to use oauth. From the docs section that you linked:
An OpenID Connect client is no different than a normal OAuth 2.0 client. When registered with openid scope, the built-in Flask OAuth client will handle everything automatically
gramps_webapi/auth/oidc.py
Outdated
email=email, | ||
fullname=user_info.get('name', ''), | ||
role=ROLE_OWNER, # Give owner role to new users | ||
pwhash='', |
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.
You can't just create a user without a tree ID. Also, you cannot just ignore the setting whether registering a new user is allowed for a given tree. Finally, you can absolutely not just give the user the role owner, allowing them to access, edit, and delete all data.
I’ve addressed all the requested changes in the latest commits:
|
requirements-dev.txt
Outdated
@@ -8,3 +8,4 @@ pre-commit | |||
celery[pytest] | |||
moto[s3]<5.0.0 | |||
PyYAML | |||
authlib |
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.
This is not a dev requirement! It should jo in pyproject.toml
@@ -0,0 +1,15 @@ | |||
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.
So this file is not used anymore?
# You should have received a copy of the GNU Affero General Public License | ||
# along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
# | ||
|
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.
You cannot remove the copyright notice!! (I know ChatGPT likes to do that...)
gramps_webapi/app.py
Outdated
" variable is deprecated and will stop working in the future." | ||
f" Please use `GRAMPSWEB_{option}` instead." | ||
f"Setting `{option}` via the environment is deprecated. " | ||
f"Use `GRAMPSWEB_{option}` instead." |
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.
Why did you change this??
gramps_webapi/app.py
Outdated
app.logger.setLevel(logging.INFO) | ||
|
||
# load default config | ||
|
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.
If your LLM removes the comments, you need to put them back in
gramps_webapi/auth/oidc.py
Outdated
@oidc_bp.route("/login/<provider>") | ||
def login(provider): | ||
if not current_app.config.get("OAUTH_ENABLED", False): | ||
return jsonify({"error": "OAuth is not enabled"}), 403 |
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.
Please use the utility method abort_with_message
gramps_webapi/auth/oidc.py
Outdated
|
||
email = user_info.get("email") | ||
if not email: | ||
return jsonify({"error": "No email provided"}), 400 |
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.
Same
gramps_webapi/auth/oidc.py
Outdated
|
||
|
||
if not user: | ||
tree_id = get_tree_id("default") # You can replace this logic as needed |
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.
That doesn't make sense, and anyway you're not adding that to the database.
gramps_webapi/auth/oidc.py
Outdated
|
||
|
||
try: | ||
user = User( |
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.
There is an add_user
function for that.
User is added via add_user function now. replaced comments. replaced jsonify with abort_with_message. |
@@ -49,20 +51,10 @@ def deprecated_config_from_env(app): | |||
This function will be removed eventually! | |||
""" | |||
options = [ |
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.
Why is this reformatted?
@@ -122,31 +115,39 @@ def create_app(config: Optional[Dict[str, Any]] = None, config_from_env: bool = | |||
ignore_lock=app.config["IGNORE_DB_LOCK"], | |||
) | |||
|
|||
|
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.
Please don't add white space changes in unmodified code - it clutters the diff
|
||
|
||
@oidc_bp.route("/login/<provider>") | ||
def login(provider): |
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.
Doc string please
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.
And please add an example to the doc string, I see there is a redirect_uri
argument expected in the request? What should it be?
abort_with_message(403, "OAuth is not enabled") | ||
|
||
|
||
redirect_uri = request.args.get("redirect_uri", url_for("oidc.authorize", provider=provider, _external=True)) |
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.
Is oidc.authorize
correct? Why not gramps_webapi.auth.oidc.authorize
?
request_cache.init_app(app, config=app.config["REQUEST_CACHE_CONFIG"]) | ||
|
||
|
||
configure_oauth(app) |
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.
This will register the providers.
Does this involve an HTTP request to the provider endpoint? If so, what if that request fails? Will the whole flask app fail to start?
I've added a couple more comments. |
if app.config.get("OAUTH_GOOGLE_CLIENT_ID") and app.config.get("OAUTH_GOOGLE_CLIENT_SECRET"): | ||
oauth.register( | ||
name="google", | ||
client_id=app.config["OAUTH_GOOGLE_CLIENT_ID"], |
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.
I don't understand why this contradicts the docs which asks for {name}_CLIENT_ID
etc., see https://docs.authlib.org/en/latest/client/flask.html#configuration
oidc_bp = Blueprint("oidc", __name__, url_prefix="/auth") | ||
|
||
|
||
def configure_oauth(app): |
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.
The frontend needs a way of knowing which of the providers have been registered in the end, based on the configuration, otherwise the frontend doesn't know which buttons to show.
I think the best solution is to add it to a list of registered providers to the /api/metadata
endpoint under the server
key.
Hey @DavidMStraub and @yuriBean, quick question. Will the new OICD implementation support other IDPs like Authentik or Authelia? Looking at the code from the PR i didn't see any reference to other providers with some general .env variables. I just saw this code in the oidc blueprints: import os
from authlib.integrations.flask_client import OAuth
from flask import Blueprint, redirect, url_for, session, request
from dotenv import load_dotenv
load_dotenv()
oauth = OAuth()
oidc_bp = Blueprint("oidc", __name__, url_prefix="/auth")
def configure_oauth(app):
oauth.init_app(app)
oauth.register(
name='google',
client_id=os.getenv("GOOGLE_CLIENT_ID"),
client_secret=os.getenv("GOOGLE_CLIENT_SECRET"),
access_token_url='https://oauth2.googleapis.com/token',
authorize_url='https://accounts.google.com/o/oauth2/auth',
api_base_url='https://www.googleapis.com/oauth2/v1/',
client_kwargs={'scope': 'openid email profile'},
)
oauth.register(
name='github',
client_id=os.getenv("GITHUB_CLIENT_ID"),
client_secret=os.getenv("GITHUB_CLIENT_SECRET"),
access_token_url='https://github.com/login/oauth/access_token',
authorize_url='https://github.com/login/oauth/authorize',
api_base_url='https://api.github.com/',
client_kwargs={'scope': 'read:user user:email'},
)
oauth.register(
name='microsoft',
client_id=os.getenv("MICROSOFT_CLIENT_ID"),
client_secret=os.getenv("MICROSOFT_CLIENT_SECRET"),
access_token_url='https://login.microsoftonline.com/common/oauth2/v2.0/token',
authorize_url='https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
api_base_url='https://graph.microsoft.com/v1.0/',
client_kwargs={'scope': 'openid email profile'},
)
@oidc_bp.route("/login/<provider>")
def login(provider):
redirect_uri = url_for("oidc.authorize", provider=provider, _external=True)
return oauth.create_client(provider).authorize_redirect(redirect_uri)
@oidc_bp.route("/callback/<provider>")
def authorize(provider):
client = oauth.create_client(provider)
token = client.authorize_access_token()
user = client.parse_id_token(token) if provider == "google" else client.get('user').json()
session['user'] = user
return {"status": "logged_in", "provider": provider, "user": user} This is just a dirty example of how it could work, supporting even multiple generic providers. But of course having only one other than the big techs would be more than enough as many other projects do. Thanks guys! import os
from authlib.integrations.flask_client import OAuth
from flask import Blueprint, redirect, url_for, session, request
from dotenv import load_dotenv
load_dotenv()
oauth = OAuth()
oidc_bp = Blueprint("oidc", __name__, url_prefix="/auth")
def configure_oauth(app):
oauth.init_app(app)
# Google
oauth.register(
name='google',
client_id=os.getenv("GOOGLE_CLIENT_ID"),
client_secret=os.getenv("GOOGLE_CLIENT_SECRET"),
access_token_url='https://oauth2.googleapis.com/token',
authorize_url='https://accounts.google.com/o/oauth2/auth',
api_base_url='https://www.googleapis.com/oauth2/v1/',
client_kwargs={'scope': 'openid email profile'},
)
# GitHub
oauth.register(
name='github',
client_id=os.getenv("GITHUB_CLIENT_ID"),
client_secret=os.getenv("GITHUB_CLIENT_SECRET"),
access_token_url='https://github.com/login/oauth/access_token',
authorize_url='https://github.com/login/oauth/authorize',
api_base_url='https://api.github.com/',
client_kwargs={'scope': 'read:user user:email'},
)
# Microsoft
oauth.register(
name='microsoft',
client_id=os.getenv("MICROSOFT_CLIENT_ID"),
client_secret=os.getenv("MICROSOFT_CLIENT_SECRET"),
access_token_url='https://login.microsoftonline.com/common/oauth2/v2.0/token',
authorize_url='https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
api_base_url='https://graph.microsoft.com/v1.0/',
client_kwargs={'scope': 'openid email profile'},
)
# Generic OIDC Providers
def register_oidc_provider(name):
prefix = f"OIDC_{name.upper()}_"
oauth.register(
name=name,
client_id=os.getenv(f"{prefix}CLIENT_ID"),
client_secret=os.getenv(f"{prefix}CLIENT_SECRET"),
access_token_url=os.getenv(f"{prefix}ACCESS_TOKEN_URL"),
authorize_url=os.getenv(f"{prefix}AUTHORIZE_URL"),
api_base_url=os.getenv(f"{prefix}API_BASE_URL"),
client_kwargs={'scope': os.getenv(f"{prefix}SCOPE", "openid email profile")},
server_metadata_url=os.getenv(f"{prefix}METADATA_URL"),
)
# Register configured OIDC providers
oidc_providers = os.getenv("OIDC_PROVIDERS", "").split(",")
for provider in oidc_providers:
if provider.strip():
register_oidc_provider(provider.strip().lower())
@oidc_bp.route("/login/<provider>")
def login(provider):
redirect_uri = url_for("oidc.authorize", provider=provider, _external=True)
return oauth.create_client(provider).authorize_redirect(redirect_uri)
@oidc_bp.route("/callback/<provider>")
def authorize(provider):
client = oauth.create_client(provider)
token = client.authorize_access_token()
if provider in ['google', 'microsoft'] or provider.startswith('oidc_'):
user = client.parse_id_token(token)
else:
user = client.get('user').json()
session['user'] = user
return {"status": "logged_in", "provider": provider, "user": user} And we would use these env variables on the docker compose: OIDC_AUTHENTIK_CLIENT_ID=
OIDC_AUTHENTIK_CLIENT_SECRET=
OIDC_AUTHENTIK_ACCESS_TOKEN_URL=
OIDC_AUTHENTIK_AUTHORIZE_URL=
OIDC_AUTHENTIK_API_BASE_URL=
OIDC_AUTHENTIK_METADATA_URL=
OIDC_AUTHENTIK_SCOPE= |
This has already been asked here. #654 (comment) |
To be clear, anyone helping bringing this to the finish line would be very welcome, I won't have time to directly work on this feature, but the current PR is far from being in a mergeable state. |
This PR adds backend-only OpenID Connect (OIDC) login to the Gramps Web API using Authlib. It allows users to log in using Google, GitHub, or Microsoft accounts.
Features
.env
auth/oidc.py
blueprintREADME.md
and.env.example
Setup
Install requirements:
pip install Authlib python-dotenv
Add your .env:
cp .env.example .env
Start server:
flask run