[1.0.x] Azure OAuth Client SSO

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

Please verify Git has been set to use the same email used for your drupal.org account. Differently, your commits are not associated to your account.

I did not have the time to make a more complete review, but I wanted to report this as first change to be done.

Please see PHP coding standards → and API documentation and comment standards → to understand how Drupal code is expected to be formatted and what documentation comments need to document.

I have updated the development branch as per the provided information. Would please review and check

Fix phpcss issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml azure_oauth_ss
o/

FILE: azure_oauth_sso/azure_oauth_sso.info.yml
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/BaseOAuth.php
--------------------------------------------------------------------------------
FOUND 16 ERRORS AFFECTING 12 LINES
--------------------------------------------------------------------------------
  5 | ERROR | Doc comment is empty
  9 | ERROR | Missing member variable doc comment
 10 | ERROR | Missing member variable doc comment
 11 | ERROR | Missing member variable doc comment
 12 | ERROR | Missing member variable doc comment
 12 | ERROR | Class property $client_id should use lowerCamel naming without underscores
 13 | ERROR | Missing member variable doc comment
 13 | ERROR | Class property $ad_tenant should use lowerCamel naming without underscores
 14 | ERROR | Missing member variable doc comment
 14 | ERROR | Class property $client_secret should use lowerCamel naming without underscores
 15 | ERROR | Missing member variable doc comment
 15 | ERROR | Class property $redirect_uri should use lowerCamel naming without underscores
 17 | ERROR | Doc comment is empty
 25 | ERROR | Doc comment is empty
 33 | ERROR | Doc comment is empty
 41 | ERROR | Doc comment is empty
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/Form/OauthConfigForm.php
--------------------------------------------------------------------------------
FOUND 5 ERRORS AND 8 WARNINGS AFFECTING 13 LINES
--------------------------------------------------------------------------------
  10 | ERROR   | Doc comment is empty
  16 | ERROR   | Doc comment is empty
  23 | ERROR   | Doc comment is empty
  30 | ERROR   | Doc comment is empty
  78 | WARNING | #options values usually have to run through t() for translation
  79 | WARNING | #options values usually have to run through t() for translation
  85 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 111 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 112 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 122 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 134 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 146 | ERROR   | Doc comment is empty
 150 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/Form/OauthFieldsMappingForm.php
--------------------------------------------------------------------------------
FOUND 8 ERRORS AND 3 WARNINGS AFFECTING 10 LINES
--------------------------------------------------------------------------------
   8 | ERROR   | Doc comment is empty
  11 | ERROR   | Class name doesn't match filename; expected "class OauthFieldsMappingForm"
  11 | ERROR   | Class name must begin with a capital letter
  13 | ERROR   | Doc comment is empty
  20 | ERROR   | Doc comment is empty
  27 | ERROR   | Doc comment is empty
  33 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  38 | ERROR   | The array declaration extends to column 389 (the limit is 80). The array content should be split up over multiple lines
 156 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 174 | ERROR   | Doc comment is empty
 178 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/Form/OauthRolesMappingForm.php
