Allow to fill missing custom fields during Omniauth registration - #24652
Conversation
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
|
Noteworthy mentions:
P.S.: If anyone has a clue on how we could properly add tests for this case, I think we sourly need that xD |
NobodysNightmare
changed the title
c2c886f to
e420bf4
Compare
I think the failing spec pointed me to where I can add this missing spec... working on it ^^ |
d47dde9 to
1801be3
Compare
oliverguenther
left a comment
oliverguenther
left a comment
There was a problem hiding this comment.
Changes and specs LGTM.
With the consent no longer happening in that page, we can remove AccountController#self_registration!:379 as that's dead code if I'm not mistaken.
Not quite sure. I can still see calls to I definitely think that more cleanup could be put into all of this. For example I still need to figure out how |
bsatarnejad
left a comment
bsatarnejad
left a comment
There was a problem hiding this comment.
LGTM!
the removed inline consent check is safe because consent is still enforced by :consent auth stage: config/initializers/authentication_stages.rb, so no bypass.
One small cleanup on the now-dead user.assign_attributes, otherwise ready. 👍
| @@ -72,9 +72,7 @@ def register_with_auth_source(user) # rubocop:disable Metrics/AbcSize | |||
| # on-the-fly registration via omniauth or via auth source | |||
| if pending_omniauth_registration? | |||
| user.assign_attributes permitted_params.user_register_via_omniauth | |||
There was a problem hiding this comment.
assign_attributes looks dead now. We used to need it to build user.attributes for register_via_omniauth, but we now pass permitted_params.user_register_via_omniauth directly . Can we drop this line?
There was a problem hiding this comment.
It probably is dead, but I have to admit that I didn't dare touching it. As written above, this user here is passed down from the AccountController and I am not entirely sure whether anyone is depending on side effects of that assignment still.
I'd like to push that cleanup out into a future PR.
| # Is the user authenticated via an external authentication source via OmniAuth? | ||
| def uses_external_authentication? | ||
| user_auth_provider_links.exists? | ||
| user_auth_provider_links.any? |
There was a problem hiding this comment.
Worth a short comment explaining why this must be any? and not exists?
When signing up through Omniauth we don't need to show password fields and the possibility to register through other auth providers (we already have one). The use of `exists?` mandated that the check was performed via a database lookup before, which doesn't work for unpersisted users that don't have corresponding database entries yet, but that do have unpersisted association records already. Therefore `any?` works when `exists?` doesn't.
The params contain more than merely the attributes of the user record. Most notably, they also contain custom fields, which are associated to the user afterwards, but are not immediately part of the attributes. This makes it possible to: 1. Login through Omniauth 2. During registration have validation errors for required custom fields 3. Fill out missing custom fields 4. Register successfully (still being an Omniauth user)
This was added very recently to improve rendering of consent errors in all registration flows. However, for the omniauth case it kicked us out of the flow that would end up in `render_omniauth_registration_form`. Thus a failed consent would switch from the omniauth registration form (that has no password and auth provider inputs, but connects the resulting user to their original auth provider) to the regular registration form. By removing this check, confirming the consent during the registration form is not required anymore. This is at least consistent with remaining omniauth behaviour. Lacking a required field, the user would be created first and then asked to confirm the consent in a separate step. This is what happens now as well. Caveat: Even if the consent is confirmed during the registration form, the second step will pop up anyways. This issue exists regardless of keeping or removing this check and is one more thing we need to fix about omniauth registrations.
1801be3 to
e54f9d1
Compare
4 participants
This PR solves several issues around registering users via Omniauth when their information is not yet complete. We explicitly did want to support this (see methods such as
render_omniauth_registration_form), but lost the ability to do so long ago.This should now be working again.
Ticket
https://community.openproject.org/wp/SI-235