Page MenuHome

Blender ID Oauth2 login extension for Phabricator
Needs ReviewPublic

Authored by Frederic (TaOuch) on Mon, Oct 7, 11:39 PM.

Details

Summary
  • Add the Blender ID Auth provider in Phabricator, fill up the field, copy the return URL
  • Add the new OAuth2 application in Blender ID admin, insert the retrun URL given by Phabricator, copy the Client ID and the Client secret
  • Go back to Phabricator and copy your credential into the Blender ID provider settings

Notes
Blender ID accepts only https return URL, therefore Phabricator have to run under https

T69300 BlenderID support in Phabricator

Diff Detail

Repository
rP Phabricator

Event Timeline

Frederic (TaOuch) edited the summary of this revision. (Show Details)

i couldnt commit files inside /src/extensions/ as its inside the .gitignore, so i moved them out

Use git add -f to stage files in the folder which is ignored by .gitignore.

i put back the Blender ID auth files into src/extensions/blender-id/

Sybren A. Stüvel (sybren) requested changes to this revision.Tue, Oct 15, 4:29 PM

The blender-logo.png file doesn't want to show itself here; just wondering if PNG is the best choice here. Is it used in one place with predictable size? Otherwise SVG might be better.

src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php
27

I don't know the code style rules used in this project, but this line seems to be rather long.

118

Again, I'm not sure about the code style rules for this project, but in most projects we prefer full English sentences (so starting with a capital letter, ending with a period, and having the proper punctuation) for comments.

src/extensions/blender-id/PhutilBlenderIDAuthAdapter.php
78

There is no 'Bleder', and Blender doesn't require anything. 'Blender ID' is the proper name.

93

Same disclaimer about code style rules, but generally we end files with a newline. Same for other files (like blender-id.css).

src/extensions/blender-id/README.md
3

The sections below are probably steps to perform in order to use this extension. Explicitly mentioning this is a good idea.

15

I don't think this README should express any preference to the name of the OAuth2 Application.

16

typo

16

If at all predictable, I would add an example here, f.e. https://sitename/oauth2/return or whatever is the url. This makes it easier to find the correct URL to configure.

27

typo

This revision now requires changes to proceed.Tue, Oct 15, 4:29 PM
Frederic (TaOuch) set the repository for this revision to rP Phabricator.

Updates of the code style to fit the rules.

The blender-logo.png is used only on the login page. It has only 2 possible sizes:

  • For non retina displays 28px x 28px
  • For retina displays 56px x 56px

src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php
14

Ok, this is now a full sentence, but it has also lost information. Previously it was clear that the string returned was a CSS class, now that info is lost. Why not just write "CSS class of the Blender icon."?

20

Try to keep such comments about the function. To me this comment reads like "In Paris you can see the Eiffel Tower"; something that can be true, but it's unclear how it relates to the code. "Returns the help text displayed at the Edit Auth Provider section." seems much clearer to me.

76

Same as above. This comment makes a statement about what "we" want, but doesn't relate it to the function. "Adds the Blender ID URI to the Phabricator config array." reads better to me.

@Sybren A. Stüvel (sybren) Thanks you so much for your comments and your patience. It is much appreciated. Sorry that it has be comments on my comments :) I am still adapting myself to such a big open source project like Blender. But if you bare with me, and i am sure i can be useful for Blender as i have a some experience in the web development.

src/extensions/blender-id/PhabricatorBlenderIDAuthProvider.php
27

see here where the text is displayed. It is the same length than other Oauth provider in Phabricator.

118

OK! :)

src/extensions/blender-id/PhutilBlenderIDAuthAdapter.php
93

Ok. I have misunderstood a previous comment from Sergey. All Phabricator class file seems to end with an empty line.

Updates on some comments so its more clear what a function is doing.