Move and Split include/security #5946

theatischbein · 2018-10-17T16:55:57Z

To issue #3878 I did

split up include/security.php as proposed in the ethercalc into
- /src/Core/Authentication.php
- /src/Util/Security.php
rename function Authentication::authenticate_success to Authentication::success
remove old calls of require_onces(security.php) (sometimes the file wasn't even needed)
replace in Authentication.php old System::baseUrl() with $a = self::getApp() and $a->getBaseUrl()

Strange thing was:

in some file there were functions from former /include/security.php but no require_onces(security.php)
Are they included by some other use Friendica\... call ?
Now in those classes I add the proper use call

Also needed PR in addon friendica/friendica-addons#756

…n::success

MrPetovan · 2018-10-17T17:03:41Z

Under the previous require system, some common libraries (database, security, etc...) would end up systematically included, which means that a require call in certain files wouldn't be needed in order to have the functions accessible. Of course this was a terrible practice and I'm glad you're participating to the effort or moving to an explicit autoload system.

MrPetovan

Thanks for your work, I have several requests, mainly about names/locations.

MrPetovan · 2018-10-17T17:49:26Z

src/Core/Authentication.php

+	 *
+	 * @return string Hashed data
+	 */
+	public static function cookie_hash($user)


When you move global functions to a class, please rename the methods to camelCase. You can also add details on the method behavior.

For example, I would rename this method to getCookieHashForUser since it returns data (get- prefix) and has a mandatory User record parameter (-ForUser suffix).

MrPetovan · 2018-10-17T17:50:01Z

src/Core/Authentication.php

+	 * @param int $time
+	 * @param array $user Record from "user" table
+	 */
+	public static  function new_cookie($time, $user = [])


Please rename this method to setCookie.

MrPetovan · 2018-10-17T17:51:04Z

src/Core/Authentication.php

+	 * @param type $interactive
+	 * @param type $login_refresh
+	 */
+	public static function success($user_record, $login_initial = false, $interactive = false, $login_refresh = false)


Please rename this method to setAuthenticatedSessionForUser.

MrPetovan · 2018-10-17T17:52:33Z

src/Core/Authentication.php

+	/**
+	 * @brief Kills the "Friendica" cookie and all session data
+	 */
+	public static function nuke_session()


Please rename this method to deleteSession.

MrPetovan · 2018-10-17T17:54:02Z

src/Core/Authentication.php

+			Addon::callHooks('logged_in', $a->user);
+
+			if (($a->module !== 'home') && isset($_SESSION['return_url'])) {
+				goaway($a->getbaseUrl() . '/' . $_SESSION['return_url']);


This probably should be the object of a future PR, but please keep in mind in your refactoring that goaway() or any other HTTP redirection function/method should only be called from within a module, in this case the Login module.

MrPetovan · 2018-10-17T18:05:50Z

src/Util/Security.php

+
+	public static function check_form_security_token_ForbiddenOnErr($typename = '', $formname = 'form_security_token')
+	{
+		if (!check_form_security_token($typename, $formname)) {


Watch out for this lone call to a non-existent function once you removed include/security.php.

MrPetovan · 2018-10-17T18:06:11Z

src/Util/Security.php

+	}
+}
+
+?>


Please remove the closing PHP tag and leave a single new line at the end of the file.

MrPetovan · 2018-10-17T18:08:01Z

src/Core/Authentication.php

+		if ($user) {
+			$value = json_encode(["uid" => $user["uid"],
+				"hash" => self::cookie_hash($user),
+				"ip" => defaults($_SERVER, 'REMOTE_ADDR', '0.0.0.0')]);


Watch out for global functions, their corresponding file should be require_once by this file until we move them to classes.

src/Core/Authentication.php

MrPetovan · 2018-10-17T18:08:30Z

src/Core/Authentication.php

+		}
+
+		if ($login_initial) {
+			logger('auth_identities: ' . print_r($a->identities, true), LOGGER_DEBUG);


Watch out for global functions, their corresponding file should be require_once by this file until we move them to classes.

rabuzarus · 2018-10-17T19:01:55Z

src/Util/Security.php

+		$local_user = local_user();
+		$remote_user = remote_user();
+
+		/**


/**is used to indicate doxygen documentation. Please use /* for inline comments

rabuzarus · 2018-10-17T19:02:08Z

src/Util/Security.php

+				 AND deny_gid  = ''
+		";
+
+		/**


/**is used to indicate doxygen documentation. Please use /* for inline comments

rabuzarus · 2018-10-17T19:02:21Z

src/Util/Security.php

+		 */
+		if ($local_user && $local_user == $owner_id) {
+			$sql = '';
+		/**


/**is used to indicate doxygen documentation. Please use /* for inline comments

… BaseModule + fix multiline comments

MrPetovan · 2018-10-17T19:42:45Z

I'm sorry you have some conflicts to solve after the last merge.

…nflict with develop

MrPetovan · 2018-10-17T19:58:33Z

Have you checked for any remaining instances of the global function names you removed in the whole repo (vendor/ excluded)?

theatischbein · 2018-10-17T20:03:13Z

Do you mean the function names from include/security.php with global function names ?

MrPetovan · 2018-10-17T20:05:40Z

Yes I do. There were a couple remaining when you first submitted the PR, so I just want to make sure you double-checked everywhere. Watch out for function references as callbacks, some IDEs (like NetBeans) will miss those function references because it's a simple string.

theatischbein · 2018-10-17T20:36:57Z

I had a typo and not all files were changed by refactoring.
Now I checked for all old function names. There are only left in comments or logs.
Edit: As I write this, I think I should change the logs to...

MrPetovan · 2018-10-17T22:20:54Z

Please update comments and logging strings as well.

MrPetovan · 2018-10-19T12:55:19Z

Thank you for your work once again!

annando · 2018-10-19T19:07:26Z

@jonnytischbein There's a problem:

PHP Warning: require_once(include/security.php): failed to open stream: No such file or directory in /path/to/friendica/mod/pubsub.php on line 9

MrPetovan · 2018-10-19T19:09:33Z

I'm on it.

annando · 2018-10-19T19:10:26Z

Please have a look at src/Protocol/DFRN.php as well.

MrPetovan · 2018-10-19T19:11:18Z

Yup.

tobiasd · 2018-10-20T05:04:38Z

This seems to have broken my system. The system chokes and throws an error 500; so far I could not find anything in the logs.

theatischbein · 2018-10-20T06:36:36Z

At which page do you get this error? Or at any page?
Do you know which addons you have activated? Because there were changes too. (Did you pull in addon?)

I can't reproduce am error 500 at current develop.

tobiasd · 2018-10-20T09:17:57Z

Addons is a good hint yes. Maybe it is the projects addon I'll have a look.

tobiasd · 2018-10-20T09:48:12Z

Jupp pinging @fabrixxm so he can check ;-)

MrPetovan · 2018-10-20T12:49:34Z

There were a couple of addons that required the include/security.php file, have you updated them?

theatischbein · 2018-10-20T13:10:12Z

Yes in friendica/friendica-addons#756

But since I missed some in the base repo, I'm not trusting my methods with grep and IDE :/
On my branch that you merge shouldn't be any require_ones. (I checked just now) They only were used in windowsphonepush.php and advancedcontentfilter.php

MrPetovan · 2018-10-20T13:13:32Z

I can confirm there aren't any more instances of security.php in addon, but maybe @tobiasd didn't pull the addon repo.

MrPetovan · 2018-10-21T12:18:21Z

Related discussion: https://forum.friendi.ca/display/f3ad7b1c-155b-cab3-e4d8-7ab744885399

theatischbein · 2018-10-21T13:48:20Z

Seems to be an addon projects which isn't included in the addon repo.

tobiasd · 2018-10-21T14:26:36Z

Sorry for the tone of my comment @jonnytischbein I really appreciate your efforts of support in that issue. Since the projects addon is not official you don't need to worry about it not being updated with the rest :-)

@MrPetovan you as well sorry!

MrPetovan · 2018-10-21T18:51:23Z

It's all good!

theatischbein added 3 commits October 17, 2018 14:19

Move include/security tp /src/Core/Authentication and /src/Util/Security

05be2db

remove unsed require_onces of security.php

45e5a7f

Rename function Authentication::authenticate_success to Authenticatio…

ce2b1f5

…n::success

theatischbein mentioned this pull request Oct 17, 2018

Move include/security to /src/Core/Authentication.php and /src/Util/Security.php friendica/friendica-addons#756

Merged

MrPetovan requested changes Oct 17, 2018

View reviewed changes

MrPetovan added the Enhancement label Oct 17, 2018

MrPetovan added this to the 2018.12 milestone Oct 17, 2018

rabuzarus reviewed Oct 17, 2018

View reviewed changes

Renaming functions + moving functions from security to Model/Item and…

f3fc1f3

… BaseModule + fix multiline comments

MrPetovan approved these changes Oct 17, 2018

View reviewed changes

Remove unneeded Util/Security includings + Resolve including merge co…

0c0e17b

…nflict with develop

MrPetovan approved these changes Oct 17, 2018

View reviewed changes

add remaning/moving to files not touch by refactoring because of typo

a937cf0

MrPetovan approved these changes Oct 17, 2018

View reviewed changes

update function names in logger and comments

cafd5ca

MrPetovan approved these changes Oct 19, 2018

View reviewed changes

MrPetovan merged commit ec0d3a6 into friendica:develop Oct 19, 2018

This was referenced Oct 19, 2018

Remove last references to include/security.php #5972

Merged

Update Addon documentation about location of logged_in hook #5973

Merged

Repository owner deleted a comment from tobiasd Oct 21, 2018

annando mentioned this pull request Oct 29, 2018

Move global functions from include/ to class methods in src/ #3878

Closed

7 tasks

+              	}
+              }
+              ?>

Move and Split include/security #5946

Move and Split include/security #5946

Conversation

theatischbein commented Oct 17, 2018 • edited

MrPetovan commented Oct 17, 2018 • edited

MrPetovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrPetovan commented Oct 17, 2018

MrPetovan commented Oct 17, 2018

theatischbein commented Oct 17, 2018

MrPetovan commented Oct 17, 2018

theatischbein commented Oct 17, 2018 • edited

MrPetovan commented Oct 17, 2018

MrPetovan commented Oct 19, 2018

annando commented Oct 19, 2018

MrPetovan commented Oct 19, 2018

annando commented Oct 19, 2018

MrPetovan commented Oct 19, 2018

tobiasd commented Oct 20, 2018

theatischbein commented Oct 20, 2018 • edited

tobiasd commented Oct 20, 2018 • edited

tobiasd commented Oct 20, 2018

MrPetovan commented Oct 20, 2018

theatischbein commented Oct 20, 2018 • edited

MrPetovan commented Oct 20, 2018

MrPetovan commented Oct 21, 2018

theatischbein commented Oct 21, 2018 • edited

tobiasd commented Oct 21, 2018

MrPetovan commented Oct 21, 2018

theatischbein commented Oct 17, 2018 •

edited

MrPetovan commented Oct 17, 2018 •

edited

theatischbein commented Oct 17, 2018 •

edited

theatischbein commented Oct 20, 2018 •

edited

tobiasd commented Oct 20, 2018 •

edited

theatischbein commented Oct 20, 2018 •

edited

theatischbein commented Oct 21, 2018 •

edited