From e0544579d208b6af396da08f1454922013f5ecc8 Mon Sep 17 00:00:00 2001 From: fantiu Date: Wed, 22 Apr 2026 12:05:30 +0800 Subject: [PATCH] fix(auth): attribute CLI auth denial to channel before user scope (#144) * chore: update coverage badge [skip ci] * chore: update coverage badge [skip ci] * fix(auth): attribute CLI auth denial to channel before user scope --------- Co-authored-by: github-actions[bot] --- .github/badges/coverage.svg | 2 +- internal/auth/classify_denial_reason_test.go | 144 +++++++++++++++++++ internal/auth/oauth_helpers.go | 44 ++++-- 3 files changed, 177 insertions(+), 13 deletions(-) create mode 100644 internal/auth/classify_denial_reason_test.go diff --git a/.github/badges/coverage.svg b/.github/badges/coverage.svg index d9f3d122..bfc9c9db 100644 --- a/.github/badges/coverage.svg +++ b/.github/badges/coverage.svg @@ -1 +1 @@ -coverage: 48.7%coverage48.7% \ No newline at end of file +coverage: 49.8%coverage49.8% \ No newline at end of file diff --git a/internal/auth/classify_denial_reason_test.go b/internal/auth/classify_denial_reason_test.go new file mode 100644 index 00000000..3fde4344 --- /dev/null +++ b/internal/auth/classify_denial_reason_test.go @@ -0,0 +1,144 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +import "testing" + +func TestClassifyDenialReason(t *testing.T) { + cases := []struct { + name string + status *CLIAuthStatus + currentChannel string + want string + }{ + { + name: "error CHANNEL_REQUIRED", + status: &CLIAuthStatus{ + ErrorCode: "CHANNEL_REQUIRED", + }, + want: "channel_required", + }, + { + name: "error NO_AUTH", + status: &CLIAuthStatus{ + ErrorCode: "NO_AUTH", + }, + want: "no_auth", + }, + { + name: "success false or nil result → unknown", + status: &CLIAuthStatus{ + Success: false, + }, + want: "unknown", + }, + { + name: "cliAuthEnabled true → no denial", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{CLIAuthEnabled: true}, + }, + want: "", + }, + { + name: "userScope forbidden wins over channel", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{ + CLIAuthEnabled: false, + UserScope: "forbidden", + ChannelScope: "specified", + AllowedChannels: []string{"channel-a"}, + }, + }, + currentChannel: "channel-b", + want: "user_forbidden", + }, + { + // Real-world case reported: user is in allowedUsers but the current + // DWS_CHANNEL is not in allowedChannels. Reason must be channel, + // NOT user. + name: "user allowed but channel not in allowedChannels → channel_not_allowed", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{ + CLIAuthEnabled: false, + UserScope: "specified", + AllowedUsers: []string{"014566033934857460"}, + ChannelScope: "specified", + AllowedChannels: []string{"2a4a658e467998befb7fa333c19ba2b3a3bacfa4"}, + }, + }, + currentChannel: "different-channel", + want: "channel_not_allowed", + }, + { + name: "channelScope specified but current channel empty → channel_required", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{ + CLIAuthEnabled: false, + UserScope: "specified", + ChannelScope: "specified", + AllowedChannels: []string{"channel-a"}, + }, + }, + currentChannel: "", + want: "channel_required", + }, + { + name: "channel matches allowedChannels → fall back to user denial", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{ + CLIAuthEnabled: false, + UserScope: "specified", + ChannelScope: "specified", + AllowedChannels: []string{"channel-a"}, + }, + }, + currentChannel: "channel-a", + want: "user_not_allowed", + }, + { + name: "only userScope=specified, no channel restriction → user_not_allowed", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{ + CLIAuthEnabled: false, + UserScope: "specified", + }, + }, + currentChannel: "", + want: "user_not_allowed", + }, + { + name: "no user or channel restriction → cli_not_enabled", + status: &CLIAuthStatus{ + Success: true, + Result: &CLIAuthResult{CLIAuthEnabled: false}, + }, + want: "cli_not_enabled", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := classifyDenialReason(tc.status, tc.currentChannel) + if got != tc.want { + t.Fatalf("classifyDenialReason() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/auth/oauth_helpers.go b/internal/auth/oauth_helpers.go index d6df1075..372b353a 100644 --- a/internal/auth/oauth_helpers.go +++ b/internal/auth/oauth_helpers.go @@ -22,6 +22,7 @@ import ( "net/http" "net/url" "os" + "slices" "time" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/pkg/config" @@ -1129,15 +1130,26 @@ type CLIAuthStatus struct { // The server computes cliAuthEnabled by considering the org switch, userScope, // and channelScope together; the CLI uses it as-is. type CLIAuthResult struct { - CLIAuthEnabled bool `json:"cliAuthEnabled"` - UserScope string `json:"userScope,omitempty"` // "all" | "specified" | "forbidden" - AllowedUsers []string `json:"allowedUsers,omitempty"` // staffId list when userScope="specified" - ChannelScope string `json:"channelScope,omitempty"` // "all" | "specified" - AllowedChannels []string `json:"allowedChannels,omitempty"` // channelCode list when channelScope="specified" + CLIAuthEnabled bool `json:"cliAuthEnabled"` + UserScope string `json:"userScope,omitempty"` // "all" | "specified" | "forbidden" + AllowedUsers []string `json:"allowedUsers,omitempty"` // staffId list when userScope="specified" + ChannelScope string `json:"channelScope,omitempty"` // "all" | "specified" + AllowedChannels []string `json:"allowedChannels,omitempty"` // channelCode list when channelScope="specified" + ChannelConfigEnabled bool `json:"channelConfigEnabled,omitempty"` // whether org has any channel restriction configured } // classifyDenialReason inspects a CLIAuthStatus response and returns a machine-readable // denial reason string. Returns "" when access is granted. +// +// Priority rationale: +// 1. Explicit org-wide ban (userScope=forbidden) always wins. +// 2. Channel scope is evaluated BEFORE user scope because the CLI has +// authoritative knowledge of DWS_CHANNEL and can verify membership against +// allowedChannels. This avoids falsely blaming the user when the real +// denial cause is a channel mismatch (e.g. user is in allowedUsers but the +// current channel is not in allowedChannels). +// 3. Only when the channel is unrestricted or matches do we attribute the +// denial to the user scope. func classifyDenialReason(status *CLIAuthStatus, currentChannel string) string { if status.ErrorCode == "CHANNEL_REQUIRED" { return "channel_required" @@ -1148,18 +1160,26 @@ func classifyDenialReason(status *CLIAuthStatus, currentChannel string) string { if status.Result == nil || !status.Success { return "unknown" } - if status.Result.CLIAuthEnabled { + r := status.Result + if r.CLIAuthEnabled { return "" } - // cliAuthEnabled=false — infer reason from auxiliary fields (same priority as server) - if status.Result.UserScope == "forbidden" { + + if r.UserScope == "forbidden" { return "user_forbidden" } - if status.Result.UserScope == "specified" { - return "user_not_allowed" + + if r.ChannelScope == "specified" { + if currentChannel == "" { + return "channel_required" + } + if !slices.Contains(r.AllowedChannels, currentChannel) { + return "channel_not_allowed" + } } - if status.Result.ChannelScope == "specified" && currentChannel != "" { - return "channel_not_allowed" + + if r.UserScope == "specified" { + return "user_not_allowed" } return "cli_not_enabled" }