[MonologBridge] Add TokenProcessor #21086

maidmaid · Dec 28, 2016

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#8156

TokenProcessor is an out-of-the-box class that adds token in extra info log, like this:

[2016-12-29 14:48:09] app.INFO: admin connected! [] {"token":{
    "username":"admin",
    "authenticated":true,
    "roles":["ROLE_ADMIN"]}
}

maidmaid · Dec 28, 2016

If this feature will be merged, is it a good idea to register this processor in MonologBundle for all logs? Or else, maybe simply write a doc that explains its registration (containing something like this)?

linaori · Dec 29, 2016

Would this add the token to every log line? Because I don't want to see the token added 10x if my log contains 10 log lines to be honest.

The idea is great though, I'm using a custom formatter for the logging at the moment and I rather have this in the core.

lyrixx · Dec 29, 2016

@iltar Yes it will. That's how processor works.

But:

This processor is not enabled by default
You can add process on specific channel and/or handler.

maidmaid · Dec 29, 2016

I have improved token normalization that now is more json-like/readable:

[2016-12-29 14:48:09] app.INFO: admin connected! [] {"token":{
    "username":"admin",
    "auth.":true,
    "roles":["ROLE_ADMIN"]}
}

linaori · Dec 29, 2016

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

-            $records['extra']['token'] = (string) $token;
+            $records['extra']['token'] = array(
+                'username' => $token->getUsername(),
+                'auth.' => $token->isAuthenticated(),


I suggest to call this key authenticated

fabpot · Dec 30, 2016

src/Symfony/Bridge/Monolog/Processor/DebugProcessor.php

@@ -25,6 +25,7 @@ public function __invoke(array $record)
            'timestamp' => $record['datetime']->getTimestamp(),
            'message' => $record['message'],
            'priority' => $record['level'],
+


should be reverted

fabpot · Dec 30, 2016

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

+                'auth.' => $token->isAuthenticated(),
+                'roles' => array_map(function (Role $role) {
+                    return $role->getRole();
+                }, $token->getRoles()),


should be on one line

maidmaid · Dec 30, 2016

Thank you for your review @fabpot and @iltar. Changes are done.

xabbuh · Dec 30, 2016

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

+use Symfony\Component\Security\Core\Role\Role;
+
+/**
+ * TokenProcessor adds token extra info.


"Adds the current security token to the log entry."?

Indeed, it's better.

xabbuh · Dec 30, 2016

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

+        return $records;
+    }
+
+    public function isDeniedProcess()


Is this method really needed?

In CLI mode, extra.token should not be added, for example in commands but not in tests. This method allows to mock this behavior in tests.

In CLI mode, extra.token should not be added

Why not?

I was more referring to this being a dedicated method while it could easily be inlined.

Furthermore, a method being public is an extension point which we would have to maintain.

Indeed, this is not very helpful. I have removed this method.

xabbuh · Jan 10, 2017

You need to add the security/core package as a dependency to the require-dev section in the composer.json file of MonologBridge.

ro0NL · Jan 10, 2017

src/Symfony/Bridge/Monolog/Processor/TokenProcessor.php

+            $records['extra']['token'] = array(
+                'username' => $token->getUsername(),
+                'authenticated' => $token->isAuthenticated(),
+                'roles' => array_map(function (Role $role) { return $role->getRole(); }, $token->getRoles()),


Should be RoleInterface

RoleInterface is now deprecated

And not yet removed, they are still returned from getRoles.

👍 it's done

maidmaid · Jan 10, 2017

I updated the composer.json @xabbuh.

maidmaid · Apr 3, 2017

AppVeyor break tests are related?

sstok · Apr 7, 2017

AppVeyor Failure seems unrelated.

maidmaid · Jul 12, 2017

PR ready to merge since some time :)

lyrixx · Jul 12, 2017

👍

nicolas-grekas · Jul 12, 2017

src/Symfony/Bridge/Monolog/composer.json

@@ -23,6 +23,7 @@
    "require-dev": {
        "symfony/console": "~2.8|~3.0|~4.0",
        "symfony/event-dispatcher": "~2.8|~3.0|~4.0",
+        "symfony/security-core": "~2.8|~3.0",


missing |~4.0

RoleInterface is used here, and will be removed in 4.0.

then I suggest to remove the type hint, it's purely internal and doesn't justify a deps limitation

nicolas-grekas · Jul 12, 2017

Thank you @maidmaid.

This PR was merged into the 3.4 branch. Discussion ---------- [MonologBridge] Add TokenProcessor | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a ``TokenProcessor`` is an out-of-the-box class that adds token in extra info log, like this: ``` [2016-12-29 14:48:09] app.INFO: admin connected! [] {"token":{ "username":"admin", "authenticated":true, "roles":["ROLE_ADMIN"]} } ``` Commits ------- 3763515 Add TokenProcessor

carsonbot added Status: Needs Review MonologBridge Feature labels Dec 28, 2016

linaori reviewed Dec 29, 2016

View reviewed changes

fabpot reviewed Dec 30, 2016

View reviewed changes

xabbuh reviewed Dec 30, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Jan 2, 2017

ro0NL reviewed Jan 10, 2017

View reviewed changes

maidmaid force-pushed the userprocessor branch from 8975ddd to 76ac3fd Compare March 23, 2017 12:49

maidmaid force-pushed the userprocessor branch from 2405d84 to c55e321 Compare April 3, 2017 08:23

maidmaid force-pushed the userprocessor branch from 2b4f012 to 6f74a75 Compare April 12, 2017 11:44

maidmaid force-pushed the userprocessor branch from 6f74a75 to b4af6dc Compare May 20, 2017 06:42

maidmaid changed the base branch from master to 3.4 May 20, 2017 06:42

maidmaid force-pushed the userprocessor branch 4 times, most recently from ac46318 to ef3e18c Compare June 5, 2017 22:43

maidmaid force-pushed the userprocessor branch 2 times, most recently from 9210609 to 26b9d1f Compare July 1, 2017 09:16

maidmaid force-pushed the userprocessor branch from 26b9d1f to 2427f5c Compare July 3, 2017 11:58

maidmaid force-pushed the userprocessor branch from 2427f5c to a0d2926 Compare July 10, 2017 22:36

Simperfit approved these changes Jul 12, 2017

View reviewed changes

nicolas-grekas reviewed Jul 12, 2017

View reviewed changes

maidmaid force-pushed the userprocessor branch from a0d2926 to beb7624 Compare July 12, 2017 12:55

Add TokenProcessor

3763515

maidmaid force-pushed the userprocessor branch from beb7624 to 3763515 Compare July 12, 2017 13:09

nicolas-grekas merged commit 3763515 into symfony:3.4 Jul 12, 2017

maidmaid deleted the userprocessor branch July 12, 2017 13:12

maidmaid mentioned this pull request Jul 16, 2017

Add TokenProcessor symfony/symfony-docs#8156

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[MonologBridge] Add TokenProcessor #21086

[MonologBridge] Add TokenProcessor #21086

Uh oh!

Conversation

maidmaid commented Dec 28, 2016 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maidmaid commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori commented Dec 29, 2016

Uh oh!

lyrixx commented Dec 29, 2016

Uh oh!

maidmaid commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid commented Dec 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid commented Jan 10, 2017

Uh oh!

maidmaid commented Apr 3, 2017

Uh oh!

sstok commented Apr 7, 2017

Uh oh!

maidmaid commented Jul 12, 2017

Uh oh!

lyrixx commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 12, 2017

Uh oh!

Uh oh!

maidmaid commented Dec 28, 2016 •

edited by nicolas-grekas

Loading

maidmaid commented Dec 28, 2016 •

edited

Loading

nicolas-grekas Jul 12, 2017 •

edited

Loading