Merge pull request #81 from kjsanger/bug/deny-federated-synonym

Fix defects in OIDC to iRODS user mapping and permission checks

.github/workflows/run-tests.yml

@@ -54,11 +54,6 @@ jobs:
         go-version-file: "go.mod"
         cache: true
 
-    - name: "Install test runner"
-      run: |
-        go install -mod=mod github.com/onsi/ginkgo/v2/ginkgo
-        go get github.com/onsi/gomega/...
-
     - name: "Configure iRODS clients"
       run: |
         mkdir -p "$HOME/.irods"

Makefile

@@ -8,7 +8,7 @@ export GOARCH := $(shell go env GOARCH)
 
 CGO_ENABLED := 1
 
-.PHONY: build build-linux build-darwin build-windows check clean coverage install lint test
+.PHONY: build build-linux build-darwin build-windows check clean coverage install lint test-install test
 
 all: build
 
@@ -37,10 +37,13 @@ lint:
 
 check: test
 
-test:
+test-install:
+	go install github.com/onsi/ginkgo/v2/ginkgo
+
+test: test-install
 	ginkgo -r --race
 
-coverage:
+coverage: test-install
 	ginkgo -r --cover -coverprofile=coverage.out
 
 dist: build

server/handlers.go

@@ -338,15 +338,20 @@ func HandleIRODSGet(server *SqyrrlServer) http.Handler {
 			return
 		}
 
-		userZone := server.iRODSAccount.ClientZone
+		localZone := server.iRODSAccount.ClientZone
 
 		var isReadable bool
 
 		if server.isAuthenticated(r) {
+			// The username obtained from the email address does not include the iRODS
+			// zone. We use the local zone to which the  Sqyrrl server is connected as
+			// the user's zone.
 			userName := iRODSUsernameFromEmail(logger, server.getSessionUserEmail(r))
+			userZone := localZone
+
 			logger.Debug().Str("user", userName).Msg("User is authenticated")
 
-			isReadable, err = IsReadableByUser(logger, rodsFs, userName, userZone, objPath)
+			isReadable, err = IsReadableByUser(logger, rodsFs, localZone, userName, userZone, objPath)
 			if err != nil {
 				logger.Err(err).Msg("Failed to check if the object is readable")
 				writeErrorResponse(logger, w, http.StatusInternalServerError)
@@ -364,7 +369,7 @@ func HandleIRODSGet(server *SqyrrlServer) http.Handler {
 			}
 		} else {
 			logger.Debug().Msg("User is not authenticated")
-			isReadable, err = IsPublicReadable(logger, rodsFs, userZone, objPath)
+			isReadable, err = IsPublicReadable(logger, rodsFs, localZone, objPath)
 			if err != nil {
 				logger.Err(err).Msg("Failed to check if the object is public readable")
 				writeErrorResponse(logger, w, http.StatusInternalServerError)

server/irods.go

@@ -18,6 +18,7 @@
 package server
 
 import (
+	"crypto/tls"
 	"fmt"
 	"net/http"
 	"os"
@@ -141,6 +142,14 @@ func NewIRODSAccount(logger zerolog.Logger, manager *config.ICommandsEnvironment
 		return nil, err
 	}
 
+	var tlsConfig *tls.Config
+	if tlsConfig, err = account.SSLConfiguration.GetTLSConfig(account.Host); err != nil {
+		logger.Err(err).Msg("Failed to update TLS configuration")
+		return nil, err
+	}
+	// Add a cipher suite that is compatible with older iRODS servers (4.2.7)
+	tlsConfig.CipherSuites = append(tlsConfig.CipherSuites, tls.TLS_RSA_WITH_AES_256_CBC_SHA)
+
 	if password != "" {
 		if err = InitIRODS(logger, manager, password); err != nil {
 			logger.Err(err).
@@ -190,18 +199,41 @@ func NewIRODSAccount(logger zerolog.Logger, manager *config.ICommandsEnvironment
 }
 
 // IsReadableByUser checks if the data object at the given path is readable by the
-// given user in the zone hosting the file.
+// given user in the zone the Sqyrrl server is logged into.
 //
 // If iRODS is federated, there may be multiple zones, each with their own users.
-// The zone argument is the zone of user whose read permission is to be checked,
-// which is normally the current zone. This is consulted only if the ACL user zone is
-// empty.
+//
+// The localZone argument is the zone that Sqyrrl is logged into. This is consulted
+// only if an access permission in the data object's ACL has an empty user zone, in which
+// case the local zone is assumed.
+//
+// The userZone argument is the zone of the user whose read permission is to be checked.
 //
 // A file is considered readable if the user has read access or is in a group that has
 // read access.
 func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem,
-	userName string, userZone string, rodsPath string) (_ bool, err error) {
+	localZone string, userName string, userZone string, rodsPath string) (_ bool, err error) {
 	var acl []*types.IRODSAccess
+	var users []*types.IRODSUser
+
+	localUserExists := false
+	if users, err = filesystem.ListUsers(); err != nil {
+		return false, err
+	}
+	for _, user := range users {
+		if user.Name == userName && user.Zone == userZone {
+			localUserExists = true
+			break
+		}
+	}
+	if !localUserExists {
+		logger.Warn().
+			Str("path", rodsPath).
+			Str("user", userName).
+			Str("zone", userZone).
+			Msg("iRODS user not found")
+		return false, nil
+	}
 
 	if acl, err = filesystem.ListACLs(rodsPath); err != nil {
 		return false, err
@@ -231,25 +263,25 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem,
 
 	for _, ac := range acl {
 		// ACL user zone may be empty if it refers to the local zone
-		var effectiveUserZone string
+		var acUserZone string
 		if ac.UserZone != "" {
-			effectiveUserZone = ac.UserZone
+			acUserZone = ac.UserZone
 		} else {
-			effectiveUserZone = userZone
+			acUserZone = localZone
 		}
 
 		hasRead := ac.AccessLevel == types.IRODSAccessLevelReadObject
 		hasOwn := ac.AccessLevel == types.IRODSAccessLevelOwner
 
 		// There is permission directly for the user
 		if ac.UserType == types.IRODSUserRodsUser || ac.UserType == types.IRODSUserRodsAdmin {
-			if effectiveUserZone == userZone && ac.UserName == userName && (hasRead || hasOwn) {
+			if acUserZone == userZone && ac.UserName == userName && (hasRead || hasOwn) {
 				logger.Trace().
 					Str("path", rodsPath).
 					Str("user", userName).
 					Str("zone", userZone).
-					Str("effective_zone", effectiveUserZone).
 					Str("ac_user", ac.UserName).
+					Str("ac_zone", acUserZone).
 					Str("ac_level", string(ac.AccessLevel)).
 					Bool("read", hasRead).
 					Bool("own", hasOwn).
@@ -263,7 +295,7 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem,
 				Str("user", userName).
 				Str("zone", userZone).
 				Str("ac_user", ac.UserName).
-				Str("effective_zone", effectiveUserZone).
+				Str("ac_zone", acUserZone).
 				Str("ac_level", string(ac.AccessLevel)).
 				Bool("read", hasRead).
 				Bool("own", hasOwn).
@@ -275,13 +307,13 @@ func IsReadableByUser(logger zerolog.Logger, filesystem *ifs.FileSystem,
 			// Check if user in the group of this AC (ac.UserName is the name of the AC's group, unfortunately)
 			_, userInGroup := userGroupLookup[ac.UserName]
 
-			if effectiveUserZone == userZone && userInGroup && (hasRead || hasOwn) {
+			if acUserZone == userZone && userInGroup && (hasRead || hasOwn) {
 				logger.Trace().
 					Str("path", rodsPath).
 					Str("user", userName).
 					Str("zone", userZone).
-					Str("effective_zone", effectiveUserZone).
 					Str("ac_user", ac.UserName).
+					Str("ac_zone", acUserZone).
 					Str("ac_level", string(ac.AccessLevel)).
 					Bool("read", hasRead).
 					Bool("own", hasOwn).

server/irods_test.go

@@ -34,13 +34,15 @@ import (
 
 var _ = Describe("iRODS functions", func() {
 	var conn *connection.IRODSConnection
-	var zone string
+	var localZone, userZone string
 	var workColl string
 	var testFile, localPath, remotePath string
 	var err error
 
 	BeforeEach(func(ctx SpecContext) {
-		zone = "testZone"
+		localZone = "testZone"
+		userZone = localZone
+
 		workColl = TmpRodsPath(rootColl, "iRODSGetHandler")
 		err = irodsFS.MakeDir(workColl, true)
 		Expect(err).NotTo(HaveOccurred())
@@ -67,17 +69,17 @@ var _ = Describe("iRODS functions", func() {
 
 	When("a non-existent path is given", func() {
 		It("should return a FileNotFoundError", func() {
-			_, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic,
-				zone, "/no/such/path")
+			_, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+				userInPublic, userZone, "/no/such/path")
 			Expect(err).To(HaveOccurred())
 			Expect(err).To(MatchError(&types.FileNotFoundError{}))
 		})
 	})
 
 	When("a non-existent user is given", func() {
 		It("should return false", func() {
-			readable, err := server.IsReadableByUser(suiteLogger, irodsFS, "no_such_user",
-				zone, remotePath)
+			readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+				"no_such_user", userZone, remotePath)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(readable).To(BeFalse())
 		})
@@ -108,21 +110,31 @@ var _ = Describe("iRODS functions", func() {
 
 			When("the user is in the public group", func() {
 				It("should return false", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeFalse())
 				})
+
+				When("the user is in a different zone", func() {
+					It("should return false", func() {
+						readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+							userInPublic, "anotherZone", remotePath)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(readable).To(BeFalse())
+					})
+				})
 			})
 
 			When("the user is not in the public group", func() {
 				It("should return false", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userNotInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeFalse())
 				})
 			})
+
 		})
 
 		When("the data object has read permissions for the public group", func() {
@@ -134,21 +146,31 @@ var _ = Describe("iRODS functions", func() {
 
 			When("the user is in the public group", func() {
 				It("should return true", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeTrue())
 				})
+
+				When("the user is in a different zone", func() {
+					It("should return false", func() {
+						readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+							userInPublic, "anotherZone", remotePath)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(readable).To(BeFalse())
+					})
+				})
 			})
 
 			When("the user is not in the public group", func() {
 				It("should return false", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userNotInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeFalse())
 				})
 			})
+
 		})
 
 		When("the data object has own permissions for the public group", func() {
@@ -160,21 +182,31 @@ var _ = Describe("iRODS functions", func() {
 
 			When("the user is in the public group", func() {
 				It("should return true", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeTrue())
 				})
+
+				When("the user is in a different zone", func() {
+					It("should return false", func() {
+						readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+							userInPublic, "anotherZone", remotePath)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(readable).To(BeFalse())
+					})
+				})
 			})
 
 			When("the user is not in the public group", func() {
 				It("should return false", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userNotInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeFalse())
 				})
 			})
+
 		})
 
 		When("the data object has read permissions for several groups", func() {
@@ -188,17 +220,26 @@ var _ = Describe("iRODS functions", func() {
 
 			When("the user is in one of the groups", func() {
 				It("should return true", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userInOthers,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userInOthers, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeTrue())
 				})
+
+				When("the user is in a different zone", func() {
+					It("should return false", func() {
+						readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+							userInOthers, "anotherZone", remotePath)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(readable).To(BeFalse())
+					})
+				})
 			})
 
 			When("the user is not in one of the groups", func() {
 				It("should return false", func() {
-					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, userNotInPublic,
-						zone, remotePath)
+					readable, err := server.IsReadableByUser(suiteLogger, irodsFS, localZone,
+						userNotInPublic, userZone, remotePath)
 					Expect(err).NotTo(HaveOccurred())
 					Expect(readable).To(BeFalse())
 				})