This module integrates with the Clef service, letting users log in with their Android or iOS smartphone. Clef is a smartphone app that lets you "forget your passwords" and log into sites as easily as taking a photo.

The Clef module adds a button to Drupal login forms. If you've already logged into Clef, pressing that button logs you into Drupal automatically. If not, you'll be asked to scan a QR code with the Clef app on your phone.

Drupal users are matched to their Clef identities by email address, so existing users can take advantage of Clef right away. Future versions of the Clef module will support user registration using the Clef app.

The current project page can be found in my sandbox:
http://drupal.org/sandbox/DaveRoss/1923862

The current repository is located at:
git clone --recursive --branch 7.x-1.x git.drupal.org:sandbox/DaveRoss/1923862.git clef

This is a Drupal 7.x project, with plans to port to 8.x as development proceeds.

Reviews of other projects

http://drupal.org/node/1922796#comment-7103118
http://drupal.org/node/1913916#comment-7103212
http://drupal.org/node/1852392#comment-7103386

Comments

beljaako’s picture

Status:Needs work» Needs review

- There are several drupal coding standards issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxdaveross1923862git
- The version in the info file should be something like: 7.x-1.0. Not: 1.0.
- There is no validation on the admin form.
- There are some notices you need to fix, last two appear when json_decode fails.

Notice: Undefined index: code in clef_login_callback() (67 clef.module).
Notice: Undefined property: stdClass::$access_token in clef_login_callback() (79 clef.module).
Notice: Undefined property: stdClass::$success in clef_login_callback() (81 clef.module).

davidmac’s picture

Great idea for a module.

On first review, I'd suggest examing the output of the automated test provided by the 'pareview' script, which is detailed below. You'll find the relevant guides to the coding standards here:

http://drupal.org/coding-standards

FILE: /usr/local/bin/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 9 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
14 | WARNING | Line exceeds 80 characters; contains 215 characters
16 | WARNING | Line exceeds 80 characters; contains 220 characters
18 | WARNING | Line exceeds 80 characters; contains 210 characters
28 | WARNING | Line exceeds 80 characters; contains 110 characters
30 | WARNING | Line exceeds 80 characters; contains 295 characters
32 | WARNING | Line exceeds 80 characters; contains 120 characters
34 | WARNING | Line exceeds 80 characters; contains 200 characters
39 | WARNING | Line exceeds 80 characters; contains 287 characters
41 | WARNING | Line exceeds 80 characters; contains 234 characters
--------------------------------------------------------------------------------

