[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254176067
##
File path:
server/src/test/scala/org/apache/livy/server/SessionServletSpec.scala
##
@@ -114,17 +111,66 @@ class SessionServletSpec extends
BaseSessionServletSpec[Session, RecoveryMetadat
}
it("should attach owner information to sessions") {
+ jpost[MockSessionView]("/", Map()) { res =>
+assert(res.owner === null)
+assert(res.proxyUser === None)
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, adminHeaders, SC_OK)
+ }
+
jpost[MockSessionView]("/", Map(), headers = aliceHeaders) { res =>
assert(res.owner === "alice")
+assert(res.proxyUser === Some("alice"))
+assert(res.logs === IndexedSeq("log"))
+delete(res.id, aliceHeaders, SC_OK)
+ }
+
+ jpost[MockSessionView]("/?doAs=alice", Map(), headers = adminHeaders) {
res =>
+assert(res.owner === ADMIN)
+assert(res.proxyUser === Some("alice"))
assert(res.logs === IndexedSeq("log"))
delete(res.id, aliceHeaders, SC_OK)
}
}
it("should allow other users to see non-sensitive information") {
+ jpost[MockSessionView]("/", Map()) { res =>
+jget[MockSessionView](s"/${res.id}", headers = bobHeaders) { res =>
+ assert(res.owner === null)
+ assert(res.proxyUser === None)
+ assert(res.logs === IndexedSeq("log"))
Review comment:
My point is: the focus of this PR is adding the parameter `doAs` in order to
allow user impersonation. The authorization logic is orthogonal to this change.
Adding tests for this situation I think is helpful in order to determine and
ensure the current behavior, since they were missing.
If we decide that the logic/behavior we have currently for the
authorization, I think we can change it adding some parameters in order to
ensure users who upgrade can have no behavior change. But anyway, I think this
should not be done in this PR, since the scope of this PR is to add the `doAs`
parameter, not to change the impersonation and access control logic.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r254176751
##
File path:
server/src/test/scala/org/apache/livy/server/interactive/JobApiSpec.scala
##
@@ -227,3 +230,59 @@ class JobApiSpec extends BaseInteractiveServletSpec {
}
}
+
+class JobApiSpecNoImpersonation extends JobApiSpec {
+ override protected def createConf(): LivyConf = synchronized {
+super.createConf().set(LivyConf.IMPERSONATION_ENABLED, false)
+ }
+
+ it("should not support user impersonation") {
+assume(!createConf().getBoolean(LivyConf.IMPERSONATION_ENABLED))
+jpost[SessionInfo]("/", createRequest(inProcess = false)) { data =>
+ try {
+waitForIdle(data.id)
+data.owner should be (null)
+data.proxyUser should be (null)
+val user = runJob(data.id, new GetCurrentUser())
+user should be (UserGroupInformation.getCurrentUser.getUserName)
+ } finally {
+deleteSession(data.id)
+ }
+}
+
+val headers = makeUserHeaders(PROXY)
+jpost[SessionInfo]("/", createRequest(inProcess = false), headers =
headers) { data =>
+ try {
+waitForIdle(data.id)
+data.owner should be (PROXY)
+data.proxyUser should be (null)
+val user = runJob(data.id, new GetCurrentUser(), headers = headers)
+user should be (UserGroupInformation.getCurrentUser.getUserName)
+ } finally {
+deleteSession(data.id)
+ }
+}
+ }
+
+ it("should not honor impersonation requests") {
Review comment:
As I said before, I think that if we want to introduce a change in the
behavior we currently have for user impersonation, we should do it in another
JIRA/PR.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r253107340
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -160,6 +160,29 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
*/
protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+ /**
+ * Returns the impersonated user as given by "doAs" as a request parameter.
+ */
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+Option(request.getParameter("doAs"))
+ }
+
+ /**
+ * Returns the proxyUser for the given request.
+ */
+ protected def proxyUser(
+ request: HttpServletRequest,
+ createRequestProxyUser: Option[String]): Option[String] = {
+impersonatedUser(request).orElse(createRequestProxyUser)
+ }
+
+ /**
+ * Gets the request user or impersonated user to determine the effective
user.
+ */
+ protected def effectiveUser(request: HttpServletRequest): String = {
+accessManager.checkImpersonation(impersonatedUser(request),
remoteUser(request)).orNull
Review comment:
why orNull? Shouldn't be `remoteUser(request)` in that case? I'd expect some
NullPointerException to happen with `orNull` or anyway, some problems when
checking access permissions (since we have null instead of the requesting
user). May you please also add a UT for this?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252992534
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,23 +99,32 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false.
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
+ impersonatedUser: String): Boolean = {
Review comment:
I am wondering why you are changing this to Boolean and introducing a new
method for doing what this was doing earlier. Seems you are just complicating
things.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252743394
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -160,6 +160,30 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
*/
protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+ /**
+ * Returns the impersonated user as given by "doAs" as a request parameter.
+ */
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+Option(request.getParameter("doAs"))
+ }
+
+ /**
+ * Returns the proxyUser for the given request.
+ */
+ protected def proxyUser(request: HttpServletRequest,
+ createRequestProxyUser: Option[String]): Option[String] = {
+
impersonatedUser(request).orElse(createRequestProxyUser).orElse(Option(remoteUser(request)))
Review comment:
Because previously the check was in `checkImpersonation`. Since you moved it
out from there, you should have been put it somewhere else. The point is: this
is not the right place where to put it, since this is not the `proxyUser` which
this method should return and to match previous behavior this should be done
only when impersonation in enabled (otherwise we'd need None at the end).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252576879
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -160,6 +160,30 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
*/
protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+ /**
+ * Returns the impersonated user as given by "doAs" as a request parameter.
+ */
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+Option(request.getParameter("doAs"))
+ }
+
+ /**
+ * Returns the proxyUser for the given request.
+ */
+ protected def proxyUser(request: HttpServletRequest,
+ createRequestProxyUser: Option[String]): Option[String] = {
+
impersonatedUser(request).orElse(createRequestProxyUser).orElse(Option(remoteUser(request)))
Review comment:
why `orElse(Option(remoteUser(request)))`? In that case we should return
`None` here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252573979
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -160,6 +160,30 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
*/
protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+ /**
+ * Returns the impersonated user as given by "doAs" as a request parameter.
+ */
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+Option(request.getParameter("doAs"))
+ }
+
+ /**
+ * Returns the proxyUser for the given request.
+ */
+ protected def proxyUser(request: HttpServletRequest,
+ createRequestProxyUser: Option[String]): Option[String] = {
+
impersonatedUser(request).orElse(createRequestProxyUser).orElse(Option(remoteUser(request)))
+ }
+
+ /**
+ * Gets the request user or impersonated user to determine the effective
user.
+ */
+ protected def effectiveUser(request: HttpServletRequest): String = {
+val requestUser = remoteUser(request)
+impersonatedUser(request).orElse(Option(requestUser))
+ .filter(accessManager.checkImpersonation(requestUser, _)).orNull
Review comment:
I think this should be:
```
impersonatedUser(request).filter(accessManager.checkImpersonation(requestUser,
_)).getOrElse(requestUser)
```
Am I wrong? Why are you checking the impersonation also when there is no
impersonated user?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252576594
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala
##
@@ -57,10 +57,12 @@ object BatchSession extends Logging {
livyConf: LivyConf,
accessManager: AccessManager,
owner: String,
+ impersonatedUser: Option[String],
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None): BatchSession = {
val appTag = s"livy-batch-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
+
+impersonatedUser.foreach(accessManager.checkImpersonation(owner, _))
Review comment:
IIUC, as the code is now, when impersonation is disabled and we are using
the old proxyUser feature, we would allow the impersonation, right? Because
having the foreach here doesn't throw any exception and the returned false is
just ignored.
Anyway, can we add a UT for this case, in order to ensure the behavior is
fine?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252296678
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala
##
@@ -57,10 +57,12 @@ object BatchSession extends Logging {
livyConf: LivyConf,
accessManager: AccessManager,
owner: String,
+ impersonatedUser: Option[String],
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None): BatchSession = {
val appTag = s"livy-batch-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
+
+impersonatedUser.filter(accessManager.checkImpersonation(owner, _))
Review comment:
well, then `foreach` is better than `filter` if we return nothing. Anyway,
it actually works because you are doing the same check in `def
impersonatedUser` and `def legacyProxyUser`, so here you are relying on them,
rather than on this check .What I meant was doing the check here as it was
earlier and in those methods just check which user needs to be impersonated. I
see that you need that for the `effectiveUser` call, but I think we can move
the filter there and use this one here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252294370
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -183,11 +203,11 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
private def doWithSession(fn: (S => Any),
allowAll: Boolean,
- checkFn: Option[(String, String) => Boolean]): Any = {
+ checkFn: Option[(Session, String) => Boolean]): Any = {
val sessionId = params("id").toInt
sessionManager.get(sessionId) match {
case Some(session) =>
-if (allowAll || checkFn.map(_(session.owner,
remoteUser(request))).getOrElse(false)) {
+if (allowAll || checkFn.map(_(session,
effectiveUser(request))).getOrElse(false)) {
Review comment:
Yes, as I said, I think it is fine, but need to be documented
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252155798
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSessionServlet.scala
##
@@ -42,20 +42,25 @@ class BatchSessionServlet(
override protected def createSession(req: HttpServletRequest): BatchSession
= {
val createRequest = bodyAs[CreateBatchRequest](req)
+val owner = remoteUser(req)
+val proxyUser = impersonatedUser(req)
Review comment:
this code is duplicated, what about creating a method `proxyUser`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252149000
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,43 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
Review comment:
nit: missing dot
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252155964 ## File path: server/src/main/scala/org/apache/livy/sessions/Session.scala ## @@ -204,7 +204,7 @@ abstract class Session(val id: Int, val owner: String, val livyConf: LivyConf) protected def stopSession(): Unit - protected val proxyUser: Option[String] + val proxyUser: Option[String] Review comment: please add a comment: ``` // Visible for testing. ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252153640
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -156,9 +156,29 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
}
/**
- * Returns the remote user for the given request. Separate method so that
tests can override it.
- */
- protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+* Returns the remote user for the given request. Separate method so that
tests can override it.
+*/
+ protected def remoteUser(req: HttpServletRequest): String = req.getRemoteUser
+
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+val impersonatedUser = Option(request.getParameter("doAs"))
+
impersonatedUser.filter(accessManager.checkImpersonation(remoteUser(request),
_))
+ }
+
+ /**
+* Returns the proxy user as determined by the json request body or owner
if available.
Review comment:
nit: bad indent
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252149530 ## File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala ## @@ -156,9 +156,29 @@ abstract class SessionServlet[S <: Session, R <: RecoveryMetadata]( } /** - * Returns the remote user for the given request. Separate method so that tests can override it. - */ - protected def remoteUser(req: HttpServletRequest): String = req.getRemoteUser() +* Returns the remote user for the given request. Separate method so that tests can override it. Review comment: bad indentation and unneeded changes, please avoid them This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252153677
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -156,9 +156,29 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
}
/**
- * Returns the remote user for the given request. Separate method so that
tests can override it.
- */
- protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+* Returns the remote user for the given request. Separate method so that
tests can override it.
+*/
+ protected def remoteUser(req: HttpServletRequest): String = req.getRemoteUser
+
+ protected def impersonatedUser(request: HttpServletRequest): Option[String]
= {
+val impersonatedUser = Option(request.getParameter("doAs"))
+
impersonatedUser.filter(accessManager.checkImpersonation(remoteUser(request),
_))
+ }
+
+ /**
+* Returns the proxy user as determined by the json request body or owner
if available.
+* This is necessary to preserve backwards compatibility prior to "doAs"
impersonation.
+*/
+ protected def legacyProxyUser(owner: String, bodyProxyUser: Option[String]):
Option[String] = {
+bodyProxyUser.filter(accessManager.checkImpersonation(owner,
_)).orElse(Option(owner))
+ }
+
+ /**
+* Gets the request user or impersonated user to determine the effective
user
Review comment:
ditto
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252149231
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,43 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+true
+ } else {
throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ s"User '$requestUser' not allowed to impersonate
'$impersonatedUser'.")
}
- target.orElse(Option(requestUser))
} else {
- None
+ false
}
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
Review comment:
the new description is wrong IMHO, and these are unneeded changes (just
renaming variables). Please avoid them
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252155857
##
File path:
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSession.scala
##
@@ -67,14 +67,16 @@ object InteractiveSession extends Logging {
def create(
id: Int,
owner: String,
+ impersonatedUser: Option[String],
livyConf: LivyConf,
accessManager: AccessManager,
request: CreateInteractiveRequest,
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None,
mockClient: Option[RSCClient] = None): InteractiveSession = {
val appTag = s"livy-session-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
+
+impersonatedUser.filter(accessManager.checkImpersonation(owner, _))
Review comment:
ditto
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252156448 ## File path: server/src/test/scala/org/apache/livy/server/BaseSessionServletSpec.scala ## @@ -77,7 +77,6 @@ abstract class BaseSessionServletSpec[S <: Session, R <: RecoveryMetadata] addServlet(servlet, "/*") protected def toJson(msg: AnyRef): Array[Byte] = mapper.writeValueAsBytes(msg) - Review comment: in general we best avoid doing unneeded changes like these, makes easier checking the history of the changes.. so please undo the changes. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252154524
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -183,11 +203,11 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
private def doWithSession(fn: (S => Any),
allowAll: Boolean,
- checkFn: Option[(String, String) => Boolean]): Any = {
+ checkFn: Option[(Session, String) => Boolean]): Any = {
val sessionId = params("id").toInt
sessionManager.get(sessionId) match {
case Some(session) =>
-if (allowAll || checkFn.map(_(session.owner,
remoteUser(request))).getOrElse(false)) {
+if (allowAll || checkFn.map(_(session,
effectiveUser(request))).getOrElse(false)) {
Review comment:
again, this is a behavior change. Probably it is fine to leave it here, but
at least we need to document this behavior change. cc @vanzin
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252149380
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,43 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+true
+ } else {
throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ s"User '$requestUser' not allowed to impersonate
'$impersonatedUser'.")
}
- target.orElse(Option(requestUser))
} else {
- None
+ false
}
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
*/
- def hasSuperAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkSuperUser(requestUser)
+ def hasSuperAccess(requestUser: String, impersonatedUser: String): Boolean =
{
+requestUser == impersonatedUser || checkSuperUser(requestUser)
}
/**
- * Check that the request's user has modify access to resources owned by the
given target user.
+ * Check that the request user has modify access to the given session.
*/
- def hasModifyAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkModifyPermissions(requestUser)
+ def hasModifyAccess(session: Session, effectiveUser: String): Boolean = {
+session.owner == effectiveUser || checkModifyPermissions(effectiveUser)
}
/**
- * Check that the request's user has view access to resources owned by the
given target user.
+ * Check that the request user has view access to the given session
Review comment:
nit: missing dot
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r252155202
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala
##
@@ -57,10 +57,12 @@ object BatchSession extends Logging {
livyConf: LivyConf,
accessManager: AccessManager,
owner: String,
+ impersonatedUser: Option[String],
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None): BatchSession = {
val appTag = s"livy-batch-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
+
+impersonatedUser.filter(accessManager.checkImpersonation(owner, _))
Review comment:
the result of the filter is never used, so it is useless as it is now...
Moreover it is weird that tests are passing nonetheless... may you please add a
UT for this?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251888435
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,51 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
-throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+return true
}
- target.orElse(Option(requestUser))
-} else {
- None
+ throw new AccessControlException(
+s"User '$requestUser' not allowed to impersonate '$impersonatedUser'.")
}
+false
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
*/
- def hasSuperAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkSuperUser(requestUser)
+ def hasSuperAccess(
+ requestUser: String,
+ impersonatedUser: String): Boolean = {
+requestUser == impersonatedUser || checkSuperUser(requestUser)
}
/**
- * Check that the request's user has modify access to resources owned by the
given target user.
+ * Check that the request user has modify access to the given session.
*/
- def hasModifyAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkModifyPermissions(requestUser)
+ def hasModifyAccess(
+ session: Session,
+ effectiveUser: String): Boolean = {
+session.owner == effectiveUser ||
Review comment:
Yes, but IIUC this is a behavior change. We can argue whether it is right or
not the current behavior and eventually change it, but we should definitely not
do that in this PR, as it is unrelated with it. If I am not missing something,
please revert these changes and create a separate ticket in order to discuss
them separately.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251723202
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSession.scala
##
@@ -55,12 +54,11 @@ object BatchSession extends Logging {
id: Int,
request: CreateBatchRequest,
livyConf: LivyConf,
- accessManager: AccessManager,
owner: String,
+ impersonatedUser: Option[String],
sessionStore: SessionStore,
mockApp: Option[SparkApp] = None): BatchSession = {
val appTag = s"livy-batch-$id-${Random.alphanumeric.take(8).mkString}"
-val impersonatedUser = accessManager.checkImpersonation(request.proxyUser,
owner, livyConf)
Review comment:
yes, exactly, that's my worry.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251422686
##
File path: server/src/main/scala/org/apache/livy/server/SessionServlet.scala
##
@@ -156,9 +156,39 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
}
/**
- * Returns the remote user for the given request. Separate method so that
tests can override it.
- */
- protected def remoteUser(req: HttpServletRequest): String =
req.getRemoteUser()
+* Gets the remote user from the given request
+*/
+ protected def getRemoteUser(request: HttpServletRequest): String = {
+request.getRemoteUser
+ }
+
+ protected def getImpersonatedUser(request: HttpServletRequest):
Option[String] = {
+val impersonatedUser = Option(request.getParameter("doAs"))
+
impersonatedUser.filter(accessManager.checkImpersonation(getRemoteUser(request),
_))
+ }
+
+
+ /**
+* Returns the proxy user as determined by the json request body or owner
if available.
+* This is necessary to preserve backwards compatibility prior to "doAs"
impersonation.
+*/
+ protected def getLegacyProxyUser(owner: String,
+ bodyProxyUser: Option[String]):
Option[String] = {
Review comment:
nit: indentation
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251422378
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,51 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
-throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+return true
}
- target.orElse(Option(requestUser))
-} else {
- None
+ throw new AccessControlException(
+s"User '$requestUser' not allowed to impersonate '$impersonatedUser'.")
}
+false
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
*/
- def hasSuperAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkSuperUser(requestUser)
+ def hasSuperAccess(
+ requestUser: String,
+ impersonatedUser: String): Boolean = {
+requestUser == impersonatedUser || checkSuperUser(requestUser)
}
/**
- * Check that the request's user has modify access to resources owned by the
given target user.
+ * Check that the request user has modify access to the given session.
*/
- def hasModifyAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkModifyPermissions(requestUser)
+ def hasModifyAccess(
+ session: Session,
+ effectiveUser: String): Boolean = {
+session.owner == effectiveUser ||
Review comment:
this is different from the previous implementation, why do we need this
change?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251421028
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,51 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
Review comment:
mmmh...now you are using the `conf` passed when building the
`AccessManager`, which may be different from the one which was used before this
patch. Despite this one seems a cleaner approach, I wonder if there may be some
behavioral changes due to this. cc @vanzin for his opinion.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251421630
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,51 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
-throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+return true
}
- target.orElse(Option(requestUser))
-} else {
- None
+ throw new AccessControlException(
+s"User '$requestUser' not allowed to impersonate '$impersonatedUser'.")
}
+false
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
*/
- def hasSuperAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkSuperUser(requestUser)
+ def hasSuperAccess(
Review comment:
nit: one line?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251421684
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +99,51 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Checks that the request user can impersonate the target user.
+ * If impersonation is enabled and the user does not have permission to
impersonate
+ * then throws an `AccessControlException`. If impersonation is disabled
returns false
*/
def checkImpersonation(
- target: Option[String],
requestUser: String,
- livyConf: LivyConf): Option[String] = {
-if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
- if (!target.forall(hasSuperAccess(_, requestUser))) {
-throw new AccessControlException(
- s"User '$requestUser' not allowed to impersonate '$target'.")
+ impersonatedUser: String): Boolean = {
+if (conf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) {
+ if (hasSuperAccess(requestUser, impersonatedUser)) {
+return true
}
- target.orElse(Option(requestUser))
-} else {
- None
+ throw new AccessControlException(
+s"User '$requestUser' not allowed to impersonate '$impersonatedUser'.")
}
+false
}
/**
- * Check that the requesting user has admin access to resources owned by the
given target user.
+ * Check that the request user has is able to impersonate the given user.
*/
- def hasSuperAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkSuperUser(requestUser)
+ def hasSuperAccess(
+ requestUser: String,
+ impersonatedUser: String): Boolean = {
+requestUser == impersonatedUser || checkSuperUser(requestUser)
}
/**
- * Check that the request's user has modify access to resources owned by the
given target user.
+ * Check that the request user has modify access to the given session.
*/
- def hasModifyAccess(target: String, requestUser: String): Boolean = {
-requestUser == target || checkModifyPermissions(requestUser)
+ def hasModifyAccess(
Review comment:
nit: one line
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r250715644
##
File path: server/src/main/scala/org/apache/livy/server/AccessManager.scala
##
@@ -98,46 +100,79 @@ private[livy] class AccessManager(conf: LivyConf) extends
Logging {
def isAccessControlOn: Boolean = aclsOn
/**
- * Checks that the requesting user can impersonate the target user.
- * If the user does not have permission to impersonate, then throws an
`AccessControlException`.
- *
- * @return The user that should be impersonated. That can be the target user
if defined, the
- * request's user - which may not be defined - otherwise, or `None`
if impersonation is
- * disabled.
+ * Gets the remote user from the given request
+ */
+ def getRequestUser(request: HttpServletRequest): String = {
Review comment:
I'd prefer to keep the AccessManager independent from `HttpServletRequest`:
logically, the `AccessManager` has no connection at all with the REST/HTTP
interface and I think we should keep it like that.
We can put all the methods which depend on `HttpServletRequest` in the
`SessionServlet` class and keep there the logic depending on it.
Eg. it is used also in the Thrift API and we should not use there anything
which is specific for the REST API.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
mgaido91 commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r250716539
##
File path:
server/src/main/scala/org/apache/livy/server/batch/BatchSessionServlet.scala
##
@@ -42,20 +42,24 @@ class BatchSessionServlet(
override protected def createSession(req: HttpServletRequest): BatchSession
= {
val createRequest = bodyAs[CreateBatchRequest](req)
+val owner = accessManager.getRequestUser(req)
+val legacyProxyUser = getLegacyProxyUser(owner, createRequest.proxyUser,
req)
+val impersonatedUser = getImpersonatedUser(req).orElse(legacyProxyUser)
Review comment:
we are doing the parsing for `legacyProxyUser ` also when it is not needed,
we can do:
```
orElse(getLegacyProxyUser())
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
