Ath default OIDC #651
Merged
Ath default OIDC #651
Conversation
This file contains 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
- If in the authproviders.ini.local configuration file for ldap mapping no value is entered we apply default values
- If the authproviders file does not contain any configuration or if it is empty, the list of providers is not displayed
- Retrieve OIDC roles to assign ACLs based on configuration - Manage role priority to apply top ranked ACL in profile_order of configuration file - Fixed complete destruction of the local session following disconnection from the provider upon disconnection
Reviewer's Guide by SourceryThis pull request implements authentication against an OIDC provider. It introduces changes in Sequence diagram for OIDC Authentication FlowsequenceDiagram
actor User
participant Web as Web Interface
participant OIDC as OIDC Provider
participant MMC as MMC Agent
User->>Web: Click OIDC provider button
Web->>OIDC: Redirect to provider login
OIDC-->>User: Display login form
User->>OIDC: Enter credentials
OIDC->>Web: Return with auth code
Web->>OIDC: Exchange code for tokens
OIDC-->>Web: Return tokens and user info
Web->>MMC: Create/Update user account
Note over Web,MMC: Map OIDC user data to local user
Web->>MMC: Set user ACL based on roles
MMC-->>Web: Confirm user setup
Web-->>User: Redirect to dashboard
Sequence diagram for OIDC Sign-out FlowsequenceDiagram
actor User
participant Web as Web Interface
participant OIDC as OIDC Provider
User->>Web: Request sign-out
Web->>OIDC: Sign-out request with ID token
OIDC-->>Web: Confirm sign-out
Web->>Web: Destroy local session
Web-->>User: Redirect to login page with sign-out message
Class diagram for User Authentication ComponentsclassDiagram
class AuthenticationHandler {
+handleProviderSelection(providersConfig)
+handleAuthentication(providerKey, providersConfig)
+handleSignout()
+getAclString(userInfo, providersConfig, providerKey)
+updateUserMail(uid, mail)
}
class UserManager {
+add_user(uid, password, firstName, lastName, homeDir, createHomeDir, ownHomeDir, primaryGroup)
+setAcl(user, aclString)
+auth_user(username, password, persist)
+changeUserAttributes(uid, attribute, value)
}
class SessionManager {
+handleSession()
+generateStr(length)
}
AuthenticationHandler --> UserManager
AuthenticationHandler --> SessionManager
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 @spointu - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider standardizing error handling across the codebase - some places use proper error handling with redirects while others just exit(). This could improve maintainability and user experience.
- Please add documentation for the new ACL role mapping feature, including examples of how to configure the profiles_order and acl_ mappings in the config file.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| function updateUserMail($uid, $mail) |
There was a problem hiding this comment.
🚨 suggestion (security): Consider adding email format validation before updating
Adding validation here would provide an additional security layer, even if validation exists elsewhere
|
|
||
| return $providersConfig[$providerKey]['lmcACL'] ?? ''; | ||
| } | ||
|
|
||
| // manages the Openid Connect authentication process (Auth + Return phase) | ||
| function handleAuthentication($providerKey, $providersConfig) |
There was a problem hiding this comment.
suggestion: Consider extracting user management logic into a separate function
This would improve code organization and make the authentication flow easier to understand
Suggested implementation:
// Handles user-specific management operations
function handleUserManagement()
{
if (isset($_POST['lang'])) {
$lang = htmlspecialchars($_POST['lang'], ENT_QUOTES, 'UTF-8');
$_SESSION['lang'] = $_POST['lang'];
}
}
// manages the Openid Connect authentication process (Auth + Return phase)
function handleAuthentication($providerKey, $providersConfig)
{
// Handle the authentication process
handleUserManagement();
Since we can only see part of the code, you'll need to:
- Review and move any other user-related operations from
handleAuthenticationintohandleUserManagement - Make sure any necessary variables or parameters are passed between the functions
- Update any references to these functions elsewhere in the codebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Implement authentication through OpenID Connect (OIDC) providers.
New Features:
Tests: