-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the ability to disable local authentication #10102
Add the ability to disable local authentication #10102
Conversation
Build failed.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
Note that it is quite possible even with the setting validation to wind up in a state where all users are locked out. In that case, opening up the shell with |
@one-t are you able to provide the ldap configuration? There shouldn't be any ldap user on the system that doesn't have an ldap_dn associated with it. |
awx/api/conf.py
Outdated
if attrs.get('DISABLE_LOCAL_AUTH', False): | ||
if not any(getattr(settings, s, None) for s in remote_auth_settings): | ||
raise serializers.ValidationError(_("There are no remote authentication systems configured.")) | ||
if not User.objects.exclude(profile__ldap_dn='').exclude(enterprise_auth__isnull=True).exclude(social_auth__isnull=True).exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@one-t for a bit more context, the check is here. This should account for every previously logged-in LDAP user.
This is the LDAP config:
|
The user does have a DN associated:
|
🤦 I think I see what I did wrong... |
and expose it in the settings UI.
when this setting gets turned on.
when the DISABLE_LOCAL_AUTH setting is set. This avoids the ugliness of getting a SuspiciousOperation error for any request/response cycles that are in flight when a user gets bounced.
Build succeeded.
|
I confirmed that this last update fixed the issue, the feature can now be enabled when LDAP is the only provider configured. |
@wenottingham @mabashian We need to address the issue of whether and how to do the user validation when toggling this setting on. The options, as I understand them, are:
During the demo questioning, somebody said something that I didn't quite catch about some types of remote accounts not being checkible for sysadmin privileges? Perhaps I misheard, but I'd like to get some clarification about that. |
Certain identity providers map users into roles (and teams) by attributes. Not all remote user sources allow mapping sysadmin, and while we could in theory check that a mapping is defined for some, we can't check that it would ever actually fire. I'm actually leaning towards 3, then 1, as the preferred options. |
I believe that we need some sort of confirmation modal when disabling local authentication. This feature allows the user to take some pretty drastic actions that could be catastrophic we need to have some sort of modal that tells them what it means to disable local auth, and ask them to confirm that they want to do that. Perhaps @trahman73 can chime in here. |
@AlexSCorey I'm in favor of having a confirmation modal as suggested by Jeff's Option 3. |
@mabashian @trahman73 who from the UI team is going to take responsibility for it? According to the spreadsheet, there is no UI dev assigned to this issue. |
@jbradberry I've assigned myself to this issue for the modal. Is merge being blocked by that UI modal? |
@AlexSCorey depends on how we want to play it. I could take out the user existence validation (since we are trying to do number 3) and merge, and we could do the UI part as a follow-on PR. Or we could just add your commits here. Either way works for me. |
since we are going to do a confirmation modal dialog instead.
The remote user existence validation has been removed. |
Build succeeded.
|
Build succeeded.
|
A couple things that have come to mind while testing this: Do we want to actually disable the local superuser accounts? The original issue mentions leaving local admin accounts available for backup/restore processes. As implemented, this makes it impossible to use oauth2 token-based authentication with AWX, since we disable the creation of oauth2 tokens for external accounts (which would be the only type of valid account with this feature enabled). Giving these tokens to superusers isn't a great solution either but we don't really have a concept of a dedicated Service Account in AWX. One solution might be to add an option to remove the ability for Org Admins to create user accounts. That gets a bit deeper into the RBAC forest but since this option can be disabled and enabled inside awx by a superuser, it doesn't really change the privilege equation. |
The general request by customers is to disable all local accounts. I'd suspect whatever warning in the docs should note that the user may need to enable tokens for social auth users. |
Build succeeded (gate pipeline).
|
Hi team, |
SUMMARY
When an external authentication system is enabled, users would like the ability to disable local authentication for enhanced security.
related #4553
TODO
DISABLE_LOCAL_AUTH
User.objects.filter(Q(profile__isnull=True) | Q(profile__ldap_dn=''), enterprise_auth__isnull=True, social_auth__isnull=True)
get_external_account
auth backend associated with them
external users orauth sources configuredISSUE TYPE
COMPONENT NAME
AWX VERSION