Skip to content
Snippets Groups Projects

Resolve #2957273 "Jwt log in"

Merged Resolve #2957273 "Jwt log in"
issue/jwt-2957273:2957273-jwt-log-in into 2.x
3 unresolved threads
Merged Alex Pott requested to merge issue/jwt-2957273:2957273-jwt-log-in into 2.x
3 unresolved threads

Related to #2957273

Merge request reports

Checking pipeline status.

Approval is optional

Merged by Peter WolaninPeter Wolanin 1 year ago (Mar 15, 2024 3:14pm UTC)

Merge details

  • Changes merged into 2.x with b9fec633 (commits were squashed).
  • Did not delete the source branch.

Pipeline #120253 passed

Stage: build
Stage: validate
Stage: test

Pipeline passed for b9fec633 on 2.x

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
modules/jwt_auth_issuer/config/install/jwt_auth_issuer.config.yml 0 → 100644
1 jwt_in_login_response: true
  • Peter Wolanin
    Peter Wolanin @pwolanin started a thread on the diff
    • modules/jwt_auth_issuer/src/EventSubscriber/JwtLoginSubscriber.php 0 → 100644
    37 $this->routeBuilder = $routeBuilder;
    38 }
    39
    40 /**
    41 * {@inheritdoc}
    42 */
    43 public function onAlterRoutes(RouteBuildEvent $event) {
    44 // Disabled by configuration.
    45 if (!$this->configFactory->get('jwt_auth_issuer.config')->get('jwt_in_login_response')) {
    46 return;
    47 }
    48
    49 $collection = $event->getRouteCollection();
    50 $route = $collection->get('user.login.http');
    51 if ($route) {
    52 $route->addDefaults(['_controller' => '\Drupal\jwt_auth_issuer\Controller\JwtAuthIssuerLoginController::login']);
    • Peter Wolanin
      Peter Wolanin @pwolanin
      Maintainer

      Instead of replacing the controller on the existing route, can we add a second route with a requirement that the format is JSON? A check for query string "_format" doesn't seem that robust?

      Curious also this doesn't just subclass the existing controller? I guess decorating it is ok too

    • A
      Alex Pott @alexpott
      Author Contributor

      The route we're decorating is also specific to JSON - it actually get more added in \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber... I think we should change the code to work for all serializers - and open a core issue to add a way to alter what's returned without having to re-serialize.

    • Please register or sign in to reply
  • Alex Pott added 2 commits

    added 2 commits

    • 54d15c72 - Support all the formats
    • 8ae2d559 - Use API requests instead of drupalGet() as that has stateful request headers

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    • 9a5fa860 - Add test coverage for different serialization formats

    Compare with previous version

  • Peter Wolanin
    Peter Wolanin @pwolanin started a thread on the diff
    • modules/jwt_auth_issuer/src/Controller/JwtAuthIssuerLoginController.php 0 → 100644
    78 */
    79 public function login(Request $request): Response {
    80 $response = $this->userController->login($request);
    81
    82 // Ensure not error response.
    83 if ($response->getStatusCode() !== 200) {
    84 return $response;
    85 }
    86
    87 // Note that the call to UserAuthenticationController::login() will have
    88 // already validated that the format is supported.
    89 $format = $request->getRequestFormat();
    90
    91 // Decode and add JWT token.
    92 if ($content = $response->getContent()) {
    93 if ($decoded = $this->serializer->decode($content, $format)) {
    • Peter Wolanin
      Peter Wolanin @pwolanin
      Maintainer

      This looks like an argument against composition vs subclassing since now I have to make sure the logic about constructing the serializer and its options stays in sync, plus with the core alter hook issue we hope this class goes away.

    • A
      Alex Pott @alexpott
      Author Contributor

      The sub class would have to do the same as the format is set inside \Drupal\user\Controller\UserAuthenticationController::login() - this is just a further argument that having an alter would be nice.

      Edited by Alex Pott
    • Please register or sign in to reply
  • Alex Pott added 1 commit

    added 1 commit

    Compare with previous version

  • Alex Pott added 1 commit

    added 1 commit

    Compare with previous version

  • Alex Pott added 2 commits

    added 2 commits

    • 973b5985 - 1 commit from branch project:2.x
    • 394dd6b2 - Merge branch '2.x' into 2957273-jwt-log-in

    Compare with previous version

  • Alex Pott added 2 commits

    added 2 commits

    • 1249a3d8 - 1 commit from branch project:2.x
    • 8704e85a - Merge branch '2.x' into 2957273-jwt-log-in

    Compare with previous version

  • Peter Wolanin merged

    merged

    • Please register or sign in to reply