-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[feature]: 🚀 add Microsoft OAuth provider #9722
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: preview
Are you sure you want to change the base?
Changes from all commits
2567053
b6703ec
c22eaba
2b521a0
ac3720b
e3d1533
9c4dbc4
bfefdc2
b6ce34c
e87e9e8
56b4f02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| # Copyright (c) 2023-present Plane Software, Inc. and contributors | ||
| # SPDX-License-Identifier: AGPL-3.0-only | ||
| # See the LICENSE file for details. | ||
|
|
||
| import os | ||
| from datetime import datetime | ||
| import pytz | ||
| import requests | ||
|
|
||
| from plane.authentication.adapter.oauth import OauthAdapter | ||
| from plane.license.utils.instance_value import get_configuration_value | ||
| from plane.authentication.adapter.error import ( | ||
| AUTHENTICATION_ERROR_CODES, | ||
| AuthenticationException, | ||
| ) | ||
|
|
||
|
|
||
| class MicrosoftOAuthProvider(OauthAdapter): | ||
| userinfo_url = "https://graph.microsoft.com/v1.0/me" | ||
| scope = "openid email profile https://graph.microsoft.com/User.Read" | ||
| provider = "microsoft" | ||
|
|
||
| def __init__(self, request, code=None, state=None, callback=None): | ||
| (MICROSOFT_CLIENT_ID, MICROSOFT_CLIENT_SECRET, MICROSOFT_TENANT_ID) = get_configuration_value( | ||
| [ | ||
| {"key": "MICROSOFT_CLIENT_ID", "default": os.environ.get("MICROSOFT_CLIENT_ID")}, | ||
| {"key": "MICROSOFT_CLIENT_SECRET", "default": os.environ.get("MICROSOFT_CLIENT_SECRET")}, | ||
| {"key": "MICROSOFT_TENANT_ID", "default": os.environ.get("MICROSOFT_TENANT_ID")}, | ||
| ] | ||
| ) | ||
| if not (MICROSOFT_CLIENT_ID and MICROSOFT_CLIENT_SECRET): | ||
| raise AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["MICROSOFT_NOT_CONFIGURED"], | ||
| error_message="MICROSOFT_NOT_CONFIGURED", | ||
| ) | ||
| tenant = MICROSOFT_TENANT_ID or "common" | ||
| self.token_url = f"https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token" | ||
| self.auth_url = f"https://login.microsoftonline.com/{tenant}/oauth2/v2.0/authorize" | ||
| redirect_uri = f"{'https' if request.is_secure() else 'http'}://{request.get_host()}/auth/microsoft/callback/" | ||
| from urllib.parse import urlencode | ||
| url_params = { | ||
| "client_id": MICROSOFT_CLIENT_ID, | ||
| "scope": self.scope, | ||
| "redirect_uri": redirect_uri, | ||
| "response_type": "code", | ||
| "state": state, | ||
| } | ||
| auth_url = f"{self.auth_url}?{urlencode(url_params)}" | ||
| super().__init__( | ||
| request, self.provider, MICROSOFT_CLIENT_ID, self.scope, redirect_uri, | ||
| auth_url, self.token_url, self.userinfo_url, | ||
| client_secret=MICROSOFT_CLIENT_SECRET, code=code, callback=callback, | ||
| ) | ||
|
|
||
| def set_token_data(self): | ||
| data = { | ||
| "code": self.code, | ||
| "client_id": self.client_id, | ||
| "client_secret": self.client_secret, | ||
| "redirect_uri": self.redirect_uri, | ||
| "grant_type": "authorization_code", | ||
| "scope": self.scope, | ||
| } | ||
| token_response = self.get_user_token(data=data) | ||
| super().set_token_data({ | ||
| "access_token": token_response.get("access_token", ""), | ||
| "refresh_token": token_response.get("refresh_token", None), | ||
| "access_token_expired_at": ( | ||
| datetime.fromtimestamp(token_response.get("expires_in"), tz=pytz.utc) | ||
| if token_response.get("expires_in") else None | ||
| ), | ||
| "refresh_token_expired_at": None, | ||
| "id_token": token_response.get("id_token", ""), | ||
| }) | ||
|
|
||
| def set_user_data(self): | ||
| headers = {"Authorization": f"Bearer {self.token_data.get('access_token')}"} | ||
| user_info_response = requests.get(self.userinfo_url, headers=headers).json() | ||
| email = user_info_response.get("mail") or user_info_response.get("userPrincipalName") | ||
| user_data = { | ||
| "email": email, | ||
| "user": { | ||
| "avatar": "", | ||
| "first_name": user_info_response.get("givenName", ""), | ||
| "last_name": user_info_response.get("surname", ""), | ||
| "provider_id": user_info_response.get("id"), | ||
| "is_password_autoset": True, | ||
| }, | ||
| } | ||
| super().set_user_data(user_data) | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,68 @@ | ||||||
| # Copyright (c) 2023-present Plane Software, Inc. and contributors | ||||||
| # SPDX-License-Identifier: AGPL-3.0-only | ||||||
| # See the LICENSE file for details. | ||||||
|
|
||||||
| import uuid | ||||||
| from django.http import HttpResponseRedirect | ||||||
| from django.views import View | ||||||
|
|
||||||
| from plane.authentication.provider.oauth.microsoft import MicrosoftOAuthProvider | ||||||
| from plane.authentication.utils.login import user_login | ||||||
| from plane.authentication.utils.redirection_path import get_redirection_path | ||||||
| from plane.authentication.utils.user_auth_workflow import post_user_auth_workflow | ||||||
| from plane.license.models import Instance | ||||||
| from plane.authentication.utils.host import base_host | ||||||
| from plane.authentication.adapter.error import AuthenticationException, AUTHENTICATION_ERROR_CODES | ||||||
| from plane.utils.path_validator import get_safe_redirect_url | ||||||
|
|
||||||
|
|
||||||
| class MicrosoftOauthInitiateEndpoint(View): | ||||||
| def get(self, request): | ||||||
| request.session["host"] = base_host(request=request, is_app=True) | ||||||
| next_path = request.GET.get("next_path") | ||||||
| if next_path: | ||||||
| request.session["next_path"] = str(next_path) | ||||||
| instance = Instance.objects.first() | ||||||
| if instance is None or not instance.is_setup_done: | ||||||
| exc = AuthenticationException( | ||||||
| error_code=AUTHENTICATION_ERROR_CODES["INSTANCE_NOT_CONFIGURED"], | ||||||
| error_message="INSTANCE_NOT_CONFIGURED", | ||||||
| ) | ||||||
| return HttpResponseRedirect(get_safe_redirect_url( | ||||||
| base_url=base_host(request=request, is_app=True), next_path=next_path, | ||||||
| params=exc.get_error_dict())) | ||||||
| try: | ||||||
| state = uuid.uuid4().hex | ||||||
| provider = MicrosoftOAuthProvider(request=request, state=state) | ||||||
| request.session["state"] = state | ||||||
| return HttpResponseRedirect(provider.get_auth_url()) | ||||||
| except AuthenticationException as e: | ||||||
| return HttpResponseRedirect(get_safe_redirect_url( | ||||||
| base_url=base_host(request=request, is_app=True), next_path=next_path, | ||||||
| params=e.get_error_dict())) | ||||||
|
|
||||||
|
|
||||||
| class MicrosoftCallbackEndpoint(View): | ||||||
| def get(self, request): | ||||||
| next_path = request.GET.get("next_path") | ||||||
| code = request.GET.get("code") | ||||||
| state = request.GET.get("state") | ||||||
| if not code or not state: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/makeplane-plane-9ea351f0 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- Microsoft app callback ---'
cat -n apps/api/plane/authentication/views/app/microsoft.py | sed -n '1,100p'
printf '%s\n' '--- shared OAuth adapter ---'
cat -n apps/api/plane/authentication/adapter/oauth.py | sed -n '55,145p'Repository: makeplane/plane Length of output: 9986 🏁 Script executed: printf '%s\n' '--- repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/makeplane-plane-9ea351f0/conventions/repo-wide.md
printf '%s\n' '--- API learnings ---'
cat /tmp/coderabbit-repo-knowledge/makeplane-plane-9ea351f0/learnings/apps-api-plane.md
printf '%s\n' '--- Microsoft provider ---'
cat -n apps/api/plane/authentication/provider/oauth/microsoft.py | sed -n '1,130p'
printf '%s\n' '--- OAuth completion path ---'
cat -n apps/api/plane/authentication/adapter/oauth.py | sed -n '1,60p;140,230p'Repository: makeplane/plane Length of output: 8901 Broken Authentication (CWE-352): Cross-Site Request Forgery (CSRF) Reachability: External · Exploitability: Moderate Bind the callback to the initiating session. Compare the callback 🤖 Prompt for AI Agents |
||||||
| exc = AuthenticationException( | ||||||
| error_code=AUTHENTICATION_ERROR_CODES["MICROSOFT_OAUTH_PROVIDER_ERROR"], | ||||||
| error_message="MICROSOFT_OAUTH_PROVIDER_ERROR", | ||||||
| ) | ||||||
| return HttpResponseRedirect(get_safe_redirect_url( | ||||||
| base_url=base_host(request=request, is_app=True), next_path=next_path, | ||||||
| params=exc.get_error_dict())) | ||||||
| try: | ||||||
| provider = MicrosoftOAuthProvider(request=request, code=code, callback=post_user_auth_workflow) | ||||||
| user = provider.authenticate() | ||||||
| user_login(request=request, user=user, is_app=True) | ||||||
| path = next_path or get_redirection_path(user=user) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use the session-stored post-login path. Lines 22-24 save the initiation Use Proposed fix- path = next_path or get_redirection_path(user=user)
+ path = request.session.pop("next_path", None) or get_redirection_path(user=user)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| return HttpResponseRedirect(get_safe_redirect_url( | ||||||
| base_url=base_host(request=request, is_app=True), next_path=path, params={})) | ||||||
| except AuthenticationException as e: | ||||||
| return HttpResponseRedirect(get_safe_redirect_url( | ||||||
| base_url=base_host(request=request, is_app=True), next_path=next_path, | ||||||
| params=e.get_error_dict())) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # Copyright (c) 2023-present Plane Software, Inc. and contributors | ||
| # SPDX-License-Identifier: AGPL-3.0-only | ||
| # See the LICENSE file for details. | ||
|
|
||
| import uuid | ||
| from django.http import HttpResponseRedirect | ||
| from django.views import View | ||
| from django.utils.http import url_has_allowed_host_and_scheme | ||
|
|
||
| from plane.authentication.provider.oauth.microsoft import MicrosoftOAuthProvider | ||
| from plane.authentication.utils.login import user_login | ||
| from plane.license.models import Instance | ||
| from plane.authentication.utils.host import base_host | ||
| from plane.authentication.adapter.error import AuthenticationException, AUTHENTICATION_ERROR_CODES | ||
| from plane.utils.path_validator import get_safe_redirect_url, validate_next_path, get_allowed_hosts | ||
|
|
||
|
|
||
| class MicrosoftOauthInitiateSpaceEndpoint(View): | ||
| def get(self, request): | ||
| request.session["host"] = base_host(request=request, is_space=True) | ||
| next_path = request.GET.get("next_path") | ||
| instance = Instance.objects.first() | ||
| if instance is None or not instance.is_setup_done: | ||
| exc = AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["INSTANCE_NOT_CONFIGURED"], | ||
| error_message="INSTANCE_NOT_CONFIGURED", | ||
| ) | ||
| return HttpResponseRedirect(get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_space=True), next_path=next_path, | ||
| params=exc.get_error_dict())) | ||
| try: | ||
| state = uuid.uuid4().hex | ||
| provider = MicrosoftOAuthProvider(request=request, state=state) | ||
| request.session["state"] = state | ||
| auth_url = provider.get_auth_url() | ||
| return HttpResponseRedirect(get_safe_redirect_url( | ||
| base_url=auth_url, next_path=None, params={})) | ||
| except AuthenticationException as e: | ||
| return HttpResponseRedirect(get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_space=True), next_path=next_path, | ||
| params=e.get_error_dict())) | ||
|
|
||
|
|
||
| class MicrosoftCallbackSpaceEndpoint(View): | ||
| def get(self, request): | ||
| next_path = request.GET.get("next_path") | ||
| code = request.GET.get("code") | ||
| state = request.GET.get("state") | ||
| stored_state = request.session.get("state") | ||
| if state != stored_state or not code: | ||
| exc = AuthenticationException( | ||
| error_code=AUTHENTICATION_ERROR_CODES["MICROSOFT_OAUTH_PROVIDER_ERROR"], | ||
| error_message="MICROSOFT_OAUTH_PROVIDER_ERROR", | ||
| ) | ||
| return HttpResponseRedirect(get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_space=True), next_path=next_path, | ||
| params=exc.get_error_dict())) | ||
| try: | ||
| provider = MicrosoftOAuthProvider(request=request, code=code) | ||
| user = provider.authenticate() | ||
| user_login(request=request, user=user, is_space=True) | ||
| next_path = validate_next_path(next_path=next_path) | ||
| url = f"{base_host(request=request, is_space=True).rstrip('/')}{next_path}" | ||
| if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): | ||
| return HttpResponseRedirect(url) | ||
| return HttpResponseRedirect(base_host(request=request, is_space=True)) | ||
| except AuthenticationException as e: | ||
| return HttpResponseRedirect(get_safe_redirect_url( | ||
| base_url=base_host(request=request, is_space=True), next_path=next_path, | ||
| params=e.get_error_dict())) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -367,13 +367,14 @@ | |
| DATA_UPLOAD_MAX_MEMORY_SIZE = int(os.environ.get("FILE_SIZE_LIMIT", 5242880)) | ||
|
|
||
| # Cookie Settings | ||
| SESSION_COOKIE_SECURE = secure_origins | ||
| SESSION_COOKIE_SECURE = os.environ.get("SESSION_COOKIE_SECURE", "false").lower() == "true" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/makeplane-plane-9ea351f0 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- settings context ---'
sed -n '330,390p' apps/api/plane/settings/common.py
printf '%s\n' '--- references to the setting ---'
rg -n --glob '*.py' 'SESSION_COOKIE_SECURE|SECURE_PROXY_SSL_HEADER|secure_origins' apps/api/planeRepository: makeplane/plane Length of output: 4786 🏁 Script executed: printf '%s\n' '--- changed-file diff ---'
git diff -- apps/api/plane/settings/common.py
printf '%s\n' '--- secure_origins definition ---'
sed -n '160,205p' apps/api/plane/settings/common.py
printf '%s\n' '--- session cookie save path ---'
sed -n '60,105p' apps/api/plane/authentication/middleware/session.py
printf '%s\n' '--- production settings ---'
sed -n '1,35p' apps/api/plane/settings/production.pyRepository: makeplane/plane Length of output: 4109 🏁 Script executed: printf '%s\n' '--- repository state ---'
git status --short
git rev-parse --short HEAD
printf '%s\n' '--- parent value ---'
git show HEAD^:apps/api/plane/settings/common.py | sed -n '360,378p'
printf '%s\n' '--- current and deployment references ---'
rg -n --hidden --glob '!*.lock' --glob '!node_modules' 'SESSION_COOKIE_SECURE|CORS_ALLOWED_ORIGINS|X_FORWARDED_PROTO' .
printf '%s\n' '--- applicable repository convention ---'
cat /tmp/coderabbit-repo-knowledge/makeplane-plane-9ea351f0/conventions/repo-wide.mdRepository: makeplane/plane Length of output: 4292 Security Misconfiguration (CWE-614): Sensitive Cookie in HTTPS Session Without 'Secure' Attribute Reachability: External · Exploitability: Moderate Use a secure production default for session cookies.
🤖 Prompt for AI Agents |
||
| SESSION_COOKIE_HTTPONLY = True | ||
| SESSION_ENGINE = "plane.db.models.session" | ||
| SESSION_COOKIE_AGE = int(os.environ.get("SESSION_COOKIE_AGE", 604800)) | ||
| SESSION_COOKIE_NAME = os.environ.get("SESSION_COOKIE_NAME", "session-id") | ||
| SESSION_COOKIE_DOMAIN = os.environ.get("COOKIE_DOMAIN", None) | ||
| SESSION_SAVE_EVERY_REQUEST = os.environ.get("SESSION_SAVE_EVERY_REQUEST", "0") == "1" | ||
| SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https") | ||
|
|
||
| # Admin Cookie | ||
| ADMIN_SESSION_COOKIE_NAME = "admin-session-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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: makeplane/plane
Length of output: 50372
🏁 Script executed:
Repository: makeplane/plane
Length of output: 1827
🌐 Web query:
Requests Python documentation timeout default no timeout requests.get💡 Result:
In the Python Requests library, there is no default timeout; the timeout value defaults to None [1][2]. This means that if you do not explicitly set a timeout, your request will wait indefinitely (hang) if the server does not respond [1][3][4]. Because of this behavior, the official documentation strongly recommends that you always explicitly set a timeout parameter in your requests to prevent your program from hanging indefinitely [1][3][4]. You can set the timeout in several ways: 1. Single value: Setting a single float or integer value (e.g., timeout=5) applies that time limit to both the connect and the read timeouts [1][2]. 2. Tuple: Setting a tuple (e.g., timeout=(3.05, 27)) allows you to specify the connect and read timeouts separately [1][5][6]. The timeout parameter is not a total time limit for the entire request process; rather, it defines the time the client will wait for the server to send data (specifically, the time between bytes received) [1][3][4]. If a timeout occurs, a requests.exceptions.Timeout exception is raised [3][5][4].
Citations:
Set a bounded timeout for the Graph request.
The synchronous
requests.get()call has no timeout. If Microsoft Graph does not respond, the OAuth request can hold a worker indefinitely. Add connect and read timeouts that match the outbound HTTP policy.🤖 Prompt for AI Agents
Source: Linters/SAST tools
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: makeplane/plane
Length of output: 50372
🏁 Script executed:
Repository: makeplane/plane
Length of output: 12531
🏁 Script executed:
Repository: makeplane/plane
Length of output: 26861
Use the shared Graph error path.
When Graph returns a 4xx or 5xx response, call
self.get_user_response()before mapping profile fields. The method callsraise_for_status()and raisesMICROSOFT_OAUTH_PROVIDER_ERROR, which both Microsoft callbacks redirect correctly. Add mocked 401 and 500 coverage.🤖 Prompt for AI Agents