[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] vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs" impersonation support
vanzin commented on a change in pull request #141: [LIVY-551] Add "doAs"
impersonation support
URL: https://github.com/apache/incubator-livy/pull/141#discussion_r251675173
##
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:
It's just moving the check to the servlet, which happens before this is
called. Are you worried that another call site might be added and we forget
about the access check?
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 edited a comment on issue #25: [MINOR] Updating contributors
mgaido91 edited a comment on issue #25: [MINOR] Updating contributors URL: https://github.com/apache/incubator-livy-website/pull/25#issuecomment-458167328 cc @ajbozarth @alex-the-man 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 issue #25: [MINOR] Updating contributors
mgaido91 commented on issue #25: [MINOR] Updating contributors URL: https://github.com/apache/incubator-livy-website/pull/25#issuecomment-458167328 cc @alex-the-man @ajbozarth 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 opened a new pull request #25: [MINOR] Updating contributors
mgaido91 opened a new pull request #25: [MINOR] Updating contributors URL: https://github.com/apache/incubator-livy-website/pull/25 Updating contributors list, adding new PPMC members and fixing a typo. 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 merged pull request #139: [MINOR] Fix instructions on how to build Livy.
mgaido91 merged pull request #139: [MINOR] Fix instructions on how to build Livy. URL: https://github.com/apache/incubator-livy/pull/139 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 issue #139: [MINOR] Fix instructions on how to build Livy.
mgaido91 commented on issue #139: [MINOR] Fix instructions on how to build Livy. URL: https://github.com/apache/incubator-livy/pull/139#issuecomment-458067915 Thanks. Merging to master. 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