FILE: /usr/local/bin/pareview_temp/clef.admin.inc
--------------------------------------------------------------------------------
FOUND 20 ERROR(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
  9 | ERROR | Function comment short description must end with a full stop
11 | ERROR | There should be no white space after an opening "("
11 | ERROR | Expected 0 spaces between opening bracket and argument "$form"; 1
    |       | found
11 | ERROR | There should be no white space before a closing ")"
11 | ERROR | Expected 0 spaces between argument "$form_state" and closing
    |       | bracket; 1 found
15 | ERROR | There should be no white space after an opening "("
15 | ERROR | There should be no white space before a closing ")"
16 | ERROR | There should be no white space after an opening "("
16 | ERROR | There should be no white space before a closing ")"
19 | ERROR | There should be no white space after an opening "("
19 | ERROR | There should be no white space before a closing ")"
25 | ERROR | There should be no white space after an opening "("
25 | ERROR | There should be no white space before a closing ")"
26 | ERROR | There should be no white space after an opening "("
26 | ERROR | There should be no white space before a closing ")"
29 | ERROR | There should be no white space after an opening "("
29 | ERROR | There should be no white space before a closing ")"
33 | ERROR | There should be no white space after an opening "("
33 | ERROR | There should be no white space before a closing ")"
34 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: /usr/local/bin/pareview_temp/clef.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /usr/local/bin/pareview_temp/clef.module
--------------------------------------------------------------------------------
FOUND 110 ERROR(S) AND 7 WARNING(S) AFFECTING 56 LINE(S)
--------------------------------------------------------------------------------
   8 | ERROR   | There should be no white space after an opening "("
   8 | ERROR   | There should be no white space before a closing ")"
   9 | ERROR   | There should be no white space after an opening "("
   9 | ERROR   | There should be no white space before a closing ")"
  12 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  12 | ERROR   | Function comment short description must be on a single line
  12 | ERROR   | Function comment short description must end with a full stop
  15 | ERROR   | There should be no white space after an opening "("
  15 | ERROR   | Expected 0 spaces between opening bracket and argument
     |         | "$form"; 1 found
  15 | ERROR   | There should be no white space before a closing ")"
  15 | ERROR   | Expected 0 spaces between argument "$form_id" and closing
     |         | bracket; 1 found
  16 | ERROR   | There should be no white space after an opening "("
  16 | ERROR   | There should be no white space before a closing ")"
  20 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  20 | ERROR   | Function comment short description must be on a single line
  20 | ERROR   | Function comment short description must end with a full stop
  23 | ERROR   | There should be no white space after an opening "("
  23 | ERROR   | Expected 0 spaces between opening bracket and argument
     |         | "$form"; 1 found
  23 | ERROR   | There should be no white space before a closing ")"
  23 | ERROR   | Expected 0 spaces between argument "$form_id" and closing
     |         | bracket; 1 found
  24 | ERROR   | There should be no white space after an opening "("
  24 | ERROR   | There should be no white space before a closing ")"
  28 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  28 | ERROR   | Function comment short description must end with a full stop
  34 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  37 | ERROR   | There should be no white space after an opening "("
  37 | ERROR   | There should be no white space before a closing ")"
  42 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
  47 | ERROR   | There should be no white space after an opening "("
  47 | ERROR   | There should be no white space before a closing ")"
  48 | ERROR   | There should be no white space after an opening "("
  48 | ERROR   | There should be no white space before a closing ")"
  57 | ERROR   | Function comment short description must be on a single line
  57 | ERROR   | Function comment short description must end with a full stop
  62 | ERROR   | No space before comment text; expected "//
     |         | /////////////////////////////" but found
     |         | "///////////////////////////////"
  64 | WARNING | There must be no blank line following an inline comment
  64 | ERROR   | No space before comment text; expected "//
     |         | /////////////////////////////" but found
     |         | "///////////////////////////////"
  68 | ERROR   | There should be no white space after an opening "("
  68 | ERROR   | There should be no white space before a closing ")"
  69 | ERROR   | There should be no white space after an opening "("
  69 | ERROR   | There should be no white space before a closing ")"
  72 | ERROR   | There should be no white space after an opening "("
  74 | ERROR   | There should be no white space after an opening "("
  74 | ERROR   | There should be no white space before a closing ")"
  75 | ERROR   | There should be no white space after an opening "("
  75 | ERROR   | There should be no white space before a closing ")"
  75 | WARNING | A comma should follow the last multiline array item. Found: )
  76 | ERROR   | There should be no white space before a closing ")"
  78 | ERROR   | There should be no white space after an opening "("
  78 | ERROR   | There should be no white space before a closing ")"
  83 | ERROR   | There should be no white space after an opening "("
  83 | ERROR   | There should be no white space before a closing ")"
  84 | ERROR   | There should be no white space after an opening "("
  84 | ERROR   | There should be no white space after an opening "("
  84 | ERROR   | There should be no white space before a closing ")"
  84 | ERROR   | There should be no white space before a closing ")"
  85 | ERROR   | There should be no white space after an opening "("
  85 | ERROR   | There should be no white space before a closing ")"
  88 | ERROR   | No space before comment text; expected "//
     |         | /////////////////////////" but found
     |         | "///////////////////////////"
  90 | WARNING | There must be no blank line following an inline comment
  90 | ERROR   | No space before comment text; expected "//
     |         | /////////////////////////" but found
     |         | "///////////////////////////"
  94 | ERROR   | There should be no white space after an opening "("
  94 | ERROR   | There should be no white space after an opening "("
  94 | ERROR   | There should be no white space before a closing ")"
  94 | ERROR   | There should be no white space before a closing ")"
  96 | ERROR   | There should be no white space after an opening "("
  96 | ERROR   | There should be no white space before a closing ")"
  97 | ERROR   | There should be no white space after an opening "("
  97 | ERROR   | There should be no white space before a closing ")"
100 | ERROR   | There should be no white space after an opening "("
100 | ERROR   | There should be no white space before a closing ")"
101 | ERROR   | There should be no white space after an opening "("
101 | ERROR   | There should be no white space after an opening "("
101 | ERROR   | There should be no white space before a closing ")"
101 | ERROR   | There should be no white space before a closing ")"
102 | ERROR   | There should be no white space after an opening "("
102 | ERROR   | There should be no white space before a closing ")"
109 | ERROR   | There should be no white space after an opening "("
109 | ERROR   | There should be no white space before a closing ")"
110 | ERROR   | There should be no white space after an opening "("
110 | ERROR   | There should be no white space before a closing ")"
112 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
113 | ERROR   | There should be no white space after an opening "("
113 | ERROR   | There should be no white space after an opening "("
113 | ERROR   | There should be no white space before a closing ")"
113 | ERROR   | There should be no white space before a closing ")"
114 | ERROR   | There should be no white space after an opening "("
114 | ERROR   | There should be no white space before a closing ")"
117 | ERROR   | There should be no white space after an opening "("
117 | ERROR   | There should be no white space before a closing ")"
118 | ERROR   | There should be no white space after an opening "("
118 | ERROR   | There should be no white space before a closing ")"
119 | ERROR   | There should be no white space after an opening "("
119 | ERROR   | There should be no white space before a closing ")"
124 | ERROR   | Function comment short description must end with a full stop
125 | ERROR   | There must be an empty line before the parameter block
125 | ERROR   | Parameter comment must be on the next line at position 1
127 | ERROR   | There should be no white space after an opening "("
127 | ERROR   | Expected 0 spaces between opening bracket and argument
     |         | "$form"; 1 found
