-
Notifications
You must be signed in to change notification settings - Fork 7
Sponsor Login Controller #5372
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
base: main
Are you sure you want to change the base?
Sponsor Login Controller #5372
Conversation
|
I believe the linting is failing because Pint and PHPCS have conflicting documentation. For example: not valid in Pint, valid in PHPCS |
| } | ||
|
|
||
| // Generate and dispatch OTP using Spatie | ||
| $sponsorUser->sendOneTimePassword(); |
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.
The SponsorUser must exist in the database at this point, because the one-time password is associated with their ID. You need to save the SponsorUser before this point if it is a new user.
| $sponsorUser = SponsorUser::where('email', $email)->first(); | ||
| if (! $sponsorUser) { | ||
| // Create temporary unsaved user for OTP verification | ||
| $sponsorUser = new SponsorUser(); | ||
| $sponsorUser->email = $email; | ||
| } |
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.
The user must already exist at this point for an OTP to be generated anyway, so you can simplify this
| $sponsorUser = SponsorUser::where('email', $email)->first(); | |
| if (! $sponsorUser) { | |
| // Create temporary unsaved user for OTP verification | |
| $sponsorUser = new SponsorUser(); | |
| $sponsorUser->email = $email; | |
| } | |
| $sponsorUser = SponsorUser::where('email', $email)->sole(); |
| if (! $sponsorUser->exists) { | ||
| $sponsorUser->save(); | ||
| } |
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.
Not required since the user must already exist earlier in the flow
| // Establish authenticated session | ||
| $request->session()->regenerate(); | ||
| session([ | ||
| 'sponsor_authenticated' => true, | ||
| 'sponsor_id' => $sponsor->id, | ||
| 'sponsor_name' => $sponsor->name, | ||
| 'sponsor_email' => $email, | ||
| ]); | ||
| session()->forget('sponsor_email_pending'); |
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.
Consider using Auth::login instead
| Auth::login($user); |
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.
Are we using that middleware for regular users, and if so, could it be problematic to use Auth::login() on multiple User types?
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.
CasAuthenticate is how all current users authenticate to the web interface, yes.
There would be some kinks to work out with access to normal member pages, but I still think that's a better design than manually managing sessions.
|
|
||
| This password will expire in 10 minutes. | ||
|
|
||
| If you did not request this password, please contact [email protected]. |
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.
We typically include a footer with an unsubscribe link from our email vendor, Postmark
apiary/resources/views/mail/dues/paymentreminder.blade.php
Lines 13 to 15 in db9e03d
| ---- | |
| To stop receiving emails from {{ config('app.name') }}, visit @{{{ pm:unsubscribe }}}. |
There's a bunch of other plumbing we do with unsubscribes via Postmark, but, we can get more into that later.
| // Establish authenticated session | ||
| $request->session()->regenerate(); | ||
| session([ | ||
| 'sponsor_authenticated' => true, | ||
| 'sponsor_id' => $sponsor->id, | ||
| 'sponsor_name' => $sponsor->name, | ||
| 'sponsor_email' => $email, | ||
| ]); | ||
| session()->forget('sponsor_email_pending'); |
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.
Are we using that middleware for regular users, and if so, could it be problematic to use Auth::login() on multiple User types?
| return response()->json([ | ||
| 'success' => true, | ||
| 'message' => 'Login successful! Redirecting to dashboard...', | ||
| 'redirect' => route('home'), |
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.
Where does this redirect to? If it goes to the normal home page, we will need to change it to a sponsor-specific page before deploying the app.
Fixes issue #5322. NOTE: the functionality of the controller has not been tested yet, but I'm making a PR so what I have so far can be reviewed.