417 hwioauthbundle implementation#65
Draft
iliaskhat wants to merge 2 commits into
Draft
Conversation
Reviewer's GuideAdds optional Azure Entra ID SSO support using HWIOAuthBundle, wiring it into security, user entity, login UI, and configuration, with domain-based auto-provisioning, role syncing, events, and tests; also adjusts migrations path in the README. Sequence diagram for Azure SSO login via HWIOAuthBundlesequenceDiagram
actor User
participant Browser
participant HWIOAuthBundle
participant AzureUserProvider
participant UserRepository
participant EventDispatcher
User->>Browser: Click Sign in with Microsoft
Browser->>HWIOAuthBundle: GET /login/check-azure
HWIOAuthBundle->>AzureUserProvider: loadUserByOAuthUserResponse(response)
alt user matched by azureObjectId
AzureUserProvider->>UserRepository: findOneBy(azureObjectId)
UserRepository-->>AzureUserProvider: User
AzureUserProvider->>User: syncAzureRoles(roles)
AzureUserProvider->>User: update(email, roles)
AzureUserProvider->>UserRepository: save()
AzureUserProvider->>EventDispatcher: dispatch(AzureLoginEvent)
AzureUserProvider-->>HWIOAuthBundle: User
else user matched by email
AzureUserProvider->>UserRepository: findOneBy(email)
UserRepository-->>AzureUserProvider: User
AzureUserProvider->>User: linkAzureAccount(oid)
AzureUserProvider->>User: syncAzureRoles(roles)
AzureUserProvider->>UserRepository: save()
AzureUserProvider->>EventDispatcher: dispatch(AzureLoginEvent)
AzureUserProvider-->>HWIOAuthBundle: User
else new user auto-provisioned
AzureUserProvider->>AzureUserProvider: assertEmailDomainIsAllowed(email)
AzureUserProvider->>User: createFromAzureProfile(email, oid, roles)
AzureUserProvider->>UserRepository: add(User)
AzureUserProvider->>EventDispatcher: dispatch(AzureLoginEvent)
AzureUserProvider-->>HWIOAuthBundle: User
end
HWIOAuthBundle-->>Browser: Authenticated session
Browser-->>User: Logged in via Azure
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
Comment on lines
+116
to
+117
| throw new AccessDeniedException( | ||
| sprintf('Email domain of "%s" is not allowed to log in via Azure.', $email) |
There was a problem hiding this comment.
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.
| @@ -1,8 +1,22 @@ | |||
| hwi_oauth: | |||
| # https://github.com/hwi/HWIOAuthBundle/blob/master/docs/2-configuring_resource_owners.md | |||
| firewall_names: [main] | |||
Member
There was a problem hiding this comment.
Dat is niet meer nodig? Uit 2.0 changelog:
- Deprecated: configuration parameter
firewall_names, firewalls are now computed automatically - all firewalls that have definedoauthauthenticator/provider will be collected.
| resource: "@HWIOAuthBundle/Resources/config/routing/redirect.php" | ||
| prefix: /connect | ||
|
|
||
| hwi_oauth_connect: |
|
|
||
| public function up(Schema $schema): void | ||
| { | ||
| $table = $schema->getTable('user'); |
|
|
||
| App\Controller\User\LoginController: | ||
| arguments: | ||
| $azureClientId: '%env(AZURE_CLIENT_ID)%' |
Member
There was a problem hiding this comment.
Waarom niet via #[Autowire]
|
|
||
| // 1. Match on azure_object_id — most common path after first login | ||
| $user = $this->userRepository->findOneBy(['azureObjectId' => $oid]); | ||
| if ($user !== null) { |
Member
There was a problem hiding this comment.
Ik zou werken met instanceof if ($user instanceof User) { }
| } | ||
|
|
||
| $allowedDomain = ltrim($this->allowedEmailDomain, '@'); | ||
| if (!str_ends_with(strtolower($email), '@' . strtolower($allowedDomain))) { |
Member
There was a problem hiding this comment.
Dat gaat ook nooit werken met meerdere domains? Bv bij LDR hebben we die van LDR en onze van sumocoders.
| arguments: | ||
| $allowedEmailDomain: '%env(AZURE_ALLOWED_EMAIL_DOMAIN)%' | ||
|
|
||
| App\Controller\User\LoginController: |
Member
There was a problem hiding this comment.
Mag dan weg als het via Autowire gaat
|
|
||
| Add the following redirect URIs to the SumoCoders app registration in the Azure Portal: | ||
|
|
||
| * `https://<project>.wip/login/check-sumocoders` |
| Add the following redirect URIs to the SumoCoders app registration in the Azure Portal: | ||
|
|
||
| * `https://<project>.wip/login/check-sumocoders` | ||
| * `https://<project>.phpXX.sumocoders.eu/login/check-sumocoders` |
Member
There was a problem hiding this comment.
project.client.phpXX.sumocoders.eu
| * `https://<project>.wip/login/check-sumocoders` | ||
| * `https://<project>.phpXX.sumocoders.eu/login/check-sumocoders` | ||
|
|
||
| > Replace `<project>` with the project name. |
Member
There was a problem hiding this comment.
En
replace
<client>with client name.
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
2 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 optional Azure SSO via HWIOAuthBundle with support for both client and SumoCoders tenants, including database changes and login flow updates.
New Features:
Enhancements:
Build:
Tests: