Problem/Motivation

This module seems to have a few flaws. This issue is dedicated to document these and potentially create appropriate fixes.

Steps to reproduce

Proposed resolution

Document and fix potential vulnerabilites / issues

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork social_auth_google_one_tap-3563136

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

grevil created an issue. See original summary.

grevil’s picture

grevil commented

Added a few to-dos. The missing CSRF token validation is critical. Everything else is medium to minor.

Additionally, to the comments, we should see that we use the Drupal Coding Standards.

grevil opened merge request !1

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Active » Needs work

Very helpful additions to match standards @grevil thank you very much for your review!
This will also be helpful for the maintainer in #3563058: Opt into Drupal’s security advisory policy.

I'll link this one there.

@stjuan627 are you willing / planning to improve this?

grevil’s picture

grevil commented
Status: Needs work » Needs review

Alright, all done.

There is one part left, which might nor yet work properly. We still need to test this issue's branch MR in the future.

Setting to NR for the rest though.

grevil’s picture

grevil commented

And also we don't need any csrf token. Google will provide the authentication through its php sdk which is required by this module, and csrf tokens are usually not used for log in flows.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

csrf tokens are usually not used for log in flows

That should be checked, when proceeding, I'm not really sure if that's correct... Should be verified then.

grevil’s picture

grevil commented
Title: Module assesement » Module assesement and best practice "overhaul"
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 1.0.x-dev » 2.x-dev
Status: Needs review » Needs work
grevil’s picture

grevil commented
Status: Needs work » Needs review

Ok, that should be it, please review!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Thanks @grevil great work! I left some comments.

We should also test this carefully, as the module doesn't have tests. #3565649: Write (more) tests

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil
grevil’s picture

grevil commented
Status: Needs work » Needs review

Back to NR.

grevil’s picture

grevil commented
Assigned: grevil » Unassigned
Status: Needs review » Reviewed & tested by the community

grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@grevil I think we should leave 1.0.x as-is and only merge this into 2.x!

  • anybody committed 48712403 on 1.0.x 
    Revert "feat: #3563136 Module assesement"

This reverts commit...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Done now and reverted with 1.0.0-rc6 (identical to 1.0.0-rc4)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.