--------------------------------------------------------------------------------
FOUND 7 ERRORS AND 6 WARNINGS AFFECTING 12 LINES
--------------------------------------------------------------------------------
   8 | ERROR   | Doc comment is empty
  11 | ERROR   | Class name doesn't match filename; expected "class OauthRolesMappingForm"
  11 | ERROR   | Class name must begin with a capital letter
  13 | ERROR   | Doc comment is empty
  20 | ERROR   | Doc comment is empty
  27 | ERROR   | Doc comment is empty
  42 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  44 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  51 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  54 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  55 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
  58 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 115 | ERROR   | Doc comment is empty
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/Service/OAuthTokenService.php
--------------------------------------------------------------------------------
FOUND 4 ERRORS AND 4 WARNINGS AFFECTING 6 LINES
--------------------------------------------------------------------------------
 10 | ERROR   | Doc comment is empty
 17 | ERROR   | Doc comment is empty
 21 | WARNING | User::load calls should be avoided in classes, use dependency injection instead
 21 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 25 | ERROR   | Doc comment is empty
 31 | WARNING | User::load calls should be avoided in classes, use dependency injection instead
 31 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 53 | ERROR   | Doc comment is empty
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/src/Controller/OauthLogin.php
--------------------------------------------------------------------------------
FOUND 25 ERRORS AND 19 WARNINGS AFFECTING 40 LINES
--------------------------------------------------------------------------------
  15 | ERROR   | Doc comment is empty
  20 | ERROR   | Missing member variable doc comment
  21 | ERROR   | Missing member variable doc comment
  22 | ERROR   | Missing member variable doc comment
  23 | ERROR   | Missing member variable doc comment
  24 | ERROR   | Missing member variable doc comment
  25 | ERROR   | Missing member variable doc comment
  26 | ERROR   | Missing member variable doc comment
  28 | ERROR   | Doc comment is empty
  32 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  33 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  36 | ERROR   | Doc comment is empty
  76 | ERROR   | Doc comment is empty
  80 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("code") instead  
  80 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("error") instead 
  91 | ERROR   | Doc comment is empty
  95 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  96 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("code") instead  
  96 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("error") instead 
 108 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("code") instead  
 109 | ERROR   | The $_GET super global must not be accessed directly; inject the request_stack service and use $stack->getCurrentRequest()->query->get("code") instead  
 124 | ERROR   | Doc comment is empty
 129 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 130 | WARNING | User::loadMultiple calls should be avoided in classes, use dependency injection instead
 221 | ERROR   | Doc comment is empty
 247 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 253 | ERROR   | Doc comment is empty
 266 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 276 | ERROR   | Doc comment is empty
 280 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 289 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 325 | ERROR   | Doc comment is empty
 340 | ERROR   | Doc comment is empty
 345 | WARNING | User::load calls should be avoided in classes, use dependency injection instead
 345 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 351 | WARNING | File::load calls should be avoided in classes, use dependency injection instead
 363 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 367 | ERROR   | Doc comment is empty
 372 | WARNING | User::load calls should be avoided in classes, use dependency injection instead
 372 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 376 | WARNING | File::load calls should be avoided in classes, use dependency injection instead
 383 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 388 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 396 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: azure_oauth_sso/azure_oauth_sso.module
----------------------------------------------------------------------------
FOUND 13 ERRORS AFFECTING 13 LINES
----------------------------------------------------------------------------
   3 | ERROR | Missing short description in doc comment
  15 | ERROR | Doc comment is empty
  19 | ERROR | Doc comment is empty
  26 | ERROR | Doc comment is empty
  30 | ERROR | Doc comment is empty
  52 | ERROR | Doc comment is empty
  56 | ERROR | Doc comment is empty
  70 | ERROR | Doc comment is empty
  74 | ERROR | Doc comment is empty
  90 | ERROR | Doc comment is empty
  94 | ERROR | Doc comment is empty
 104 | ERROR | Doc comment is empty
 108 | ERROR | Doc comment is empty
----------------------------------------------------------------------------

I have updated the development branch. Would you please check the development branch then I can proceed to create new release.

Rest looks fine to me.

Let’s wait for a Code Review Administrator to take a look and if everything goes fine, you will get the role.

Thanks for your contribution!

1. JS files: please add file doc block comments at the top describing what the JS file does and why.
2. azure_oauth_sso_page_attachments(): you are adding your library to every single page on your Drupal site. Is that intentional? Or should it only be added on the login page?
3. azure_oauth_sso_custom_logout(): not every Drupal site uses language parameters in URLs. Are you sure the redirect URL is correct on sites without language in the URL?
4. class OauthLogin: member variables requestStack and oauthService are not defined, please add them.
5. config schema is missing, see https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →
6. OauthLogin::loginUser(): this will not scale. You cannot load all user accounts of a site and then compare email addresses. This will break on sites with more than 10k users. You need to query for the specific email address instead.
7. getMyPhoto(): instead of hard-coding the image path you should get it from the user picture field settings. That way it is stored as if the user would have uploaded a picture. This also helps on sites that use the private file system to store user pictures.

I think the loading of all users on login is currently a blocker, as this would break any large drupal site.

@klausi Thanks a lot for sharing this valuable information!

I completely agree with the comments.

Great news! We've reviewed your comments and addressed them all on the development branch.

Would you mind taking a look?

1. JS files: wow the AI generated comments you added are really bad. They don't explain why the JS file exists and what its purpose is. They just explain badly what the code does without giving a good summary that makes sense to a human. Please only use AI generated text when it makes sense and actually explains something.
2. OauthLogin::$currentUser: doc data type is wrong. Actually all datatypes of all your class variables are wrong, they are not string.

But that are not application blockers.

Thanks for your contribution, Ahmad!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find lots more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

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