📫 refactor: OpenID Email Claim Fallback (#10296)

* 📫 refactor: Enhance OpenID email Fallback

* Updated email retrieval logic to use preferred_username or upn if email is not available.
* Adjusted logging and user data assignment to reflect the new email handling approach.
* Ensured email domain validation checks the correct email source.

* 🔄 refactor: Update Email Domain Validation Logic

* Modified `isEmailDomainAllowed` function to return true for falsy emails and missing domain restrictions.
* Added new test cases to cover scenarios with and without domain restrictions.
* Ensured proper validation when domain restrictions are present.
This commit is contained in:
Danny Avila 2025-10-29 12:57:43 -04:00 committed by GitHub
parent 05c706137e
commit 861ef98d29
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 29 additions and 15 deletions

View file

@ -357,16 +357,18 @@ async function setupOpenId() {
};
const appConfig = await getAppConfig();
if (!isEmailDomainAllowed(userinfo.email, appConfig?.registration?.allowedDomains)) {
/** Azure AD sometimes doesn't return email, use preferred_username as fallback */
const email = userinfo.email || userinfo.preferred_username || userinfo.upn;
if (!isEmailDomainAllowed(email, appConfig?.registration?.allowedDomains)) {
logger.error(
`[OpenID Strategy] Authentication blocked - email domain not allowed [Email: ${userinfo.email}]`,
`[OpenID Strategy] Authentication blocked - email domain not allowed [Email: ${email}]`,
);
return done(null, false, { message: 'Email domain not allowed' });
}
const result = await findOpenIDUser({
findUser,
email: claims.email,
email: email,
openidId: claims.sub,
idOnTheSource: claims.oid,
strategyName: 'openidStrategy',
@ -433,7 +435,7 @@ async function setupOpenId() {
provider: 'openid',
openidId: userinfo.sub,
username,
email: userinfo.email || '',
email: email || '',
emailVerified: userinfo.email_verified || false,
name: fullName,
idOnTheSource: userinfo.oid,
@ -447,8 +449,8 @@ async function setupOpenId() {
user.username = username;
user.name = fullName;
user.idOnTheSource = userinfo.oid;
if (userinfo.email && userinfo.email !== user.email) {
user.email = userinfo.email;
if (email && email !== user.email) {
user.email = email;
user.emailVerified = userinfo.email_verified || false;
}
}

View file

@ -6,15 +6,27 @@ describe('isEmailDomainAllowed', () => {
jest.clearAllMocks();
});
it('should return false if email is falsy', async () => {
it('should return true if email is falsy and no domain restrictions exist', async () => {
const email = '';
const result = isEmailDomainAllowed(email);
expect(result).toBe(true);
});
it('should return true if domain is not present in the email and no domain restrictions exist', async () => {
const email = 'test';
const result = isEmailDomainAllowed(email);
expect(result).toBe(true);
});
it('should return false if email is falsy and domain restrictions exist', async () => {
const email = '';
const result = isEmailDomainAllowed(email, ['domain1.com']);
expect(result).toBe(false);
});
it('should return false if domain is not present in the email', async () => {
it('should return false if domain is not present in the email and domain restrictions exist', async () => {
const email = 'test';
const result = isEmailDomainAllowed(email);
const result = isEmailDomainAllowed(email, ['domain1.com']);
expect(result).toBe(false);
});

View file

@ -3,6 +3,12 @@
* @param allowedDomains
*/
export function isEmailDomainAllowed(email: string, allowedDomains?: string[] | null): boolean {
/** If no domain restrictions are configured, allow all */
if (!allowedDomains || !Array.isArray(allowedDomains) || !allowedDomains.length) {
return true;
}
/** If restrictions exist, validate email format */
if (!email) {
return false;
}
@ -13,12 +19,6 @@ export function isEmailDomainAllowed(email: string, allowedDomains?: string[] |
return false;
}
if (!allowedDomains) {
return true;
} else if (!Array.isArray(allowedDomains) || !allowedDomains.length) {
return true;
}
return allowedDomains.some((allowedDomain) => allowedDomain?.toLowerCase() === domain);
}