Repository navigation
417 hwioauthbundle implementation - #65
Merged
tijsverkoyen merged 13 commits intoMay 20, 2026
Merged
tijsverkoyen merged 13 commits into
tijsverkoyen merged 13 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
azure_object_idcolumn is nullable but has a unique index in the migration; this can cause issues on some databases (e.g. PostgreSQL doesn’t allow multiple NULLs in a unique index), so consider using a partial/index-where non-null or enforcing uniqueness at the application level instead. - Injecting
AZURE_CLIENT_IDandSUMOCODERS_CLIENT_IDas required constructor arguments inLoginControllermeans the container will fail if these env vars are absent; if you want Azure/ Sumocoders login to be truly optional, consider using default env values (e.g.env(default::string: '')) or making the arguments nullable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `azure_object_id` column is nullable but has a unique index in the migration; this can cause issues on some databases (e.g. PostgreSQL doesn’t allow multiple NULLs in a unique index), so consider using a partial/index-where non-null or enforcing uniqueness at the application level instead.
- Injecting `AZURE_CLIENT_ID` and `SUMOCODERS_CLIENT_ID` as required constructor arguments in `LoginController` means the container will fail if these env vars are absent; if you want Azure/ Sumocoders login to be truly optional, consider using default env values (e.g. `env(default::string: '')`) or making the arguments nullable.
## Individual Comments
### Comment 1
<location path="src/Security/OAuth/AzureUserProvider.php" line_range="116-117" />
<code_context>
+
+ $allowedDomain = ltrim($this->allowedEmailDomain, '@');
+ if (!str_ends_with(strtolower($email), '@' . strtolower($allowedDomain))) {
+ throw new AccessDeniedException(
+ sprintf('Email domain of "%s" is not allowed to log in via Azure.', $email)
+ );
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The email-domain error message won't match the existing translation key, so it likely won't be localized.
The NL translation you added is tied to the key `"Email domain is not allowed to log in via Azure."`, but the thrown message is `"Email domain of "%s" is not allowed to log in via Azure."`. Because these differ, this message won’t be localized. To make it translatable, either reuse the existing key and log the email separately, or introduce a translation key with a parameter instead of hardcoding the `sprintf` string.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jonasdekeukelaere
marked this pull request as draft
May 13, 2026 05:39
| resource: "@HWIOAuthBundle/Resources/config/routing/redirect.php" | ||
| prefix: /connect | ||
|
|
||
| hwi_oauth_connect: |
Author
There was a problem hiding this comment.
deze route is niet nodig, word gebruikt om meerdere OAuth providers (google, fb,..) aan 1 account te connecteren.
| {{ form_end(form) }} | ||
|
|
||
| <p class="align-self-center">{{ 'New to x?'|trans }} <a href="{{ url('user_register') }}">{{ 'Sign up'|trans }}</a></p> | ||
|
|
Member
There was a problem hiding this comment.
Ik vraag me af of we niet beter de form vervangen.
added 8 commits
May 13, 2026 13:28
tijsverkoyen
approved these changes
May 13, 2026
jonasdekeukelaere
approved these changes
May 13, 2026
tijsverkoyen
marked this pull request as ready for review
May 20, 2026 07:09
Reviewer's GuideIntegrates optional Azure Entra ID SSO via HWIOAuthBundle, wiring it into the existing Symfony security firewall, extending the User model for Azure identities, exposing Microsoft/SumoCoders login options in the UI, and covering the behavior with tests and documentation. Sequence diagram for Azure SSO login via HWIOAuthBundlesequenceDiagram
actor EndUser
participant Browser
participant HWIOAuth as HWIOAuthBundle
participant Azure
participant AzureUserProvider
participant UserRepository
participant EventDispatcher
EndUser->>Browser: Click Azure login button
Browser->>HWIOAuth: GET /login/check-azure
HWIOAuth->>Azure: Redirect to Azure authorize endpoint
Azure-->>HWIOAuth: Authorization code
HWIOAuth->>Azure: Exchange code for tokens
Azure-->>HWIOAuth: ID token and profile data
HWIOAuth->>AzureUserProvider: loadUserByOAuthUserResponse(response)
alt Existing Azure user (matched by azureObjectId)
AzureUserProvider->>UserRepository: findOneBy(azureObjectId)
UserRepository-->>AzureUserProvider: User
AzureUserProvider->>User: syncAzureRoles(roles)
AzureUserProvider->>User: update(email, roles)
AzureUserProvider->>UserRepository: save()
else Existing local user (matched by email)
AzureUserProvider->>UserRepository: findOneBy(email)
UserRepository-->>AzureUserProvider: User
AzureUserProvider->>User: linkAzureAccount(oid)
AzureUserProvider->>User: syncAzureRoles(roles)
AzureUserProvider->>UserRepository: save()
else No existing user (auto provision)
AzureUserProvider->>AzureUserProvider: User::createFromAzureProfile(email, oid, roles)
AzureUserProvider->>UserRepository: add(User)
end
AzureUserProvider->>EventDispatcher: dispatch(AzureLoginEvent)
AzureUserProvider-->>HWIOAuth: User
HWIOAuth-->>Browser: Authenticated session cookie
Entity relationship diagram for User azure_object_id fielderDiagram
USER {
int id PK
string email
string azure_object_id "nullable, unique"
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The production AzureUserProvider no longer matches the behaviour exercised in AzureUserProviderTest (e.g. constructor signature and allowed-domain logic), so it would be good to align the implementation with the tested contract or update the tests to reflect the intended design.
- syncAzureRoles() currently overwrites the stored roles array, which can drop implicit roles like ROLE_USER; consider normalizing the input so base roles are preserved while still reflecting Azure role changes.
- The login template hides the email/password form entirely when any SSO option is enabled, whereas the README suggests SSO can be used alongside local accounts; consider always rendering the traditional login form and showing the SSO buttons in addition to it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The production AzureUserProvider no longer matches the behaviour exercised in AzureUserProviderTest (e.g. constructor signature and allowed-domain logic), so it would be good to align the implementation with the tested contract or update the tests to reflect the intended design.
- syncAzureRoles() currently overwrites the stored roles array, which can drop implicit roles like ROLE_USER; consider normalizing the input so base roles are preserved while still reflecting Azure role changes.
- The login template hides the email/password form entirely when any SSO option is enabled, whereas the README suggests SSO can be used alongside local accounts; consider always rendering the traditional login form and showing the SSO buttons in addition to it.
## Individual Comments
### Comment 1
<location path="templates/user/login.html.twig" line_range="10-8" />
<code_context>
</div>
{% endif %}
+ {% if azure_login_enabled or sumocoders_login_enabled %}
+ <div class="d-grid gap-3">
+ {% if azure_login_enabled %}
+ <a href="{{ path('hwi_oauth_service_redirect', { service: 'azure' }) }}" class="btn btn-primary btn-lg">
+ <i class="bi bi-microsoft me-2"></i>
+ {{ 'Sign in with Microsoft'|trans }}
+ </a>
+ {% endif %}
+ {% if sumocoders_login_enabled %}
+ <a href="{{ path('hwi_oauth_service_redirect', { service: 'sumocoders' }) }}" class="text-muted btn btn-link">
</code_context>
<issue_to_address>
**question:** Enabling Azure/SumoCoders disables the regular username/password login form entirely.
Given this `if/else` structure, any time `azure_login_enabled` or `sumocoders_login_enabled` is true, the classic form won’t render. If SSO is meant to be additive, this logic should render the SSO buttons alongside the form instead of replacing it. If SSO is meant to disable local login, that behavior is currently implicit, so please confirm it matches product requirements.
</issue_to_address>
### Comment 2
<location path="src/Security/OAuth/AzureUserProvider.php" line_range="89-92" />
<code_context>
+ return $refreshedUser;
+ }
+
+ public function supportsClass(string $class): bool
+ {
+ return $class === User::class;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Strict `supportsClass` check can fail for Doctrine proxy classes.
Because Doctrine proxies use classes like `Proxies\__CG__\App\Entity\User\User`, this strict comparison means the provider won’t support proxied `User` instances, which can cause refresh/auth issues in some setups. Consider using `return is_a($class, User::class, true);` to keep the type narrow while correctly handling proxies.
```suggestion
public function supportsClass(string $class): bool
{
return is_a($class, User::class, true);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -7,6 +7,21 @@ | |||
| </div> | |||
| {% endif %} | |||
There was a problem hiding this comment.
question: Enabling Azure/SumoCoders disables the regular username/password login form entirely.
Given this if/else structure, any time azure_login_enabled or sumocoders_login_enabled is true, the classic form won’t render. If SSO is meant to be additive, this logic should render the SSO buttons alongside the form instead of replacing it. If SSO is meant to disable local login, that behavior is currently implicit, so please confirm it matches product requirements.
Comment on lines
+89
to
+92
| public function supportsClass(string $class): bool | ||
| { | ||
| return $class === User::class; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Strict supportsClass check can fail for Doctrine proxy classes.
Because Doctrine proxies use classes like Proxies\__CG__\App\Entity\User\User, this strict comparison means the provider won’t support proxied User instances, which can cause refresh/auth issues in some setups. Consider using return is_a($class, User::class, true); to keep the type narrow while correctly handling proxies.
Suggested change
| public function supportsClass(string $class): bool | |
| { | |
| return $class === User::class; | |
| } | |
| public function supportsClass(string $class): bool | |
| { | |
| return is_a($class, User::class, true); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added and documented use of hwi/oauthbundle
Summary by Sourcery
Integrate Azure-based SSO using HWIOAuthBundle and expose it as an optional authentication path alongside existing login.
New Features:
Enhancements:
Build:
Tests: