Uploaded image for project: 'Artifactory Binary Repository'
  1. Artifactory Binary Repository
  2. RTFACT-22613

Storage event user plugin is not usable with NPM repositories

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Priority: 4 - Normal
    • Resolution: Unresolved
    • Affects Version/s: 6.9.6
    • Fix Version/s: None
    • Component/s: User Plugins
    • Labels:
    • Environment:

      RHEL7 Linux

    • Severity:
      Medium

      Description

      This is probably some level of a design bug

       

      I’m building a plugin to create a namespace within an NPM repository.

      I have done this successfully for Docker, Helm and GO type repositories to date. The implementation is based on knowing the path of the artifact the user is attempting to write and being able to know the user id and groups (obtained via LDAP) that the user is in. I know that this could be managed with permissions and include/exclude paths in Artifactory, but with the use of about a page full of plugin logic I can eliminate artifactory admin work and have this be self services to users who can simply join LDAP groups to be part of different teams in the namespace hierarchy. This approach was suggested by a JFrog implmementation engineer based on this github example (which I think no longer exists, but noting because perhaps you still have it internally) https://github.com/JFrogDev/artifactory-user-plugins/security/sandboxPermissions/sandboxPermissions.groovy

      However, this approach is not working with NPM.

      The TL;DR of my request is that in its current form, you cannot implement ANY type logic within this plugin for NPM repositories which might need to reject an upload. That is a problem with the usefulness of the storage event user plugin feature in the case of NPM repositories. The basic requirements that should apply across all repo types are:

      • All invocations of the storage event plugin MUST provide the context of the user ID that made the request that is causing this action to occur
      • All repository artifact deployment behaviors, must act atomically at least as regards a single incoming HTTP request even if such a request would cause the creation of multiple files within the repository. Either all or none of the files to be created due to that HTTP request should be the result
      • Any failure from the any of the multiple calls to the storage plugin which would occur in the context of a single incoming HTTP request must be able to handle and process a CancelException in support of the needed atomic behavior and must cause the HTTP request itself to return an error code to the user.
      •  The HTTP error to the user should not be a 500 level error as this should be handled as a possible and expected possibility by Artifactory, not an unhandled exception

      What follows is the details of the currently behavior which demonstrate the issue and some minimal plugin implementations that you can use to verify my observations of the current behavior.

      Here is the scenario of my script

      When I use NPM or Yarn to publish a module, it invokes a single HTTP put command. See the below Yarn verbose output

      verbose 6.323 Performing "PUT" request to "http://repoproxy-dev.eng.netapp.com:8081/artifactory/api/npm/npm-virtual/@foobar%2ftestme2-index".
      {{ verbose 6.499 Request "http://repoproxy-dev.eng.netapp.com:8081/artifactory/api/npm/npm-virtual/@foobar%2ftestme2-index" finished with status code 201.}}
      {{ success Published.}}

      You can also see this in the Artifactory request log where this PUT is logged under the user ‘test’ that I used to login from the yarn client

      20200623111155|156|REQUEST|10.231.176.52|test|PUT|/api/npm/npm-virtual/@foobar%2ftestme2-index|HTTP/1.1|201|2183

      On the artifactory side, this invokes the plugin storage code twice under my current function. The first time it appears to be putting the package metadata into the repository, HOWEVER, as you can see from the debug logging I have in the plugin, this call occurs as an internal system user, not as the user making the HTTP request. In my logic, any user with system privs is allowed to do anything, I have no method to know that this call to the plugin was in service of a specific user’s request

      2020-06-23 11:11:55,123 [http-nio-8081-exec-2104] [INFO ] (o.j.r.n.r.h.NpmLocalRepoHandler:355) - Deploying npm package '@foobar/testme2-index/-/@foobar/testme2-index-0.1.4.tgz' into repo 'seclab-npm-local'
      {{ 2020-06-23 11:11:55,187 [http-nio-8081-exec-2104] [DEBUG] (userNamespacesNpm :35) - userNamespacesNpm: repoPath seclab-npm-local:.npm/@foobar/testme2-index/@foobar/testme2-index-0.1.4.json}}
      {{ 2020-06-23 11:11:55,188 [http-nio-8081-exec-2104] [DEBUG] (userNamespacesNpm :38) - userNamespacesNpm: User 'system' is admin and permitted to create, modify, or delete to any namespace}}

      Then my plugin is invoked again, this time to post the package tar/zip file. In this case, the user is provided to the plugin is the ‘test’ user making the request and my logic detects that “@foobar” is not an npm scope value that the test user is allowed to post (this is the call where the ‘directory’ level for the @foobar is created within the repo). Because this user is not allowed to publish under @foobal my plugin raises a CancelException and provides a 403 HTTP response code to the returned.

      {{ 2020-06-23 11:11:55,211 [http-nio-8081-exec-2104] [INFO ] (o.a.e.UploadServiceImpl:386) - Deploy to 'seclab-npm-local:@foobar/testme2-index/-/@foobar/testme2-index-0.1.4.tgz' Content-Length: unspecified}}
      {{ 2020-06-23 11:11:55,247 [http-nio-8081-exec-2104] [DEBUG] (userNamespacesNpm :35) - userNamespacesNpm: repoPath seclab-npm-local:@foobar}}
      {{ 2020-06-23 11:11:55,248 [http-nio-8081-exec-2104] [DEBUG] (userNamespacesNpm :45) - userNamespacesNpm: repoKey seclab-npm-local}}
      {{ 2020-06-23 11:11:55,249 [http-nio-8081-exec-2104] [INFO ] (userNamespacesNpm :55) - userNamespacesNpm: User 'test' push/upload NOT allowed due to invalid domain}}
      {{ 2020-06-23 11:11:55,251 [http-nio-8081-exec-2104] [ERROR] (o.a.r.i.s.StorageInterceptorsImpl:60) - Before create rejected: test push/upload NOT allowed due to invalid domain: test push/upload NOT allowed due to invalid domain}}
      {{ 2020-06-23 11:11:55,252 [http-nio-8081-exec-2104] [WARN ] (o.a.r.ArtifactoryResponseBase:125) - Sending HTTP error code 403: org.artifactory.exception.CancelException: test push/upload NOT allowed due to invalid domain}}Now, there are multiple problems when I hit this situation of a user posting a module which isn’t named in an allowable fashion or within a scope that user is allowed to use (another of my rules which doesn’t occur in this example)
      {{ 1. The 403 CancelException that I throw from the plugin never flows back on the original HTTP request (you can see it is a 201 return), so the original npm/yarn client thinks the publish succeeded (see the yarn output I posted above)}}
      {{ 2. The repo is left with a partial condition because the json metadata did get posted but the tgz file did not, so the module may appear to exist when it should not}}

      These problems existing because of what I think are the following problems

      1. The initial invocation to the storage user plugin with the request noted as being the system user. This should not be the case. This is happening because of the test user request and is storing data from the test users HTTP put command. Because the user context is incorrent, I’m not able to use this plugin to validate the request as I need to
      2. Maybe because of the first issue, but certainly a problem is that the 403 CancelException from the user plugin never returned error to the HTTP client. If the user issue on the first invocation were addressed that would help in that a cancel could be done then, but the cancel exception could occur on any invocation of the user plugin during handling of the HTTP request and it should error back to the original request.

      Here is the simplified version of the plugins that will demonstrate the issue. The support person said that the use of the _system user on the metadata call was "intended behavior as Artifactory is generating metadata on the package. This behavior was not seen with Docker, Helm, and GO because the metadata is generated by the package manager/cli, not by Artifactory."

      My assertion however is that this is insufficient if a user plugin is to have logic that is based on the user making the request to artifactory. With the first plugin example below you can see that if the CancelException is raised when the metadata invocation occurs as the system user, Artifactory has internal exceptions and returns a 500 level code to the HTTP user. So, even in the current implementation, If the context of the requesting user was even available at that point, Artifactory would not properly handle the exception

      At the end of the day, this behaviors I outlined mean that you cannot implement any type logic within this plugin for NPM repositories which might need to reject an upload. That is a problem with the usefulness of the user plugin feature in this case. I would like to have some type of feature issue raised here. The basic needs are
      • All invocations of the storage event plugin MUST provide the context of the user ID that made the request that is causing this action to occur
      • All repository artifact deployment behaviors, must act atomically at least as regards a single incoming HTTP request even if such a request would cause the creation of multiple files within the repository. Either all or none of the files to be created due to that HTTP request should be the result
      • Any failure from the any of the multiple calls to the storage plugin which would occur in the context of a single incoming HTTP request must be able to handle and process a CancelException in support of the needed atomic behavior and must cause the HTTP request itself to return an error code to the user.
      • The HTTP error to the user should not be a 500 level error as this should be handled as a possible and expected possibility by Artifactory, not an unhandled exception

       

      Plugins – Replace the name of a local NPM repo in the checked. Then publish an npm package to the repository
      The first basically will throw a CancelException on the first invocation of the plugin for the metadata, the second will do it on the second invocation or the tgz of the package.

      =================================================================
      import org.artifactory.exception.CancelException
      import org.artifactory.repo.*

      storage {
      beforeDelete

      { item -> verifyUserAuthorized(item.repoPath) }

      beforeCreate { item -> verifyUserAuthorized(item.repoPath) }

      }

      def verifyUserAuthorized(RepoPath repoPath) {
      // PUT THE NAME OF ANY INTERNAL NPM REPOSITORY INTO THIS LIST
      def checkedRepositories = ["REPONAME"]
      def pluginName = "userNamespacesNpmMetadataFail"

      def user = security.getCurrentUsername()
      def repoKey = repoPath.repoKey

      log.debug("{}: Called with user '{}'", pluginName, user)
      log.debug("{}: Called with repopath '{}'", pluginName, repoPath)

      if(repoKey in checkedRepositories)

      { // CAUSE THE PLUGIN TO FAIL ON THE FIRST INVOCATION WHICH WILL HAPPEN TO BE THE NPM METADATA INVOCATION AS SYSTEM USER throw new CancelException("Plugin will not allow upload", 403) }

      }

      =================================================================
      import org.artifactory.exception.CancelException
      import org.artifactory.repo.*

      storage {
      beforeDelete

      { item -> verifyUserAuthorized(item.repoPath) }

      beforeCreate { item -> verifyUserAuthorized(item.repoPath) }

      }

      def verifyUserAuthorized(RepoPath repoPath) {
      // PUT THE NAME OF ANY INTERNAL NPM REPOSITORY INTO THIS LIST
      def checkedRepositories = ["REPONAME"]
      def pluginName = "userNamespacesNpmPackageFail"

      def user = security.getCurrentUsername()
      def repoKey = repoPath.repoKey

      log.debug("{}: Called with user '{}'", pluginName, user)
      log.debug("{}: Called with repopath '{}'", pluginName, repoPath)

      if (security.isAdmin())

      { // RETURN IF USER HAS ADMIN LEVEL PERMISSIONS, THIS WILL MAKE THE NPM METADATA CALL BE OK return; }

      if(repoKey in checkedRepositories)

      { // CAUSE THE PLUGIN TO FAIL ON THE FIRST INVOCATION AS HTTP REQUEST USER WHICH WILL BE THE TGZ FILE FOR THE MODULE throw new CancelException("Plugin will not allow upload", 403) }

      }

       

        Attachments

          Activity

            People

            Assignee:
            Unassigned
            Reporter:
            NetAppBlueDevil Daniel Holmes
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:

                Sync Status

                Connection: RTFACT Sync
                RTMID-22613 -
                SYNCHRONIZED
                • Last Sync Date: