[#13678] Support MS Entra authentication#13679
[#13678] Support MS Entra authentication#13679TobyCyan wants to merge 54 commits intoTEAMMATES:masterfrom
Conversation
|
Hi @TobyCyan, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
There was a problem hiding this comment.
Pull request overview
Adds Microsoft Entra ID as an additional authentication option alongside existing login flows by introducing a multi-provider auth configuration, wiring provider selection from the UI into backend login/callback handling, and exposing supported providers via the AuthInfo API.
Changes:
- Introduces
app.auth.typesand new provider-specific OAuth2 config keys for Google and Microsoft Entra. - Adds provider selection UX (modal) and frontend plumbing to send the selected
providerto/login. - Adds Microsoft Entra branches in
LoginServlet/OAuth2CallbackServletand surfaces supported providers viaAuthInfo.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/services/auth.service.ts | Adds getAuthTypes() helper and caches auth-info fetch. |
| src/web/app/page.component.ts | Fetches supported providers; opens provider-selection modal; redirects with provider param. |
| src/web/app/page.component.html | Replaces direct login links with modal-trigger buttons and adds modal template. |
| src/web/app/page.component.scss | Adds styling for provider modal and Google/Microsoft sign-in buttons. |
| src/web/app/page.component.spec.ts | Adds unit tests for modal open, provider selection redirect, and button visibility. |
| src/main/resources/build.template.properties | Replaces single auth type with list; adds Google/Microsoft OAuth client config keys. |
| src/main/java/teammates/ui/servlets/AuthServlet.java | Renames Google auth flow helper; adds Microsoft client + redirect URI helper. |
| src/main/java/teammates/ui/servlets/LoginServlet.java | Adds provider param handling and Microsoft Entra authorization redirect branch. |
| src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java | Adds provider detection via encrypted state and Microsoft token redemption branch. |
| src/main/java/teammates/ui/output/AuthInfo.java | Exposes supported auth types (authTypes) to the frontend. |
| src/main/java/teammates/logic/external/FirebaseAuthService.java | Improves failure handling when Firebase credentials resource is missing. |
| src/main/java/teammates/logic/api/AuthProxy.java | Adjusts Firebase auth service initialization / login-email enablement logic. |
| src/main/java/teammates/common/util/Config.java | Adds AUTH_TYPES and new provider-specific OAuth config constants. |
| build.gradle | Adds msal4j dependency for Microsoft Entra integration. |
| package-lock.json | Updates lockfile (incl. @types/handsontable and other metadata changes). |
Comments suppressed due to low confidence (1)
src/main/java/teammates/logic/api/AuthProxy.java:24
AuthProxynow attempts to initializeFirebaseAuthServicewhenever devserver login is disabled, even if Firebase isn’t an enabled auth method for this deployment. This can cause noisy/severe logs and unnecessary startup work (then silently falls back toEmptyAuthService). Consider guarding Firebase initialization behind a config flag (e.g.,AUTH_TYPEScontainsfirebase) or removing Firebase initialization entirely if Firebase auth is no longer supported.
AuthProxy() {
AuthService fs;
if (Config.ENABLE_DEVSERVER_LOGIN) {
fs = new EmptyAuthService();
} else {
try {
fs = new FirebaseAuthService();
} catch (AuthException e) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There are major issues that need to be addressed before we can even start considering reviewing this. Before we can add support for MS Entra, we have to ensure the underlying system is ready to support multiple identity providers in a consistent and extensible way. Firstly, the system itself has to support multiple providers. This means ensuring we have a way to support storing sub and issuer (see #13691), and all the Next, a user identifier should be identified by its subject, scoped to a single issuer. This is the only stable, unique identifier by OIDC spec. All other claims are optional and should not be used as the primary way to identify someone. In fact, the way email is used now is not best practice. On top of not being compliant with the spec, it opens the system up to account hijacking attacks. There are other non-compliant parts in the existing implementations, which I'm sure an LLM will be able to identify, but those are relatively minor compared to this issue. Next, there is a need to figure out how all 4 of our auth methods work (google, entra, firebase, dev) in this new model. They all should map cleanly to it. Firebase is conspicuously missing from this implementation. This will involve first figuring out why firebase was implemented in the first place. Fundamentally, firebase is designed as an identity broker not an identity provider. It's designed to federate identities from upstream IDPs such as google and entra. There's a lot of overlap here and I would consider this an atypical use case for firebase. This should either be removed or we route everything through it and polish it up to be production ready (which it is not at the moment). I would say there's little value to having firebase here since we already manage our own user pool and integrations with IDPs. Ultimately I wouldn't be comfortable merging this in without first coming up with a proper plan that identifies relevant gaps and addressing those before we carry on. This system was never designed for this use case and if we want to do it we should be doing it properly. @wkurniawan07, would appreciate your help on this, I have given my thoughts on this but I may not have enough spare capacity to manage this at the moment. |
|
|
||
| ## Overview | ||
|
|
||
| TEAMMATES supports multiple authentication providers to verify user identity and establish a session. The authentication flow begins at the `LoginServlet`, which determines the appropriate provider based on the `provider` request parameter. Upon successful authentication, a session cookie is initialized using the verified user email. |
There was a problem hiding this comment.
... using the verified user email
May need to be changed once Sai's change is confirmed
| 3. The servlet decrypts and validates the `AuthState`, exchanges the authorization code for an access token, and uses the token to retrieve the user's email. | ||
| 4. The email is used to create a `UserInfoCookie`, which is set as the session cookie. The user is redirected to the original destination. | ||
|
|
||
| Provider-specific differences: |
There was a problem hiding this comment.
Ideally if we are using OIDC, then we would use both sub and iss and there wouldn't be provider-specific difference other than the library
Fixes #13678
The changes can be viewed and tested on this staging website.
Outline of Solution
AUTH.TYPEby defining supported authentication providers as a list inbuild.properties.providerto the back-end so it knows which flow to trigger.LoginServletandOAuth2CallbackServletfor the MS Entra ID logic flow.authentication.md.(I added a button to sign in using firebase, but this will be removed if we decide to deprecate firebase from the codebase alltogether)
Sequence Diagram of

OAuth2AuthSequence.puml