Skip to content

Fixes #6556 Expose SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL#13641

Merged
jay-steurer merged 1 commit into
ansible:develfrom
Klaas-:Klaas-enable_social_auth_username_is_full_email
Mar 27, 2023
Merged

Fixes #6556 Expose SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL#13641
jay-steurer merged 1 commit into
ansible:develfrom
Klaas-:Klaas-enable_social_auth_username_is_full_email

Conversation

@Klaas-

@Klaas- Klaas- commented Mar 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor
SUMMARY

Hi, this should fix #6556 - with that change you are able to set the setting that allows to force new social accounts to use email as username.

This change exposes a current default setting

awx/awx/settings/defaults.py

Line 493 in a47cfc5

SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL = False
to be overwrite able via awx settings boolean

ISSUE TYPE
  • New or Enhanced Feature
COMPONENT NAME
  • API
  • UI
AWX VERSION
ADDITIONAL INFORMATION

After rebasing on latest devel the local testing seems to work for me, I can see the setting in awx api and change it through ui

@fosterseth

fosterseth commented Mar 8, 2023

Copy link
Copy Markdown
Member

for the django migration errors, just kill the containers and rerun make docker-compose. there is some kind of race condition the first time starting the containers (when migrations are needed).

@fosterseth fosterseth self-assigned this Mar 8, 2023
@Klaas- Klaas- force-pushed the Klaas-enable_social_auth_username_is_full_email branch from 2120b44 to 00a7aa5 Compare March 8, 2023 20:17
@Klaas-

Klaas- commented Mar 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

for the django migration errors, just kill the containers and rerun make docker-compose. there is some kind of race condition the first time starting the containers (when migrations are needed).

Ah that would also explain why it's working now and it may be unrelated to my rebase on devel :)

@fosterseth

fosterseth commented Mar 9, 2023

Copy link
Copy Markdown
Member

pulled down the branch and tested it out in development environment + keycloak

can confirm with settings.SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL=True that the full email is used in place of the user name in AWX

image

@Klaas- Klaas- force-pushed the Klaas-enable_social_auth_username_is_full_email branch from 00a7aa5 to 89c0685 Compare March 10, 2023 21:14
@Klaas-

Klaas- commented Mar 10, 2023

Copy link
Copy Markdown
Contributor Author

I think I fixed the tests

fosterseth
fosterseth reviewed Mar 15, 2023
View reviewed changes
Comment thread awx/sso/conf.py Outdated
@fosterseth

fosterseth commented Mar 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

UI auth settings page
image

@fosterseth

fosterseth commented Mar 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

image

@Klaas- also, can you please move the setting up toward the top? maybe under "Allow anonymous users to poll metrics"

@Klaas- Klaas- mentioned this pull request Mar 16, 2023
Closed
9 tasks
@Klaas-

Klaas- commented Mar 16, 2023

Copy link
Copy Markdown
Contributor Author

image

@Klaas- also, can you please move the setting up toward the top? maybe under "Allow anonymous users to poll metrics"

I would if I understood how that gets there :)

The allow anonymous users stuff is not in the js details. So I am assuming they get build automatically from settings somehow? :)
awx/api/views/metrics.py: if settings.ALLOW_METRICS_FOR_ANONYMOUS_USERS:
awx/api/views/metrics.py: if settings.ALLOW_METRICS_FOR_ANONYMOUS_USERS or request.user.is_superuser or request.user.is_system_auditor:
awx/api/conf.py: 'ALLOW_METRICS_FOR_ANONYMOUS_USERS',
awx/settings/defaults.py:ALLOW_METRICS_FOR_ANONYMOUS_USERS = False

I was assuming the sorting comes from here:

awx/awx/ui/src/screens/Setting/MiscAuthentication/MiscAuthenticationEdit/MiscAuthenticationEdit.js

Lines 246 to 249 in 89c0685

<BooleanField
name="SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL"
config={authentication.SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL}
/>

but the allow metrics is not included there :)

Greetings
Klaas

@Klaas- Klaas- force-pushed the Klaas-enable_social_auth_username_is_full_email branch from 89c0685 to 39e7eac Compare March 16, 2023 16:52
@Klaas-

Klaas- commented Mar 16, 2023

Copy link
Copy Markdown
Contributor Author

Also the last failed test https://github.com/ansible/awx/actions/runs/4388506123/jobs/7772953312 I am not sure how to correct that one, or is it expected to fail if I add something to the api?

@Klaas- Klaas- force-pushed the Klaas-enable_social_auth_username_is_full_email branch from 39e7eac to 65f53d2 Compare March 16, 2023 17:00
@fosterseth

fosterseth commented Mar 17, 2023

Copy link
Copy Markdown
Member

@Klaas- the schema test is expected to fail since we add this new field to the API. It won't block merging

@fosterseth

fosterseth commented Mar 21, 2023

Copy link
Copy Markdown
Member

@Klaas- a QE engineer will sign off and merge this soon, and the changes will go into the next AWX release. thanks for your contribution!

@Klaas- Klaas- mentioned this pull request Mar 23, 2023
Open
9 tasks
@jay-steurer jay-steurer self-requested a review March 23, 2023 14:27
@jay-steurer

jay-steurer commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

@Klaas- could you rebase your branch? Thanks!

@Klaas-
Fixes ansible#6556 Expose SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL
9da0b82 
Signed-off-by: Klaas Demter <Klaas-@users.noreply.github.com>
@Klaas- Klaas- force-pushed the Klaas-enable_social_auth_username_is_full_email branch from 65f53d2 to 9da0b82 Compare March 23, 2023 14:43
@Klaas-

Klaas- commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

@Klaas- could you rebase your branch? Thanks!

@jay-steurer done

@gundalow gundalow mentioned this pull request Mar 23, 2023
Merged
@cypress

cypress Bot commented Mar 23, 2023

Copy link
Copy Markdown

1 failed and 3 flaky tests on run #14709 ↗︎

1 703 611 0 Flakiness 3

Details:

Fixes #6556 Expose SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL
Project: AWX - Functional Commit: 9da0b82bc6
Status: Failed Duration: 10:18 💡
Started: Mar 23, 2023 7:09 PM Ended: Mar 23, 2023 8:19 PM
Failed  cypress/e2e/integration/job-templates/job-template-crud.cy.js • 1 failed test

View Output Video

Test Artifacts
Job Templates- Create > can create a job template from project job template tab and see the template appear in that list Output Screenshots Video
Flakiness  job-templates/job-template-crud.cy.js • 1 flaky test

View Output Video

Test Artifacts
Job Templates- Create > can create a job template but not if a JT with the same name already exists Output Screenshots Video
Flakiness  inventories/inventory-sources.cy.js • 1 flaky test

View Output Video

Test Artifacts
Inventory Sourced from a Project With Content Signing Enabled > can use a project with content signature verification as an inventory source and successfully sync an inventory source Output Screenshots
Flakiness  user-journeys/normal-user-journey.cy.js • 1 flaky test

View Output Video

Test Artifacts
Activity stream > a normal user should be able to see only allowed events Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@Klaas-

Klaas- commented Mar 25, 2023

Copy link
Copy Markdown
Contributor Author

I don't think those test failures are related to my change

@jay-steurer jay-steurer merged commit 32a5186 into ansible:devel Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fosterseth fosterseth fosterseth approved these changes

@mabashian mabashian mabashian approved these changes

@jay-steurer jay-steurer jay-steurer approved these changes

Assignees

@fosterseth fosterseth

Labels

community component:api component:ui

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Azure AD username mapings

4 participants

@Klaas- @fosterseth @jay-steurer @mabashian