Skip to content

Conversation

@ocielliottc
Copy link
Collaborator

Adds the functionality to switch to a different user (given the correct permissions) and to switch back to the original user.
The UI change to allow this is within the drop-down button on the /admin/users page.

@ocielliottc ocielliottc requested a review from mkimberlin August 12, 2024 17:15
@ocielliottc ocielliottc linked an issue Aug 12, 2024 that may be closed by this pull request
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

@Requires(env = Environments.LOCAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOCAL or DEV, I think...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found no reference to a DEV environment anywhere in the code. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely correct. It is specified on deployment though (see line 109):

--set-env-vars "^@^MICRONAUT_ENVIRONMENTS=dev,cloud,google,gcp" \

You'll need to add it to Environments as Environments.DEVELOP or some such with a value of "dev"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Environment.DEVELOPMENT here from micronaut. Is that going to do what's required?

this.securityService = securityService;
}

@ExecuteOn(TaskExecutors.BLOCKING)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need this here or on the revert endpoint. It's at the class level already.

@Consumes({MediaType.APPLICATION_FORM_URLENCODED, MediaType.APPLICATION_JSON})
@Post("/begin")
@RequiredPermission(Permission.CAN_IMPERSONATE_MEMBERS)
public Mono<Object> auth(HttpRequest<?> request, String email) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of the Mono usage here. It shouldn't be necessary any longer (there are only a few endpoints that use these still). I think you want an HttpResponse<Void> here for your return object instead...or maybe a HttpResponse<?>. Then you can use methods like HttpResponse.serverError() and HttpResponse.ok() to construct a response with the right response code. Those methods return MutableHttpResponse instances. So, you can do things like add cookies, etc. if needed.

At minimum it looks like you probably want to return HttpResponse.serverError() in the default cases toward the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Authenticator.authenticate() returns a Publisher. I'm thinking Mono was left in the LocalLoginController because of this. Or maybe I just don't know how to deal with the Publisher such that we can return an HttpResponse?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this a little bit. Definitely not worth trying to change.

}).single(Mono.just(HttpResponse.status(HttpStatus.UNAUTHORIZED)));
}
} else {
LOG.error("Attempted impersonation without authentication.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpResponse.serverError() here...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still do these though...just return Mono.just(HttpResponse.serverError());

LOG.error("Attempted impersonation without authentication.");
}
}
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpResponse.serverError() here...

return null;
}

@ExecuteOn(TaskExecutors.BLOCKING)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill this @ExecuteOn

@ExecuteOn(TaskExecutors.BLOCKING)
@Produces(MediaType.TEXT_HTML)
@Get("/end")
public HttpResponse revert(HttpRequest<?> request) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think HttpResponse<Void> is generally preferred in cases like this.

public HttpResponse revert(HttpRequest<?> request) {
final Cookie ojwt = request.getCookies().get(originalJWT);
if (ojwt == null) {
return HttpResponse.status(HttpStatus.UNAUTHORIZED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpResponse.unauthorized()

}
}
return null;
return Mono.just(HttpResponse.unauthorized());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mkimberlin mkimberlin merged commit 3827323 into develop Aug 13, 2024
@mkimberlin mkimberlin deleted the feature-2569/impersonate-user branch August 13, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ability to switch to different users during development

3 participants