Skip to content
This repository was archived by the owner on Oct 29, 2020. It is now read-only.

Fix bugs with Northstar auth verification.#6401

Merged
DFurnes merged 8 commits intoDoSomethingArchive:devfrom
DFurnes:northstar-verify-touchups
May 2, 2016
Merged

Fix bugs with Northstar auth verification.#6401
DFurnes merged 8 commits intoDoSomethingArchive:devfrom
DFurnes:northstar-verify-touchups

Conversation

@DFurnes
Copy link
Contributor

@DFurnes DFurnes commented May 2, 2016

What's this PR do?

This pull request includes cleanup and fixes to the Northstar auth verification & sync code:

🐛 The "verify" check was previously being run on newly registered users (before their profiles were synced to Northstar), resulting in "failed" validation and a bunch of users with NONE saved to their Northstar ID field. This PR moves this call to occur in a submit handler on the login form, rather than a Drupal hook. Also now just sends "username" and lets Northstar infer which type. (4a655c7)

📄 Updates docblocks to try to make it clearer when different hooks and submit handlers are run, since it was ambiguous from looking at the method names without plopping dpm hooks everywhere. (ab3ec96)

👉 Adds a helper method to save the "Northstar ID" profile field, since the syntax is kinda wonky and it's nice to not have the same code duplicated all over. (71c70e8)

🍃 Removes some commented out code from the Northstar Guzzle implementation, since it was making that file harder to skim. Once we revisit #5932, hopefully we can start using the Northstar PHP library here instead! (3f86eb6)

🍂 General tidying up. (c502379, a7df8ff, 939b0bb)

How should this be manually tested?

It's probably easiest to just test this on staging TBH. But there should be significantly fewer cases where users have NONE for their Northstar ID field, since it's now properly set on registration.

Any background context you want to provide?

👻

What are the relevant tickets?

References #6260.


For review: @angaither @weerd

DFurnes added 7 commits May 2, 2016 13:34
Was ambiguous what kind of ID it was without reading implementation.
This was previously being triggered by a hook that *also* occurred on
register, so new accounts were failing “verify” step because they
hadn’t been forwarded yet.
*
* @param $user - Drupal user
* @param $northstar_response - Northstar user JSON response
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have some getters and setters in the user module, maybe this should live there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use that method here in b27921d!

@angaither
Copy link
Contributor

👍

@DFurnes DFurnes merged commit 36a03ca into DoSomethingArchive:dev May 2, 2016
@DFurnes DFurnes deleted the northstar-verify-touchups branch May 2, 2016 18:30
@DFurnes
Copy link
Contributor Author

DFurnes commented May 5, 2016

Could've sworn I already posted this somewhere, but dropping it here for future reference.

Logging in to an existing account (from homepage or campaign signup) will verify user against POST auth/verify endpoint, and save returned ID to local field. This results in the following methods being called:

        dosomething_user_login_submit
        dosomething_user_northstar_login
        dosomething_northstar_verify_user
        dosomething_northstar_save_id_field

Registering a new user will send new user to the POST users/ endpoint, and save returned ID to local field. This is the result of the following methods being called:

        dosomething_user_new_user
        dosomething_northstar_register_user
        dosomething_northstar_save_id_field
        dosomething_northstar_update_user

Going to the “edit profile” page will send update to the POST users/drupal_id/:drupal_id endpoint:

        dosomething_northstar_update_user

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants