8000 fix: Magic Link Login can be used even if `$allowMagicLinkLogins` is false by datamweb · Pull Request #778 · codeigniter4/shield · GitHub
[go: up one dir, main page]
Skip to content

fix: Magic Link Login can be used even if $allowMagicLinkLogins is false#778

Merged
kenjis merged 9 commits intocodeigniter4:developfrom
datamweb:fix-allowMagicLinkLogins
Aug 12, 2023
Merged

fix: Magic Link Login can be used even if $allowMagicLinkLogins is false#778
kenjis merged 9 commits intocodeigniter4:developfrom
datamweb:fix-allowMagicLinkLogins

Conversation

@datamweb
Copy link
Collaborator
@datamweb datamweb commented Aug 9, 2023 •
edited
Loading

fixed: #777

@datamweb datamweb added the bug Something isn't working label Aug 9, 2023
@datamweb datamweb mentioned this pull request Aug 9, 2023
Closed
@datamweb
Copy link
Collaborator Author
datamweb commented Aug 9, 2023

IMG_20230809_233657

@kenjis
Copy link
Member
kenjis commented Aug 9, 2023

Re-ran failed jobs.

@datamweb datamweb requested a review from kenjis August 9, 2023 21:59
@kenjis
Copy link
Member
kenjis commented Aug 10, 2023

We have three action methods.

$ php spark routes | grep MagicLinkController
| GET    | login/magic-link        | magic-link         | \CodeIgniter\Shield\Controllers\MagicLinkController::loginView     |                | toolbar       |
| GET    | login/verify-magic-link | verify-magic-link  | \CodeIgniter\Shield\Controllers\MagicLinkController::verify        |                | toolbar       |
| POST   | login/magic-link        | »                  | \CodeIgniter\Shield\Controllers\MagicLinkController::loginAction   |                | toolbar       |

Can you add the if statement to verify() just in case?

kenjis
kenjis reviewed Aug 10, 2023
View reviewed changes
@datamweb
Copy link
Collaborator Author
datamweb commented Aug 10, 2023

Can you add the if statement to verify() just in case?

Honestly, I thought about this, isn't it better to have a private function?

    private function checkAllowMagicLink( ): RedirectResponse
   { 
        // Check if magic-link is not allowed
        if (!setting('Auth.allowMagicLinkLogins')) {
            return redirect()->route('login')->with('error', lang('Auth.magicLinkDisabled'));
        }
   }

@kenjis
Copy link
Member
kenjis commented Aug 10, 2023

The three if statements are the exactly the same and the same meaning.
So it is better not to repeat them.

@datamweb datamweb mentioned this pull request Aug 11, 2023
Merged
@kenjis kenjis changed the title fix: check allow magiclink logins fix: Magic Link Login can be used even if $allowMagicLinkLogins is false Aug 11, 2023
@datamweb datamweb force-pushed the fix-allowMagicLinkLogins branch from 75a3562 to 6d56e46 Compare August 11, 2023 04:05
kenjis
kenjis reviewed Aug 11, 2023
View reviewed changes
src/Controllers/MagicLinkController.php Outdated
*/
public function loginView()
{
$this->displayErrorMagicLinkDisabled();
Copy link
Member
@kenjis kenjis Aug 11, 2023

Choose a reason for hiding this comment

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

RedirectResponse must be returned from this method if allowMagicLinkLogins is false.

@datamweb
Copy link
Collaborator Author
datamweb commented Aug 12, 2023

@kenjis, method verify(): RedirectResponse uses native return type , using a private method causes breaking change. Is it worth it?
Or am I missing something?

@kenjis
Copy link
Member
kenjis commented Aug 12, 2023

using a private method causes breaking change.

No, it is not a breaking change.

But this PR works, so I merge for now.

@kenjis kenjis merged commit 636684a into codeigniter4:develop Aug 12, 2023
@datamweb datamweb deleted the fix-allowMagicLinkLogins branch August 12, 2023 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenjis kenjis kenjis approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet
-->

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug: controller run even if $allowMagicLinkLogins is false

2 participants

@datamweb @kenjis
0