127 | ERROR   | There should be no white space before a closing ")"
127 | ERROR   | Expected 0 spaces between argument "$form" and closing
     |         | bracket; 1 found
131 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
132 | ERROR   | There should be no white space after an opening "("
132 | ERROR   | There should be no white space before a closing ")"
133 | ERROR   | Expected "if (...) {\n"; found "if(...) {\n"
137 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
139 | ERROR   | There should be no white space after an opening "("
139 | ERROR   | There should be no white space before a closing ")"
150 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
150 | ERROR   | Function comment short description must be on a single line
150 | ERROR   | Function comment short description must end with a full stop
153 | ERROR   | clef_uninstall() is an installation hook and must be declared
     |         | in an install file
155 | ERROR   | There should be no white space after an opening "("
155 | ERROR   | There should be no white space before a closing ")"
156 | ERROR   | There should be no white space after an opening "("
156 | ERROR   | There should be no white space before a closing ")"
158 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /usr/local/bin/pareview_temp/login_script.tpl.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
3 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

davidmac’s picture

Status:Needs review» Needs work

see above

DaveRoss’s picture

Status:Needs review» Needs work

@beljaako - great feedback. Added some defensive coding for those situations when invalid data comes across. Actually, doesn't the build script automatically add the "version" tag when the tarball is built? Took it out for now, but good catch.

@David Mc Donnell - Coding standards, ugh! Oh, well. Guess I'd better stick to Drupal's if I'm submitting Drupal code, right? Ran the code through PHP Code_Sniff with the Drupal standards & then through pareview. Comes out clean on both now.

DaveRoss’s picture

@beljaako - also added some validation to the admin form

davidmac’s picture

Once you've made changes, you need to set the status of your issue back to 'needs review' if you want it to be picked up again.

You might like to read this post on getting your review done ('pareview bonus'): http://drupal.org/node/1410826

DaveRoss’s picture

Status:Needs work» Needs review
DaveRoss’s picture

Issue summary:View changes

Removed username from the git clone command

DaveRoss’s picture

Happy to help out.

nsuit’s picture

This module worked great and is a great idea. I didn't see any coding issues, but you could maybe do a better job in linking the clef.io website and in particular the developer.clef.io subdomain with the application id and secret form. I think there is also an description missing with that form, of where to go on the clef.io site. I had to click around on your website until I finally found the Application registration under the Developer page. It is also a bit confusing that you called it application. I keep on thinking about the Facebook applications when I hear that.

DaveRoss’s picture

It's not my site. I just like what they're doing. But, yeah, the developer site can be pretty confusing.

I had to do an undergrad thesis in college and did it on online identity, back when Kim Cameron was pushing the idea of an "identity metasystem". So I have a thing for neat new alternatives to usernames & passwords.

I'll see what I can do about making the process clearer from my end. I know Drupal admin screens tend not to be as text-heavy as the WordPress ones I usually build, but maybe I can include some brief instructions and a couple links.

Thanks for the feedback & glad you enjoyed it!

davidmac’s picture

Just a small point on the access argument in hook_menu('admin/config/services/clef'). Setting this to 'access administration pages' is understandable, but a lot of developers allow clients to use the admin menu for accessing their own content/dashboard etc. Given that this page falls firmly under a developer role, it's permissions could be tighter.

So, as a suggestion, a more granular option from the role_permission table would be perhaps 'administer site configuration'. Alternatively, you might consider including a hook_permission to address this module specifically.

From a personal point of view, I'd normally like to see hook_help used for some sort of guidance however brief. The README.txt is normally looked at by developers, but a less experienced user might benefit from something on the help page.

klausi’s picture

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus

manual review:

  1. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
  2. clef_button(): do not use login_script.tpl.php like that, use the theme() function instead. Use hook_theme() to register your template. That way themes can easily override the markup.
  3. clef_login_callback(): why do you write to $_SESSION here? It is not used anywhere else? Please add a comment.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

PA robot’s picture

Status:Needs work» Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

PA robot’s picture

Issue summary:View changes

Added "Reviews of other projects"