From 0b687296bfd4a5f11b2797c5e323e7185c76ffe4 Mon Sep 17 00:00:00 2001 From: "smalruby3-editor-bot[bot]" <297607354+smalruby3-editor-bot[bot]@users.noreply.github.com> Date: Sat, 10 Oct 2026 06:15:54 +0000 Subject: [PATCH 1/3] fix(classroom): gate Google Classroom share on being a teacher of the course Sharing posts course work with the pressing teacher's own Google token, so Google requires them to be a teacher of the linked course. Smalruby let any class co-teacher press the button, so one who is not in the course hit an opaque `google ClassroomAPI error`. Judge by the fact, not by the Smalruby role: a new GET /classroom-groups/{groupId}/google-course-access probes courses/{id}/teachers/me with the caller's token (200=teacher, 404=notTeacher, anything else=unknown). A GC co-teacher of the same course still passes. The UI disables the share button only on a proven notTeacher and says who to ask; the probe uses an already-granted token only, so it never opens a consent popup. Groups now record googleClassroomLinkedBy {sub,email} on link/re-link (falling back to the class owner for old rows) so the message can name them, and a 404 from courseWork creation maps to the new googleNotCourseTeacher reason instead of googleCourseNotFound. Closes #1184 Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/rules/scratch-gui/e2e-test.md | 2 + docs/classroom/architecture.md | 24 +++ docs/classroom/source-code.md | 1 + docs/classroom/testing.md | 1 + docs/classroom/ui-ux.md | 8 + infra/smalruby-classroom/lambda/handler.ts | 163 ++++++++++++++++-- .../handler-google-course-access.test.ts | 98 +++++++++++ .../smalruby-classroom/lib/classroom-stack.ts | 10 ++ packages/scratch-gui/.prettierignore | 2 + .../classroom-modal/classroom-modal.css | 18 ++ .../classroom-modal/teacher-class-detail.jsx | 32 ++++ .../classroom-teacher-modal.jsx | 2 + .../src/containers/classroom-error-utils.js | 13 ++ .../containers/classroom-teacher-modal.jsx | 1 + .../src/containers/use-google-classroom.js | 57 +++++- .../src/containers/use-teacher-classroom.js | 16 +- packages/scratch-gui/src/lib/classroom-api.js | 19 ++ .../src/lib/classroom-google-share.js | 67 +++++++ .../src/lib/google-classroom-auth.js | 9 + packages/scratch-gui/src/locales/ja-Hira.js | 6 + packages/scratch-gui/src/locales/ja.js | 6 + .../components/teacher-class-detail.test.jsx | 56 ++++++ .../containers/classroom-error-utils.test.js | 19 ++ .../unit/lib/classroom-google-share.test.js | 50 ++++++ 24 files changed, 668 insertions(+), 12 deletions(-) create mode 100644 infra/smalruby-classroom/lambda/tests/handler-google-course-access.test.ts create mode 100644 packages/scratch-gui/src/lib/classroom-google-share.js create mode 100644 packages/scratch-gui/test/unit/lib/classroom-google-share.test.js diff --git a/.claude/rules/scratch-gui/e2e-test.md b/.claude/rules/scratch-gui/e2e-test.md index ba48b17e84e..9e69200ccc9 100644 --- a/.claude/rules/scratch-gui/e2e-test.md +++ b/.claude/rules/scratch-gui/e2e-test.md @@ -131,6 +131,8 @@ expect(await isActiveByTestId(driver, 'ruby-toolbar-mode-dncl')).toBe(true); | `classroom-assignment-name-input` | input | 課題名入力 | | `classroom-create-submit` | button | クラス作成実行（クラス名・人数・課題名 3 つすべて必須） | | `classroom-sidebar-item-{classroomId}` | li | サイドバーのクラス項目（クリックで選択・teacher-class-detail へ） | +| `classroom-post-assignment` | button | Google Classroom に共有（GC コースの教師でないときは disabled） | +| `classroom-post-assignment-blocked` | div | 共有できない理由（誰に依頼すればよいか）#1184 | **先生フロー自動化のコツ**: - `classroom-create-submit` 後は `teacher-class-detail` には**自動遷移しない**。`teacher-dashboard` に戻ってから `classroom-sidebar-item-{id}` を明示的にクリックする必要がある diff --git a/docs/classroom/architecture.md b/docs/classroom/architecture.md index e5e3934c5eb..f314db13d4a 100644 --- a/docs/classroom/architecture.md +++ b/docs/classroom/architecture.md @@ -246,6 +246,7 @@ One Tap が表示できない環境（Windows や Playwright では毎回）で | Method | Path | 説明 | |--------|------|------| | `GET` | `/classrooms/google-courses` | Google Classroom のコース一覧 | +| `GET` | `/classroom-groups/{groupId}/google-course-access` | **押す先生自身が**紐付け先コースの GC 教師かの事前判定（`{courseId, status, linkedByEmail}`。status = `teacher` / `notTeacher` / `notLinked` / `unknown`） | | `POST` | `/classrooms/google-import` | コースをインポートして課題を作成（`Classrooms` に書き込む） | | `POST` | `/classrooms/{id}/google-assignment` | Google Classroom に課題を投稿 | @@ -260,11 +261,33 @@ Google Classroom API の失敗は、Google が返した `error.status` / `error. | 401 | 401 | `googleTokenExpired` | Google のアクセストークンが失効 | | 403 | 403 | `googlePermissionDenied` | 権限（スコープ）不足 / そのコースの先生ではない | | 404 | 404 | `googleCourseNotFound` | ひもづけたコースが削除された / そのコースにアクセスできない | +| 404（課題配信時） | 404 | `googleNotCourseTeacher` | 押した先生がそのコースの GC 教師ではない（Google は未参加のコースを 404 で隠すため） | | 400 | 400 | `googleInvalidRequest` | Google がリクエストを拒否（`FAILED_PRECONDITION` 等を含む） | | その他 | 502 | `googleApiError` | レート超過・Google 側障害など | レスポンス本文: `{error, reason, googleStatus?, googleMessage?}`。 +#### 共有できる先生の判定（GC 教師かどうか） + +課題配信は **押した先生自身の Google アクセストークン**で `courseWork` を作成するため、 +Google はその先生がそのコースの教師であることを要求する。Smalruby のクラス共同担当は +Smalruby 上の権限であって GC の教師とは別物なので、両者が食い違うと Google 側で失敗する。 + +判定は Smalruby 上の役割（owner / co-teacher）ではなく **実態**で行う: +`GET /classroom-groups/{groupId}/google-course-access` が `courses/{courseId}/teachers/me` を +caller のトークンで引き、200 なら `teacher`、404 なら `notTeacher` を返す。401/403/5xx などは +`unknown`（判定不能）で、UI は**共有ボタンを無効化しない**。同じコースに GC 共同教師として +入っている先生はこの判定を通るため、従来どおり共有できる。 + +クライアントは**すでに取得済みの Google アクセストークンがあるときだけ**この判定を行う +（判定のために同意ポップアップを出さない）。判定できなかった場合は従来どおり押させ、 +失敗時に `googleNotCourseTeacher` の文言で同じ趣旨を伝える。 + +誰が紐付けたかは group の `googleClassroomLinkedBy`（`{sub, email}`）に記録し、 +`GET /classroom-groups` は表示用に `googleClassroomLinkedByEmail` を返す。この記録が無い +既存データはクラス作成者（`teacherSub`）にフォールバックし、メールが分からないときは +「連携した先生」と一般名詞で表示する（推測で名前を出さない）。 + クライアントが取得するアクセストークンは `classroom.coursework.students` を含む 3 スコープを要求するが、 Google の同意画面はスコープごとにチェックボックスを出すため**一部だけ許可される**ことがある。 クライアントは `hasGrantedAllScopes` で検証し、不足していれば `prompt: 'consent'` で再要求する。 @@ -428,6 +451,7 @@ erDiagram number schemaVersion "データモデル版 (v2 = クラス→課題モデル)" string_array topics "課題のトピック一覧 (最大20件)" string googleClassroomCourseId "GC コース紐づけ (クラス単位, 任意)" + map googleClassroomLinkedBy "GC を紐付けた先生 {sub, email} (任意)" number studentCount "人数 (課題の座席数の真実, 任意)" string_array coTeacherEmails "クラス単位の共同管理者 (任意)" string createdAt "ISO8601" diff --git a/docs/classroom/source-code.md b/docs/classroom/source-code.md index bd5e47243cf..49bb4812915 100644 --- a/docs/classroom/source-code.md +++ b/docs/classroom/source-code.md @@ -55,6 +55,7 @@ smalruby3-editor/ │ │ │ ├── classroom-api.js ← API クライアント (リトライ付き) │ │ │ ├── classroom-assignment-utils.js ← 課題エディタの純粋ヘルパー │ │ │ ├── classroom-group-utils.js ← サイドバー組階層の純粋ヘルパー +│ │ │ ├── classroom-google-share.js ← GC 共有可否の判定 + 文言（純粋）#1184 │ │ │ ├── classroom-evaluation/ ← sb3-analyzer（静的解析）+ evaluation-utils（総合評価/CSV） │ │ │ ├── classroom-kick-request-storage.js ← 退室依頼の localStorage 永続化 (#692) │ │ │ ├── google-classroom-auth.js ← Google Classroom OAuth diff --git a/docs/classroom/testing.md b/docs/classroom/testing.md index 8d547007c1c..c5d85109d21 100644 --- a/docs/classroom/testing.md +++ b/docs/classroom/testing.md @@ -151,6 +151,7 @@ Playwright MCP および Selenium integration tests で使用する `data-testid | data-testid | 要素 | 説明 | |------------|------|------| | `classroom-post-assignment` | button | 課題配信ボタン (詳細画面) | +| `classroom-post-assignment-blocked` | div | 共有できない理由 (GC コースの教師ではない共同担当) | | `classroom-post-assignment-title` | input | タイトル | | `classroom-post-assignment-description` | textarea | 説明 | | `classroom-post-assignment-submit` | button | 配信実行 | diff --git a/docs/classroom/ui-ux.md b/docs/classroom/ui-ux.md index f0468ab3f7a..f1d4e1b6ef1 100644 --- a/docs/classroom/ui-ux.md +++ b/docs/classroom/ui-ux.md @@ -243,6 +243,7 @@ Google または Microsoft アカウントでサインインする画面。先 | クラス（学級）名 | 「技術 2026年度 / 2年1組」 | `classroom-detail-name` | 課題が属するクラスの表示ラベル（`formatClassLabel(group)`。v2 以前のデータでは `Classrooms.className`） | | 課題名入力 | 課題名（編集可） | `classroom-detail-assignment-name` | blur で保存 | | 課題配信ボタン | 「課題を配信」 | `classroom-post-assignment` | → teacher-post-assignment。**未配信のとき**表示 | +| 共有できない理由 | 「このクラスの Google Classroom は ◯◯ 先生が連携しています。…」 | `classroom-post-assignment-blocked` | GC コースの教師ではないときだけ表示（配信ボタンは `disabled`） | | 課題確認リンク | 「課題を確認」 | `classroom-view-assignment` | 配信済みのとき表示（新しいタブ） | | 参加コード表示 | 「参加コード: 3cexm5」 | `classroom-detail-join-code` | 大きなフォントで中央表示 | | コード拡大ボタン | ⛶ アイコン（ツールチップ: 「全画面表示」） | `classroom-detail-expand-code` | 全画面コード表示 | @@ -258,6 +259,13 @@ Google または Microsoft アカウントでサインインする画面。先 **課題配信ボタンの表示条件:** Google Classroom 連携はクラス（group）単位に移行したため、配信ボタン（`classroom-post-assignment`）は **クラスが GC 連携済み（`group.googleClassroomCourseId`）** であれば、その課題自体に courseId が無くても表示されます（課題の投稿先はクラスのコース）。課題単位の `googleClassroomCourseId`（v2 以前のフォールバック）が有る場合も表示されます。どちらの courseId も無い（非連携クラス）ときは表示されません。配信済み（課題に `googleClassroomAlternateLink` が保存済み）になると「Google Classroom で確認」リンク（`classroom-view-assignment`）に切り替わります。 +**課題配信ボタンを押せる条件 (#1184):** 配信は**押した先生自身の Google アカウント**で行われるため、 +紐付け先の Google Classroom コースの教師でなければ Google に拒否されます。事前判定 +（`GET /classroom-groups/{groupId}/google-course-access`）で「そのコースの教師ではない」と分かった +ときだけボタンを `disabled` にし、`classroom-post-assignment-blocked` に誰に依頼すればよいかを +表示します。判定できなかったとき（Google のトークンをまだ持っていない等）はボタンを無効化せず、 +失敗時のメッセージで同じ趣旨を伝えます（判定のために同意ポップアップは出しません）。 + **座席グリッド:** 座席番号が格子状に並び（1行 10列）、各セルの背景色で状態を表します。 diff --git a/infra/smalruby-classroom/lambda/handler.ts b/infra/smalruby-classroom/lambda/handler.ts index 0de77f74d02..22180ceebbe 100644 --- a/infra/smalruby-classroom/lambda/handler.ts +++ b/infra/smalruby-classroom/lambda/handler.ts @@ -353,12 +353,16 @@ class GoogleAPIError extends Error { // opaque "Google Classroom API error" the teacher used to see (issue #1175). googleStatus: string; googleMessage: string; - constructor(statusCode: number, message: string, googleStatus = '', googleMessage = '') { + // Which of our calls failed, so the same HTTP status can be mapped to + // different reasons (see googleApiErrorResponse). + operation: string; + constructor(statusCode: number, message: string, googleStatus = '', googleMessage = '', operation = '') { super(message); this.name = 'GoogleAPIError'; this.statusCode = statusCode; this.googleStatus = googleStatus; this.googleMessage = googleMessage; + this.operation = operation; } } @@ -426,16 +430,27 @@ export function parseGoogleApiErrorBody(raw: string): { googleStatus: string; go * from a malformed request (400). The Google-provided status and message are * passed through so the UI can show an actionable message, and `reason` gives * the UI a stable key to translate. + * + * `operation` names the call that failed. Google hides a course the caller is + * not in behind a 404, so a 404 from `courseWork` creation does not mean "the + * course is gone" — it means the teacher who pressed the button is not a + * teacher of that course (issue #1184). That case gets its own reason so the + * UI can say who to ask instead of blaming the link. */ export function googleApiErrorResponse( statusCode: number, googleStatus: string, googleMessage: string, + operation = '', ): { statusCode: number; body: string } { let httpStatus: number; let reason: string; let error: string; - if (statusCode === 401) { + if (statusCode === 404 && operation === 'postCourseWork') { + httpStatus = 404; + reason = 'googleNotCourseTeacher'; + error = 'You are not a teacher of the linked Google Classroom course.'; + } else if (statusCode === 401) { httpStatus = 401; reason = 'googleTokenExpired'; error = 'Google access token expired. Please re-authorize.'; @@ -472,6 +487,7 @@ async function callGoogleClassroomAPI( path: string, method: string = 'GET', body?: unknown, + operation = '', ): Promise { const url = `https://classroom.googleapis.com/v1/${path}`; const response = await fetch(url, { @@ -499,6 +515,7 @@ async function callGoogleClassroomAPI( `Google Classroom API error: ${response.status}`, googleStatus, googleMessage, + operation, ); } return response.json(); @@ -2200,6 +2217,107 @@ async function handleUpdateSubmission( // --- Google Classroom handlers --- +/** + * Snapshot of the teacher who linked a Google Classroom course to a class. + * + * Before issue #1184 only the course id was stored, so when a co-teacher hit + * Google's "you are not a teacher of this course" wall there was no way to tell + * them who to ask. `email` is kept for display (it is the same address the + * owner already types when inviting co-teachers) and may be null when the + * identity provider supplies no email claim. + */ +export interface GoogleClassroomLinkedBy { + sub: string; + email: string | null; +} + +/** + * Build the attribution to store when a class gets linked to a GC course. + * @param identity - the teacher performing the link + * @returns the attribution to persist on the group item + */ +export function buildGoogleClassroomLinkedBy(identity: TeacherIdentity): GoogleClassroomLinkedBy { + return { + sub: identity.sub, + email: identity.email ? normalizeEmail(identity.email) : null, + }; +} + +/** + * Read the attribution back off a group item. + * + * Classes linked before #1184 carry no attribution, so fall back to the class + * owner — they are the only teacher who could have linked it under the old + * flow. The email is unknown in that case; the UI then names the linker + * generically instead of guessing. + * @param item - the group item (may be undefined) + * @returns the attribution, or null when there is nothing to attribute to + */ +export function readGoogleClassroomLinkedBy( + item: Record | undefined, +): GoogleClassroomLinkedBy | null { + const stored = item?.googleClassroomLinkedBy; + if (stored && typeof stored === 'object') { + const { sub, email } = stored as { sub?: unknown; email?: unknown }; + if (typeof sub === 'string' && sub) { + return { sub, email: typeof email === 'string' && email ? email : null }; + } + } + const owner = item?.teacherSub; + if (typeof owner === 'string' && owner) { + return { sub: owner, email: null }; + } + return null; +} + +/** + * Decide what a failed "am I a teacher of this course?" probe proves. + * + * Google answers 404 for a course the caller is not a member of (it hides the + * existence of other people's courses), so only that status proves "not a + * teacher". An expired token, a missing scope or a Google outage prove nothing + * — those stay `unknown` so the UI leaves the share button enabled rather than + * blocking a teacher who is in fact allowed (issue #1184). + * @param statusCode - the HTTP status Google answered with + * @returns 'notTeacher' when proven, 'unknown' otherwise + */ +export function classifyCourseTeacherFailure(statusCode: number): 'notTeacher' | 'unknown' { + return statusCode === 404 ? 'notTeacher' : 'unknown'; +} + +/** + * Report whether the caller can post course work to the class's linked GC + * course, so the UI can disable the share button before the teacher hits a + * Google error (issue #1184). + * + * Probes `courses/{id}/teachers/me`, which is cheap and answers exactly the + * question Google enforces on courseWork creation: a GC co-teacher of the same + * course passes, a Smalruby co-teacher who is not in the course does not. + */ +async function handleGetGoogleCourseAccess( + identity: TeacherIdentity, + accessToken: string, + groupId: string, +): Promise { + const group = await getManageableGroup(identity, groupId); + const courseId = typeof group.googleClassroomCourseId === 'string' ? group.googleClassroomCourseId : ''; + const linkedBy = readGoogleClassroomLinkedBy(group); + const linkedByEmail = linkedBy && linkedBy.sub !== identity.sub ? linkedBy.email : null; + if (!courseId) { + return { statusCode: 200, body: JSON.stringify({ courseId: null, status: 'notLinked', linkedByEmail: null }) }; + } + + let status: 'teacher' | 'notTeacher' | 'unknown'; + try { + await callGoogleClassroomAPI(accessToken, `courses/${courseId}/teachers/me`); + status = 'teacher'; + } catch (err) { + if (!(err instanceof GoogleAPIError)) throw err; + status = classifyCourseTeacherFailure(err.statusCode); + } + return { statusCode: 200, body: JSON.stringify({ courseId, status, linkedByEmail }) }; +} + async function handleListGoogleCourses(accessToken: string): Promise { const data = await callGoogleClassroomAPI(accessToken, 'courses?teacherId=me&courseStates=ACTIVE&pageSize=100') as { courses?: Array<{ id: string; name: string; section?: string }>; @@ -2392,13 +2510,19 @@ async function handlePostAssignment( } const description = typeof body.description === 'string' ? body.description.trim() : ''; - const courseWork = await callGoogleClassroomAPI(accessToken, `courses/${courseId}/courseWork`, 'POST', { - title: title.trim(), - description: description || undefined, - workType: 'ASSIGNMENT', - state: 'PUBLISHED', - materials: [{ link: { url: link, title: 'スモウルビーで開く' } }], - }) as { id: string; alternateLink: string }; + const courseWork = await callGoogleClassroomAPI( + accessToken, + `courses/${courseId}/courseWork`, + 'POST', + { + title: title.trim(), + description: description || undefined, + workType: 'ASSIGNMENT', + state: 'PUBLISHED', + materials: [{ link: { url: link, title: 'スモウルビーで開く' } }], + }, + 'postCourseWork', + ) as { id: string; alternateLink: string }; // Persist courseWork info to prevent duplicate posting await docClient.send(new UpdateCommand({ @@ -2784,6 +2908,11 @@ function mapGroupSummary(item: Record) { schemaVersion: typeof item.schemaVersion === 'number' ? item.schemaVersion : 1, topics: Array.isArray(item.topics) ? item.topics : [], googleClassroomCourseId: item.googleClassroomCourseId || null, + // Who linked the GC course (#1184). Only the email is exposed — enough for + // "ask ◯◯ sensei" and the same address co-teachers are invited by. + googleClassroomLinkedByEmail: item.googleClassroomCourseId + ? (readGoogleClassroomLinkedBy(item)?.email ?? null) + : null, studentCount: typeof item.studentCount === 'number' ? item.studentCount : null, coTeacherEmails: readCoTeacherEmails(item), createdAt: item.createdAt, @@ -2820,6 +2949,10 @@ async function handleCreateGroup(identity: TeacherIdentity, body: Record { + const parse = (res: { statusCode: number; body: string }) => JSON.parse(res.body); + + test('404 while posting course work means the caller is not a teacher of the course', () => { + const res = googleApiErrorResponse(404, 'NOT_FOUND', 'Requested entity was not found.', 'postCourseWork'); + expect(res.statusCode).toBe(404); + const body = parse(res); + expect(body.reason).toBe('googleNotCourseTeacher'); + expect(body.googleStatus).toBe('NOT_FOUND'); + }); + + test('403 while posting course work keeps the permission reason', () => { + const res = googleApiErrorResponse(403, 'PERMISSION_DENIED', 'nope', 'postCourseWork'); + expect(parse(res).reason).toBe('googlePermissionDenied'); + }); + + test('404 without an operation stays the generic course-not-found reason', () => { + expect(parse(googleApiErrorResponse(404, 'NOT_FOUND', '')).reason).toBe('googleCourseNotFound'); + }); + + test('an unrelated operation does not hijack the 404 reason', () => { + expect(parse(googleApiErrorResponse(404, 'NOT_FOUND', '', 'listCourses')).reason).toBe( + 'googleCourseNotFound', + ); + }); +}); + +describe('buildGoogleClassroomLinkedBy', () => { + test('records who linked the course so the UI can name them later', () => { + expect(buildGoogleClassroomLinkedBy({ sub: 'sub-1', email: 'teacher@example.com' })).toEqual({ + sub: 'sub-1', + email: 'teacher@example.com', + }); + }); + + test('normalizes the email so it matches the co-teacher list', () => { + expect(buildGoogleClassroomLinkedBy({ sub: 'sub-1', email: ' Teacher@Example.COM ' })).toEqual({ + sub: 'sub-1', + email: 'teacher@example.com', + }); + }); + + test('tolerates a provider that supplies no email', () => { + expect(buildGoogleClassroomLinkedBy({ sub: 'sub-1', email: null })).toEqual({ + sub: 'sub-1', + email: null, + }); + }); +}); + +describe('readGoogleClassroomLinkedBy', () => { + test('returns the stored linker', () => { + const item = { + teacherSub: 'owner-sub', + googleClassroomLinkedBy: { sub: 'linker-sub', email: 'linker@example.com' }, + }; + expect(readGoogleClassroomLinkedBy(item)).toEqual({ sub: 'linker-sub', email: 'linker@example.com' }); + }); + + test('falls back to the class owner for rows written before the field existed', () => { + expect(readGoogleClassroomLinkedBy({ teacherSub: 'owner-sub' })).toEqual({ + sub: 'owner-sub', + email: null, + }); + }); + + test('ignores a malformed stored value', () => { + expect(readGoogleClassroomLinkedBy({ teacherSub: 'owner-sub', googleClassroomLinkedBy: 'nope' })).toEqual({ + sub: 'owner-sub', + email: null, + }); + }); + + test('returns null when there is nothing to attribute the link to', () => { + expect(readGoogleClassroomLinkedBy({})).toBeNull(); + expect(readGoogleClassroomLinkedBy(undefined)).toBeNull(); + }); +}); + +describe('classifyCourseTeacherFailure', () => { + // Google hides courses the caller is not in behind a 404, so that is the only + // status that proves "not a teacher". Everything else (expired token, missing + // scope, Google outage) is inconclusive and must not disable the button. + test('404 means the caller is not a teacher of the course', () => { + expect(classifyCourseTeacherFailure(404)).toBe('notTeacher'); + }); + + test.each([401, 403, 429, 500, 502])('%i is inconclusive', (status) => { + expect(classifyCourseTeacherFailure(status)).toBe('unknown'); + }); +}); diff --git a/infra/smalruby-classroom/lib/classroom-stack.ts b/infra/smalruby-classroom/lib/classroom-stack.ts index f402b685425..e1200de4027 100644 --- a/infra/smalruby-classroom/lib/classroom-stack.ts +++ b/infra/smalruby-classroom/lib/classroom-stack.ts @@ -821,6 +821,16 @@ export class ClassroomStack extends cdk.Stack { integration, }); + // "Can I post course work to this class's linked Google Classroom course?" + // — asked before enabling the share button so a co-teacher who is not a + // teacher of that course is told who to ask instead of hitting a Google + // error (#1184). + this.api.addRoutes({ + path: '/classroom-groups/{groupId}/google-course-access', + methods: [apigatewayv2.HttpMethod.GET], + integration, + }); + // Topic list management (add/remove/rename with cascade to assignments). this.api.addRoutes({ path: '/classroom-groups/{groupId}/topics', diff --git a/packages/scratch-gui/.prettierignore b/packages/scratch-gui/.prettierignore index 7a6fa74bc2e..ec4e2240855 100644 --- a/packages/scratch-gui/.prettierignore +++ b/packages/scratch-gui/.prettierignore @@ -151,6 +151,7 @@ src/lib/* !src/lib/classroom-assignment-templates/ !src/lib/classroom-evaluation/ !src/lib/classroom-download-utils.js +!src/lib/classroom-google-share.js !src/lib/classroom-group-utils.js !src/lib/classroom-retention.js !src/lib/shared-assignment-taxonomy.js @@ -398,6 +399,7 @@ test/unit/lib/* !test/unit/lib/classroom-download-utils.test.js !test/unit/lib/classroom-group-utils.test.js !test/unit/lib/google-classroom-auth.test.js +!test/unit/lib/classroom-google-share.test.js !test/unit/lib/classroom-retention.test.js !test/unit/lib/shared-assignment-taxonomy.test.js !test/unit/lib/shared-author-profile.test.js diff --git a/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css b/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css index 17d924ab292..5e11f67eeb2 100644 --- a/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css +++ b/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css @@ -357,6 +357,24 @@ background-color: #d6e7ff; } +/* GC コースの教師ではないため共有できないときのボタン表示 (#1184)。 + 強調色をやめ、押せないことが見て分かるようにする。 */ +.join-code-action-gc:disabled { + border-color: #c8cdd8; + background-color: #f2f3f7; + color: #9aa0b0; + cursor: not-allowed; +} + +/* 共有できない理由。参加コードカードの中で 1 行占有して折り返す。 */ +.join-code-gc-blocked-note { + flex: 0 0 100%; + margin-top: 0.3rem; + color: #575e75; + font-size: 0.8rem; + line-height: 1.4; +} + /* コピー完了のリアクション: アイコンがチェックに変わり緑で強調 */ .join-code-action-copied { border-color: #4caf50; diff --git a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx index 9d6df00f698..dcc72a72eec 100644 --- a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx +++ b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx @@ -9,6 +9,7 @@ import TeacherAssignmentEditor from './teacher-assignment-editor.jsx'; import TeacherMemberDetail from './teacher-member-detail.jsx'; import { formatClassLabel } from '../../lib/classroom-class-label.js'; +import { isGoogleShareBlocked, googleShareBlockedMessage } from '../../lib/classroom-google-share.js'; import { retentionLevel } from '../../lib/classroom-retention.js'; import googleClassroomIcon from '../classroom-teacher-modal/google-classroom-icon.png'; import styles from './classroom-modal.css'; @@ -47,6 +48,7 @@ const TeacherClassDetail = ({ onOpenShareSuggestion, assignmentEditor, group, + googleCourseAccess, }) => { // Destructured with handle-prefixed names for the embedded editor // (react/jsx-handler-names requires handler props to look like handlers). @@ -168,6 +170,20 @@ const TeacherClassDetail = ({ // next to the deadline and the urgent styling on the download button. const retention = retentionLevel(selectedClassroom && selectedClassroom.expiresAt); + // Google Classroom share is posted with this teacher's own Google account, + // so a class co-teacher who is not a teacher of the linked course cannot + // share it. Disable the button with the reason instead of letting Google + // answer with an opaque error (#1184). + const gcShareBlocked = isGoogleShareBlocked(googleCourseAccess); + const gcShareBlockedText = gcShareBlocked + ? googleShareBlockedMessage( + intl, + googleCourseAccess.linkedByEmail || + (group && group.googleClassroomLinkedByEmail) || + null, + ) + : ''; + const handleShowCode = useCallback(() => { setShowCodeDisplay(true); onShowCodeDisplay(selectedClassroom.classroomId); @@ -395,6 +411,8 @@ const TeacherClassDetail = ({ ))} + {/* 共有できない理由（GC コースの教師ではない共同担当） + をボタンの近くに出す。#1184 */} + {gcShareBlocked && ( +
+ {gcShareBlockedText} +
+ )} {/* 保存期限とその警告を1行に統合（issue #1052/#1049）: @@ -885,6 +913,10 @@ TeacherClassDetail.propTypes = { onOpenShareSuggestion: PropTypes.func, assignmentEditor: PropTypes.object, group: PropTypes.object, + googleCourseAccess: PropTypes.shape({ + status: PropTypes.string, + linkedByEmail: PropTypes.string, + }), selectedClassroom: PropTypes.object.isRequired, selectedMember: PropTypes.string, }; diff --git a/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx b/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx index 35cbe0694dd..1690a9f242f 100644 --- a/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx +++ b/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx @@ -92,6 +92,7 @@ const ClassroomTeacherModal = ({ containerProps, onClose }) => { codeDisplayFullscreen, downloadProgress, googleCourses, + googleCourseAccess, selectedGoogleCourse, googleSignInRef, googleFallbackReason, @@ -340,6 +341,7 @@ const ClassroomTeacherModal = ({ containerProps, onClose }) => { errorActionHandler={errorActionHandler} errorActionLabel={errorActionLabel} errorTitle={errorTitle} + googleCourseAccess={googleCourseAccess} group={selectedGroup} isLoading={isLoading} kickRequestsBySeat={kickRequestsBySeat} diff --git a/packages/scratch-gui/src/containers/classroom-error-utils.js b/packages/scratch-gui/src/containers/classroom-error-utils.js index 291d8a46a3e..2e14cbe4c15 100644 --- a/packages/scratch-gui/src/containers/classroom-error-utils.js +++ b/packages/scratch-gui/src/containers/classroom-error-utils.js @@ -31,6 +31,19 @@ const translateGoogleClassroomError = (intl, err) => { id: 'gui.classroom.error.googlePermissionDenied', }); } + if (reason === 'googleNotCourseTeacher') { + // Google answers 404 for a course the caller is not in, so this is the + // co-teacher case from issue #1184 — not a missing class. Name the + // cause and the way out (ask the teacher who linked it); the linking + // teacher's address is not in the error, so stay generic here. + return intl.formatMessage({ + defaultMessage: + 'You are not a teacher of the linked Google Classroom class, so it cannot be shared ' + + 'from your account. Please ask the teacher who linked it to share it.', + description: 'Error when the signed-in teacher is not a teacher of the linked Google Classroom course', + id: 'gui.classroom.error.googleNotCourseTeacher', + }); + } if (reason === 'googleCourseNotFound') { return intl.formatMessage({ defaultMessage: diff --git a/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx b/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx index 7c1e1442b9d..37bec45dc5f 100644 --- a/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx +++ b/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx @@ -138,6 +138,7 @@ const ClassroomTeacherModal = () => { codeDisplayFullscreen: teacher.codeDisplayFullscreen, downloadProgress: teacher.downloadProgress, googleCourses: teacher.googleCourses, + googleCourseAccess: teacher.googleCourseAccess, selectedGoogleCourse: teacher.selectedGoogleCourse, googleSignInRef: teacher.googleSignInRef, googleFallbackReason: teacher.googleFallbackReason, diff --git a/packages/scratch-gui/src/containers/use-google-classroom.js b/packages/scratch-gui/src/containers/use-google-classroom.js index 880c096db24..f33cc4aa711 100644 --- a/packages/scratch-gui/src/containers/use-google-classroom.js +++ b/packages/scratch-gui/src/containers/use-google-classroom.js @@ -5,7 +5,12 @@ */ import { useCallback, useState } from 'react'; import classroomAPI from '../lib/classroom-api.js'; -import { requestClassroomAccessToken, clearClassroomAccessToken } from '../lib/google-classroom-auth.js'; +import { GOOGLE_COURSE_ACCESS } from '../lib/classroom-google-share.js'; +import { + requestClassroomAccessToken, + clearClassroomAccessToken, + getCachedClassroomAccessToken, +} from '../lib/google-classroom-auth.js'; import translateError from './classroom-error-utils.js'; /** @@ -37,6 +42,54 @@ const useGoogleClassroom = ({ const [googleAccessToken, setGoogleAccessToken] = useState(null); const [googleCourses, setGoogleCourses] = useState([]); const [selectedGoogleCourse, setSelectedGoogleCourse] = useState(null); + // Pre-flight for the share button: {groupId, status, linkedByEmail}. + const [googleCourseAccess, setGoogleCourseAccess] = useState(null); + + /** + * Find out whether this teacher can post to the class's linked Google + * Classroom course, so the share button can be disabled with an + * explanation instead of failing inside Google (issue #1184). + * + * Deliberately silent: it uses a Google token the teacher already granted + * and gives up (leaving the button enabled) when there is none, so the + * check never triggers a consent popup on its own. + * @param {?object} group - the class (group) whose link to check + * @returns {Promise} resolves once the state has been updated + */ + const handleCheckGoogleCourseAccess = useCallback( + async (group) => { + if (!group || !group.groupId || !group.googleClassroomCourseId) { + setGoogleCourseAccess(null); + return; + } + const accessToken = googleAccessToken || getCachedClassroomAccessToken(); + if (!accessToken) { + setGoogleCourseAccess({ + groupId: group.groupId, + status: GOOGLE_COURSE_ACCESS.UNKNOWN, + linkedByEmail: group.googleClassroomLinkedByEmail || null, + }); + return; + } + try { + const data = await classroomAPI.getGoogleCourseAccess(idToken, accessToken, group.groupId); + setGoogleCourseAccess({ + groupId: group.groupId, + status: data.status || GOOGLE_COURSE_ACCESS.UNKNOWN, + linkedByEmail: data.linkedByEmail || group.googleClassroomLinkedByEmail || null, + }); + } catch { + // A failed probe must not block sharing, and must not raise an + // error banner either — nothing the teacher did has failed yet. + setGoogleCourseAccess({ + groupId: group.groupId, + status: GOOGLE_COURSE_ACCESS.UNKNOWN, + linkedByEmail: group.googleClassroomLinkedByEmail || null, + }); + } + }, + [idToken, googleAccessToken], + ); const handleShowGoogleCourses = useCallback(() => { clearError(); @@ -162,6 +215,8 @@ const useGoogleClassroom = ({ return { googleCourses, + googleCourseAccess, + handleCheckGoogleCourseAccess, selectedGoogleCourse, handleShowGoogleCourses, handleLoadGoogleCourses, diff --git a/packages/scratch-gui/src/containers/use-teacher-classroom.js b/packages/scratch-gui/src/containers/use-teacher-classroom.js index 7eef581d9f5..8c82f5728a4 100644 --- a/packages/scratch-gui/src/containers/use-teacher-classroom.js +++ b/packages/scratch-gui/src/containers/use-teacher-classroom.js @@ -5,7 +5,7 @@ * {@link useTeacherSubmissions}, and {@link useGoogleClassroom} into * a single return object consumed by {@link ClassroomModal}. */ -import { useCallback, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import useGoogleClassroom from './use-google-classroom.js'; import useSharedAssignments from './use-shared-assignments.js'; import useTeacherAssignment from './use-teacher-assignment.js'; @@ -126,6 +126,19 @@ const useTeacherClassroom = ({ importCourseRef.current = groups.handleCreateClassFromCourse; + // Re-run the Google Classroom share pre-flight whenever the selected class + // changes, so the share button in its assignments is already disabled (with + // the reason) for a co-teacher who is not a teacher of the linked GC course + // (#1184). Silent — it only uses an access token the teacher already granted. + const { handleCheckGoogleCourseAccess } = google; + const selectedGroupId = groups.selectedGroup && groups.selectedGroup.groupId; + useEffect(() => { + handleCheckGoogleCourseAccess(groups.selectedGroup); + // groups.selectedGroup is intentionally not a dependency: it is a fresh + // object on every list refresh, and keying on its id keeps this to one + // probe per class selection. + }, [selectedGroupId, handleCheckGoogleCourseAccess]); + const evaluation = useTeacherEvaluation({ idToken: auth.idToken, classrooms: classrooms.classrooms, @@ -332,6 +345,7 @@ const useTeacherClassroom = ({ // Google Classroom googleCourses: google.googleCourses, + googleCourseAccess: google.googleCourseAccess, selectedGoogleCourse: google.selectedGoogleCourse, handleShowGoogleCourses: google.handleShowGoogleCourses, handleLoadGoogleCourses: google.handleLoadGoogleCourses, diff --git a/packages/scratch-gui/src/lib/classroom-api.js b/packages/scratch-gui/src/lib/classroom-api.js index 759c5c691cc..6bbe42c03d7 100644 --- a/packages/scratch-gui/src/lib/classroom-api.js +++ b/packages/scratch-gui/src/lib/classroom-api.js @@ -557,6 +557,25 @@ class ClassroomAPI { return this._request('GET', '/classrooms/google-courses', null, idToken, googleAccessToken); } + /** + * Ask whether the signed-in teacher may post course work to the Google + * Classroom course linked to this class (issue #1184). Never prompts for + * consent: the caller passes a token it already holds. + * @param {string} idToken - Teacher ID token + * @param {string} googleAccessToken - Google access token with Classroom scopes + * @param {string} groupId - Class (group) ID + * @returns {Promise} {courseId, status, linkedByEmail} + */ + async getGoogleCourseAccess(idToken, googleAccessToken, groupId) { + return this._request( + 'GET', + `/classroom-groups/${groupId}/google-course-access`, + null, + idToken, + googleAccessToken, + ); + } + /** * Import a Google Classroom course as a Smalruby classroom. * @param {string} idToken - Google ID token diff --git a/packages/scratch-gui/src/lib/classroom-google-share.js b/packages/scratch-gui/src/lib/classroom-google-share.js new file mode 100644 index 00000000000..fef06b776ea --- /dev/null +++ b/packages/scratch-gui/src/lib/classroom-google-share.js @@ -0,0 +1,67 @@ +/** + * Sharing a Smalruby assignment to Google Classroom posts course work with the + * **pressing** teacher's Google token, and Google only accepts that from a + * teacher of the course. Smalruby lets any class co-teacher press the button, + * so a co-teacher who is not in the Google Classroom course used to hit a raw + * `Google Classroom API error` (issue #1184). + * + * These helpers turn the server's pre-flight answer into "disable the button?" + * plus the sentence that explains who to ask. + */ +import { defineMessages } from 'react-intl'; + +/** + * Result of the "is the signed-in teacher a teacher of the linked course?" + * probe (`GET /classroom-groups/{groupId}/google-course-access`). + * @enum {string} + */ +const GOOGLE_COURSE_ACCESS = { + /** The teacher is a teacher of the linked course — sharing works. */ + TEACHER: 'teacher', + /** Google proved they are not — sharing would fail. */ + NOT_TEACHER: 'notTeacher', + /** The class has no Google Classroom course linked. */ + NOT_LINKED: 'notLinked', + /** The probe could not be run or was inconclusive. */ + UNKNOWN: 'unknown', +}; + +const messages = defineMessages({ + blockedBy: { + defaultMessage: "This class's Google Classroom was linked by {email}. Please ask them to share it.", + description: 'Explains that only the teacher who linked the Google Classroom course can share', + id: 'gui.classroom.googleShare.blockedBy', + }, + blocked: { + defaultMessage: "This class's Google Classroom was linked by another teacher. Please ask them to share it.", + description: + 'Explains that only the teacher who linked the Google Classroom course can share, ' + + 'when that teacher cannot be named', + id: 'gui.classroom.googleShare.blocked', + }, +}); + +/** + * Should the "share to Google Classroom" button be disabled? + * + * Only a proven `notTeacher` blocks. Anything else — including "we could not + * tell", which is the normal state before the teacher has granted Google + * access this session — leaves the button enabled so a teacher who is in fact + * allowed is never locked out; the failure path then explains the same thing. + * @param {?object} access - the probe result, or null when it has not run + * @returns {boolean} true when sharing is known to be impossible + */ +const isGoogleShareBlocked = (access) => Boolean(access) && access.status === GOOGLE_COURSE_ACCESS.NOT_TEACHER; + +/** + * The sentence shown instead of letting the teacher press the button. + * @param {object} intl - react-intl intl object + * @param {?string} linkedByEmail - email of the teacher who linked the course + * @returns {string} localized explanation naming who to ask, when known + */ +const googleShareBlockedMessage = (intl, linkedByEmail) => + linkedByEmail + ? intl.formatMessage(messages.blockedBy, { email: linkedByEmail }) + : intl.formatMessage(messages.blocked); + +export { GOOGLE_COURSE_ACCESS, isGoogleShareBlocked, googleShareBlockedMessage }; diff --git a/packages/scratch-gui/src/lib/google-classroom-auth.js b/packages/scratch-gui/src/lib/google-classroom-auth.js index c250240bfa8..f05d2697309 100644 --- a/packages/scratch-gui/src/lib/google-classroom-auth.js +++ b/packages/scratch-gui/src/lib/google-classroom-auth.js @@ -179,6 +179,15 @@ export const requestClassroomAccessToken = async () => { throw _permissionDeniedError(); }; +/** + * Return the cached access token, or null when there is none that is still + * valid. Never contacts Google and never opens a consent popup — used by + * checks that must stay invisible, such as the pre-flight that decides whether + * the Google Classroom share button can work (issue #1184). + * @returns {?string} Cached access token, or null. + */ +export const getCachedClassroomAccessToken = () => (_isTokenValid() ? _accessToken : null); + /** * Clear the cached access token (e.g., on 401 from backend). */ diff --git a/packages/scratch-gui/src/locales/ja-Hira.js b/packages/scratch-gui/src/locales/ja-Hira.js index 13ebc7fec9a..6acc3ed1a13 100644 --- a/packages/scratch-gui/src/locales/ja-Hira.js +++ b/packages/scratch-gui/src/locales/ja-Hira.js @@ -168,6 +168,12 @@ export default { 'このクラスの Google Classroom にかだいをはいしんするけんげんがありません。もういちどログインしてすべてのこうもくをきょかし、そのクラスのせんせいとしてさんかしているかごかくにんください。', 'gui.classroom.error.googleCourseNotFound': 'ひもづいている Google Classroom のクラスがみつかりませんでした。クラスがさくじょされたか、ログインちゅうの Google アカウントがそのクラスのせんせいではないかのうせいがあります。', + 'gui.classroom.error.googleNotCourseTeacher': + 'ログインちゅうの Google アカウントは、ひもづいている Google Classroom のクラスのせんせいではないためきょうゆうできません。れんけいしたせんせいにきょうゆうをいらいしてください。', + 'gui.classroom.googleShare.blockedBy': + 'このクラスの Google Classroom は {email} せんせいがれんけいしています。きょうゆうは {email} せんせいにいらいしてください。', + 'gui.classroom.googleShare.blocked': + 'このクラスの Google Classroom はれんけいしたせんせいのアカウントにひもづいています。きょうゆうはれんけいしたせんせいにいらいしてください。', 'gui.classroom.error.googleApiFailedDetail': 'Google Classroom にはいしんできませんでした。（Google: {detail}）', 'gui.classroom.error.googleApiFailed': 'Google Classroom にはいしんできませんでした。しばらくまってからもういちどおためしください。', diff --git a/packages/scratch-gui/src/locales/ja.js b/packages/scratch-gui/src/locales/ja.js index 82d4c10a2d6..59aff3637b2 100644 --- a/packages/scratch-gui/src/locales/ja.js +++ b/packages/scratch-gui/src/locales/ja.js @@ -164,6 +164,12 @@ export default { 'このクラスの Google Classroom に課題を配信する権限がありません。もう一度ログインしてすべての項目を許可し、そのクラスの先生として参加しているかご確認ください。', 'gui.classroom.error.googleCourseNotFound': 'ひもづいている Google Classroom のクラスが見つかりませんでした。クラスが削除されたか、ログイン中の Google アカウントがそのクラスの先生ではない可能性があります。', + 'gui.classroom.error.googleNotCourseTeacher': + 'ログイン中の Google アカウントは、ひもづいている Google Classroom のクラスの先生ではないため共有できません。連携した先生に共有を依頼してください。', + 'gui.classroom.googleShare.blockedBy': + 'このクラスの Google Classroom は {email} 先生が連携しています。共有は {email} 先生に依頼してください。', + 'gui.classroom.googleShare.blocked': + 'このクラスの Google Classroom は連携した先生のアカウントにひもづいています。共有は連携した先生に依頼してください。', 'gui.classroom.error.googleApiFailedDetail': 'Google Classroom に配信できませんでした。（Google: {detail}）', 'gui.classroom.error.googleApiFailed': 'Google Classroom に配信できませんでした。しばらく待ってからもう一度お試しください。', diff --git a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx index 73d1a94a16b..163078ee275 100644 --- a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx +++ b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx @@ -82,6 +82,62 @@ describe('TeacherClassDetail — Google Classroom post button', () => { }); }); +// Sharing posts course work with the pressing teacher's own Google account, so +// a class co-teacher who is not a teacher of the linked course cannot share. +describe('TeacherClassDetail — Google Classroom share gate (issue #1184)', () => { + const linkedProps = (googleCourseAccess) => ({ + selectedClassroom: classroom({ googleClassroomCourseId: null }), + group: { groupId: 'g1', googleClassroomCourseId: 'course-123' }, + googleCourseAccess, + }); + const postButton = () => document.querySelector('[data-testid="classroom-post-assignment"]'); + const blockedNote = () => document.querySelector('[data-testid="classroom-post-assignment-blocked"]'); + + test('disables the share button and names who to ask when the teacher is not a course teacher', () => { + renderDetail(linkedProps({ status: 'notTeacher', linkedByEmail: 'owner@example.com' })); + expect(postButton()).toBeDisabled(); + expect(blockedNote()).toBeInTheDocument(); + expect(blockedNote().textContent).toContain('owner@example.com'); + }); + + test('stays generic rather than guessing when the linking teacher is unknown', () => { + renderDetail(linkedProps({ status: 'notTeacher', linkedByEmail: null })); + expect(postButton()).toBeDisabled(); + expect(blockedNote().textContent).toContain('another teacher'); + }); + + test('keeps the share button usable for a teacher of the course', () => { + renderDetail(linkedProps({ status: 'teacher', linkedByEmail: 'owner@example.com' })); + expect(postButton()).not.toBeDisabled(); + expect(blockedNote()).not.toBeInTheDocument(); + }); + + test('keeps the share button usable when the check was inconclusive', () => { + renderDetail(linkedProps({ status: 'unknown', linkedByEmail: null })); + expect(postButton()).not.toBeDisabled(); + expect(blockedNote()).not.toBeInTheDocument(); + }); + + test('keeps the share button usable when the check never ran', () => { + renderDetail(linkedProps(null)); + expect(postButton()).not.toBeDisabled(); + expect(blockedNote()).not.toBeInTheDocument(); + }); + + test('falls back to the class attribution when the probe carries no email', () => { + renderDetail({ + selectedClassroom: classroom({ googleClassroomCourseId: null }), + group: { + groupId: 'g1', + googleClassroomCourseId: 'course-123', + googleClassroomLinkedByEmail: 'linker@example.com', + }, + googleCourseAccess: { status: 'notTeacher', linkedByEmail: null }, + }); + expect(blockedNote().textContent).toContain('linker@example.com'); + }); +}); + describe('TeacherClassDetail — retention banner (issue #1052)', () => { const DAY = 24 * 60 * 60 * 1000; const inDays = (days) => new Date(Date.now() + days * DAY).toISOString(); diff --git a/packages/scratch-gui/test/unit/containers/classroom-error-utils.test.js b/packages/scratch-gui/test/unit/containers/classroom-error-utils.test.js index 721ed7e1ac2..1409924cb39 100644 --- a/packages/scratch-gui/test/unit/containers/classroom-error-utils.test.js +++ b/packages/scratch-gui/test/unit/containers/classroom-error-utils.test.js @@ -87,6 +87,25 @@ describe('translateError Google Classroom failures', () => { expect(msg).not.toBe('Google Classroom permission was not granted.'); }); + test('a 404 from posting course work tells the teacher who to ask', () => { + const msg = translateError( + intlMock, + googleError(404, 'googleNotCourseTeacher', { googleStatus: 'NOT_FOUND' }), + ); + // Must explain the real cause (not a teacher of that course) instead of + // suggesting the linked class disappeared or dumping a raw Google + // status (issue #1184). + expect(msg).toContain('not a teacher'); + expect(msg).not.toContain('deleted'); + expect(msg).not.toContain('NOT_FOUND'); + }); + + test('the not-a-course-teacher message is distinct from course-not-found', () => { + const notTeacher = translateError(intlMock, googleError(404, 'googleNotCourseTeacher')); + const notFound = translateError(intlMock, googleError(404, 'googleCourseNotFound')); + expect(notTeacher).not.toBe(notFound); + }); + test('non-Google errors are unaffected', () => { const err = new Error('Invalid or expired session token'); err.status = 401; diff --git a/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js new file mode 100644 index 00000000000..54355081918 --- /dev/null +++ b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js @@ -0,0 +1,50 @@ +import { + GOOGLE_COURSE_ACCESS, + isGoogleShareBlocked, + googleShareBlockedMessage, +} from '../../../src/lib/classroom-google-share.js'; + +const intl = { + formatMessage: (descriptor, values) => + values && values.email ? `${descriptor.id}:${values.email}` : descriptor.id, +}; + +describe('isGoogleShareBlocked', () => { + test('blocks only when Google proved the teacher is not a course teacher', () => { + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_TEACHER })).toBe(true); + }); + + test('allows a teacher of the course', () => { + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.TEACHER })).toBe(false); + }); + + // The check runs only with an already-granted Google token, so "we could + // not tell" is the normal state — it must never disable the button, or a + // teacher who is allowed would be locked out (issue #1184). + test('does not block when the check was inconclusive', () => { + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.UNKNOWN })).toBe(false); + }); + + test('does not block when the class is not linked at all', () => { + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_LINKED })).toBe(false); + }); + + test('does not block when the check has not run', () => { + expect(isGoogleShareBlocked(null)).toBe(false); + expect(isGoogleShareBlocked(undefined)).toBe(false); + expect(isGoogleShareBlocked({})).toBe(false); + }); +}); + +describe('googleShareBlockedMessage', () => { + test('names the linking teacher when the email is known', () => { + expect(googleShareBlockedMessage(intl, 'owner@example.com')).toBe( + 'gui.classroom.googleShare.blockedBy:owner@example.com', + ); + }); + + test('stays generic rather than guessing a name when the email is unknown', () => { + expect(googleShareBlockedMessage(intl, null)).toBe('gui.classroom.googleShare.blocked'); + expect(googleShareBlockedMessage(intl, '')).toBe('gui.classroom.googleShare.blocked'); + }); +}); From 16891fab8575fc5b0dc6b49c7452d81322ee5ed6 Mon Sep 17 00:00:00 2001 From: "smalruby3-editor-bot[bot]" <297607354+smalruby3-editor-bot[bot]@users.noreply.github.com> Date: Sat, 10 Oct 2026 06:27:37 +0000 Subject: [PATCH 2/3] fix(classroom): scope the Google Classroom share gate to the class on screen The share pre-flight verdict carried a groupId but nothing compared it, so a "not a course teacher" answer about the previously selected class disabled the share button for the class now shown (the probe is a network round trip and answers can arrive out of order, making it stick). The "ask the linking teacher" note was also rendered without the conditions the share button itself has, so an assignment already posted to Google Classroom showed its "open it" link next to a note telling the teacher to get it shared. Co-Authored-By: Claude Opus 5 --- docs/classroom/source-code.md | 1 + .../classroom-modal/teacher-class-detail.jsx | 12 ++++++- .../src/lib/classroom-google-share.js | 13 +++++++- .../components/teacher-class-detail.test.jsx | 32 ++++++++++++++++-- .../unit/lib/classroom-google-share.test.js | 33 +++++++++++++++---- 5 files changed, 80 insertions(+), 11 deletions(-) diff --git a/docs/classroom/source-code.md b/docs/classroom/source-code.md index 49bb4812915..1f3ec8b23fb 100644 --- a/docs/classroom/source-code.md +++ b/docs/classroom/source-code.md @@ -116,6 +116,7 @@ CDK スタック定義。以下のリソースを作成: | `handleCreateGroup()` / `handleListGroups()` / `handleUpdateGroup()` | 組（グループ）の作成・一覧・更新/アーカイブ | | `handleDuplicateClassroom()` | 課題の複製（課題コンテンツの S3 オブジェクトコピー含む） | | `handleListGoogleCourses()` | Google Classroom コース一覧 | +| `handleGetGoogleCourseAccess()` | 押した先生が紐付け先コースの GC 教師かの事前判定 | | `handleImportGoogleClassroom()` | Google Classroom コースインポート | | `handlePostAssignment()` | Google Classroom に課題投稿 | | `verifyTeacherToken()` | Google ID Token 検証 | diff --git a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx index dcc72a72eec..2711b274502 100644 --- a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx +++ b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx @@ -174,7 +174,17 @@ const TeacherClassDetail = ({ // so a class co-teacher who is not a teacher of the linked course cannot // share it. Disable the button with the reason instead of letting Google // answer with an opaque error (#1184). - const gcShareBlocked = isGoogleShareBlocked(googleCourseAccess); + // + // Only meaningful where the share button itself is: a class linked to a GC + // course whose assignment has not been posted yet. Once posted the button + // becomes an "open it" link, and telling the teacher to ask someone to + // share what is already shared would just confuse them. + const gcShareAvailable = Boolean( + (selectedClassroom.googleClassroomCourseId || (group && group.googleClassroomCourseId)) && + !selectedClassroom.googleClassroomAlternateLink, + ); + const gcShareBlocked = + gcShareAvailable && isGoogleShareBlocked(googleCourseAccess, group && group.groupId); const gcShareBlockedText = gcShareBlocked ? googleShareBlockedMessage( intl, diff --git a/packages/scratch-gui/src/lib/classroom-google-share.js b/packages/scratch-gui/src/lib/classroom-google-share.js index fef06b776ea..ac4a1c8d798 100644 --- a/packages/scratch-gui/src/lib/classroom-google-share.js +++ b/packages/scratch-gui/src/lib/classroom-google-share.js @@ -48,10 +48,21 @@ const messages = defineMessages({ * tell", which is the normal state before the teacher has granted Google * access this session — leaves the button enabled so a teacher who is in fact * allowed is never locked out; the failure path then explains the same thing. + * + * The verdict is about one class, so it must match the class on screen. The + * probe is a network round trip and the teacher can switch classes during it + * (and answers can arrive out of order), which would otherwise let a + * `notTeacher` verdict about the previous class disable a class they can + * perfectly well share. * @param {?object} access - the probe result, or null when it has not run + * @param {?string} groupId - the class currently on screen * @returns {boolean} true when sharing is known to be impossible */ -const isGoogleShareBlocked = (access) => Boolean(access) && access.status === GOOGLE_COURSE_ACCESS.NOT_TEACHER; +const isGoogleShareBlocked = (access, groupId) => + Boolean(access) && + access.status === GOOGLE_COURSE_ACCESS.NOT_TEACHER && + Boolean(groupId) && + access.groupId === groupId; /** * The sentence shown instead of letting the teacher press the button. diff --git a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx index 163078ee275..7c6a4ba6175 100644 --- a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx +++ b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx @@ -88,7 +88,7 @@ describe('TeacherClassDetail — Google Classroom share gate (issue #1184)', () const linkedProps = (googleCourseAccess) => ({ selectedClassroom: classroom({ googleClassroomCourseId: null }), group: { groupId: 'g1', googleClassroomCourseId: 'course-123' }, - googleCourseAccess, + googleCourseAccess: googleCourseAccess && { groupId: 'g1', ...googleCourseAccess }, }); const postButton = () => document.querySelector('[data-testid="classroom-post-assignment"]'); const blockedNote = () => document.querySelector('[data-testid="classroom-post-assignment-blocked"]'); @@ -132,10 +132,38 @@ describe('TeacherClassDetail — Google Classroom share gate (issue #1184)', () googleClassroomCourseId: 'course-123', googleClassroomLinkedByEmail: 'linker@example.com', }, - googleCourseAccess: { status: 'notTeacher', linkedByEmail: null }, + googleCourseAccess: { groupId: 'g1', status: 'notTeacher', linkedByEmail: null }, }); expect(blockedNote().textContent).toContain('linker@example.com'); }); + + // The probe is async, so switching classes leaves the previous class's + // verdict in state until the new answer lands (and answers can arrive out + // of order). It must not gate a class it was never about. + test('ignores a verdict that was about a different class', () => { + renderDetail({ + selectedClassroom: classroom({ googleClassroomCourseId: null }), + group: { groupId: 'gB', googleClassroomCourseId: 'course-B' }, + googleCourseAccess: { groupId: 'gA', status: 'notTeacher', linkedByEmail: 'a@example.com' }, + }); + expect(postButton()).not.toBeDisabled(); + expect(blockedNote()).not.toBeInTheDocument(); + }); + + // Once the assignment is posted the share button becomes an "open it" + // link, so "ask someone to share it" would be telling the teacher to + // redo something that is already done. + test('does not explain how to share an assignment that is already shared', () => { + renderDetail({ + selectedClassroom: classroom({ + googleClassroomAlternateLink: 'https://classroom.google.com/c/x/a/y', + }), + group: { groupId: 'g1', googleClassroomCourseId: 'course-123' }, + googleCourseAccess: { groupId: 'g1', status: 'notTeacher', linkedByEmail: 'owner@example.com' }, + }); + expect(document.querySelector('[data-testid="classroom-view-assignment"]')).toBeInTheDocument(); + expect(blockedNote()).not.toBeInTheDocument(); + }); }); describe('TeacherClassDetail — retention banner (issue #1052)', () => { diff --git a/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js index 54355081918..5e7a0583247 100644 --- a/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js +++ b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js @@ -10,29 +10,48 @@ const intl = { }; describe('isGoogleShareBlocked', () => { + const verdict = (status, groupId = 'g1') => ({ groupId, status }); + test('blocks only when Google proved the teacher is not a course teacher', () => { - expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_TEACHER })).toBe(true); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), 'g1')).toBe(true); }); test('allows a teacher of the course', () => { - expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.TEACHER })).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.TEACHER), 'g1')).toBe(false); }); // The check runs only with an already-granted Google token, so "we could // not tell" is the normal state — it must never disable the button, or a // teacher who is allowed would be locked out (issue #1184). test('does not block when the check was inconclusive', () => { - expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.UNKNOWN })).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.UNKNOWN), 'g1')).toBe(false); }); test('does not block when the class is not linked at all', () => { - expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_LINKED })).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_LINKED), 'g1')).toBe(false); }); test('does not block when the check has not run', () => { - expect(isGoogleShareBlocked(null)).toBe(false); - expect(isGoogleShareBlocked(undefined)).toBe(false); - expect(isGoogleShareBlocked({})).toBe(false); + expect(isGoogleShareBlocked(null, 'g1')).toBe(false); + expect(isGoogleShareBlocked(undefined, 'g1')).toBe(false); + expect(isGoogleShareBlocked({}, 'g1')).toBe(false); + }); + + // The probe is a network round trip, so switching classes while it is in + // flight (or answers arriving out of order) leaves a verdict about another + // class in state. Applying it would disable sharing for a class the + // teacher is perfectly allowed to share. + test('ignores a verdict that belongs to a different class', () => { + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER, 'gA'), 'gB')).toBe(false); + }); + + test('ignores a verdict with no class attached', () => { + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_TEACHER }, 'g1')).toBe(false); + }); + + test('does not block when there is no class on screen', () => { + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), null)).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), undefined)).toBe(false); }); }); From 77a1ae90a283343effa2316285c5858aa8e80a7d Mon Sep 17 00:00:00 2001 From: "smalruby3-editor-bot[bot]" <297607354+smalruby3-editor-bot[bot]@users.noreply.github.com> Date: Sat, 10 Oct 2026 07:34:23 +0000 Subject: [PATCH 3/3] fix(classroom): block Google Classroom share unless the teacher linked it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 共有できない先生の多くは「同意すれば通る」のではなく「何をしても通らない」 （別ワークスペースの個人アカウントは、そのコースの教師になれない）。事前判定を 「ブロックの根拠」ではなく「ブロックを解除する根拠」として使うよう既定を反転した。 - 連携した先生（googleClassroomLinkedBy。既存データは teacherSub）以外は既定で 共有ボタンを disabled にする。判定はサーバーが真偽値 linkedByMe を返す形にし、 クライアントでメール文字列を突き合わせない（大小文字・別名でぶれるため） - GC 共同教師の救済は明示的なオプトイン: ブロック表示の中の classroom-post-assignment-verify を押したときだけ同意を求めて判定し、teacher なら 共有を解禁する。クラスを開いただけでは同意ポップアップを出さない方針は維持 - 同意が得られない / notTeacher のときはエラーバナーを出さず、ブロック表示のまま 「確認できませんでした。◯◯ 先生に依頼してください」に差し替える - 連携した先生本人は判定を経ずに共有でき、その 404 は googleNotCourseTeacher では なく googleCourseNotFound（連携し直す案内）にする。自分に依頼させない - 文言はドメイン一致を判定条件にしない（外部ドメインの GC 共同教師はあり得る） Refs #1184 #1175 Co-Authored-By: Claude Opus 5 --- .claude/rules/scratch-gui/e2e-test.md | 3 +- docs/classroom/architecture.md | 48 +++++--- docs/classroom/source-code.md | 4 +- docs/classroom/testing.md | 3 +- docs/classroom/ui-ux.md | 21 +++- infra/smalruby-classroom/lambda/handler.ts | 67 +++++++++-- .../handler-google-course-access.test.ts | 35 ++++++ .../classroom-modal/classroom-modal.css | 19 +++ .../classroom-modal/teacher-class-detail.jsx | 62 ++++++++-- .../classroom-teacher-modal.jsx | 4 + .../src/containers/classroom-error-utils.js | 7 +- .../containers/classroom-teacher-modal.jsx | 2 + .../src/containers/use-google-classroom.js | 91 +++++++++++--- .../src/containers/use-teacher-classroom.js | 2 + .../src/lib/classroom-google-share.js | 113 ++++++++++++++---- packages/scratch-gui/src/locales/ja-Hira.js | 8 +- packages/scratch-gui/src/locales/ja.js | 8 +- .../components/teacher-class-detail.test.jsx | 95 +++++++++++---- .../unit/lib/classroom-google-share.test.js | 103 ++++++++++++---- 19 files changed, 568 insertions(+), 127 deletions(-) diff --git a/.claude/rules/scratch-gui/e2e-test.md b/.claude/rules/scratch-gui/e2e-test.md index 9e69200ccc9..140d392aa0c 100644 --- a/.claude/rules/scratch-gui/e2e-test.md +++ b/.claude/rules/scratch-gui/e2e-test.md @@ -131,8 +131,9 @@ expect(await isActiveByTestId(driver, 'ruby-toolbar-mode-dncl')).toBe(true); | `classroom-assignment-name-input` | input | 課題名入力 | | `classroom-create-submit` | button | クラス作成実行（クラス名・人数・課題名 3 つすべて必須） | | `classroom-sidebar-item-{classroomId}` | li | サイドバーのクラス項目（クリックで選択・teacher-class-detail へ） | -| `classroom-post-assignment` | button | Google Classroom に共有（GC コースの教師でないときは disabled） | +| `classroom-post-assignment` | button | Google Classroom に共有（連携した先生以外は既定で disabled） | | `classroom-post-assignment-blocked` | div | 共有できない理由（誰に依頼すればよいか）#1184 | +| `classroom-post-assignment-verify` | button | GC 教師であることを自分で確認して共有を解禁する #1184 | **先生フロー自動化のコツ**: - `classroom-create-submit` 後は `teacher-class-detail` には**自動遷移しない**。`teacher-dashboard` に戻ってから `classroom-sidebar-item-{id}` を明示的にクリックする必要がある diff --git a/docs/classroom/architecture.md b/docs/classroom/architecture.md index f314db13d4a..d9333c8ff0e 100644 --- a/docs/classroom/architecture.md +++ b/docs/classroom/architecture.md @@ -246,7 +246,7 @@ One Tap が表示できない環境（Windows や Playwright では毎回）で | Method | Path | 説明 | |--------|------|------| | `GET` | `/classrooms/google-courses` | Google Classroom のコース一覧 | -| `GET` | `/classroom-groups/{groupId}/google-course-access` | **押す先生自身が**紐付け先コースの GC 教師かの事前判定（`{courseId, status, linkedByEmail}`。status = `teacher` / `notTeacher` / `notLinked` / `unknown`） | +| `GET` | `/classroom-groups/{groupId}/google-course-access` | **押す先生自身が**紐付け先コースの GC 教師かの判定（`{courseId, status, linkedByEmail, linkedByMe}`。status = `teacher` / `notTeacher` / `notLinked` / `unknown`） | | `POST` | `/classrooms/google-import` | コースをインポートして課題を作成（`Classrooms` に書き込む） | | `POST` | `/classrooms/{id}/google-assignment` | Google Classroom に課題を投稿 | @@ -261,7 +261,8 @@ Google Classroom API の失敗は、Google が返した `error.status` / `error. | 401 | 401 | `googleTokenExpired` | Google のアクセストークンが失効 | | 403 | 403 | `googlePermissionDenied` | 権限（スコープ）不足 / そのコースの先生ではない | | 404 | 404 | `googleCourseNotFound` | ひもづけたコースが削除された / そのコースにアクセスできない | -| 404（課題配信時） | 404 | `googleNotCourseTeacher` | 押した先生がそのコースの GC 教師ではない（Google は未参加のコースを 404 で隠すため） | +| 404（課題配信時・押した先生が連携者でない） | 404 | `googleNotCourseTeacher` | 押した先生がそのコースの GC 教師ではない（Google は未参加のコースを 404 で隠すため） | +| 404（課題配信時・押した先生が連携者） | 404 | `googleCourseNotFound` | 連携者はそのコースの教師なので、404 はコース消失/到達不能を意味する（「連携し直す」案内にする） | | 400 | 400 | `googleInvalidRequest` | Google がリクエストを拒否（`FAILED_PRECONDITION` 等を含む） | | その他 | 502 | `googleApiError` | レート超過・Google 側障害など | @@ -273,20 +274,35 @@ Google Classroom API の失敗は、Google が返した `error.status` / `error. Google はその先生がそのコースの教師であることを要求する。Smalruby のクラス共同担当は Smalruby 上の権限であって GC の教師とは別物なので、両者が食い違うと Google 側で失敗する。 -判定は Smalruby 上の役割（owner / co-teacher）ではなく **実態**で行う: -`GET /classroom-groups/{groupId}/google-course-access` が `courses/{courseId}/teachers/me` を -caller のトークンで引き、200 なら `teacher`、404 なら `notTeacher` を返す。401/403/5xx などは -`unknown`（判定不能）で、UI は**共有ボタンを無効化しない**。同じコースに GC 共同教師として -入っている先生はこの判定を通るため、従来どおり共有できる。 - -クライアントは**すでに取得済みの Google アクセストークンがあるときだけ**この判定を行う -（判定のために同意ポップアップを出さない）。判定できなかった場合は従来どおり押させ、 -失敗時に `googleNotCourseTeacher` の文言で同じ趣旨を伝える。 - -誰が紐付けたかは group の `googleClassroomLinkedBy`（`{sub, email}`）に記録し、 -`GET /classroom-groups` は表示用に `googleClassroomLinkedByEmail` を返す。この記録が無い -既存データはクラス作成者（`teacherSub`）にフォールバックし、メールが分からないときは -「連携した先生」と一般名詞で表示する（推測で名前を出さない）。 +**既定はブロック**で、共有できるのは次のどちらかに当てはまる先生だけ: + +1. **そのコースを連携した先生**（`googleClassroomLinkedBy`。既存データはクラス作成者 + `teacherSub` にフォールバック）。サーバーが真偽値 `linkedByMe` を返し、クライアントは + メール文字列を突き合わせない（大小文字・別名でぶれるため）。 + `GET /classroom-groups` も `googleClassroomLinkedByMe` を返す。 +2. **GC 教師であることを実際に確認できた先生**。 + `GET /classroom-groups/{groupId}/google-course-access` が `courses/{courseId}/teachers/me` を + caller のトークンで引き、200 なら `teacher`、404 なら `notTeacher`。401/403/5xx などは + `unknown`（判定不能）。 + +既定を「押させて失敗させる」から「ブロック」に反転したのは、**別の Google ワークスペース +（Workspace 契約の無い個人アカウント等）の共同担当は、運用でコースの教師になることが +できない**ため。この先生はそもそも Classroom のトークンを取得できず、判定は常に `unknown` +になる。判定不能を「押させる」側に倒すと、この先生だけが救われないまま残る。 + +正当な GC 共同教師の救済は**明示的なオプトイン**: +UI のブロック表示に「自分もこの Google Classroom のクラスの先生です（確認する）」を置き、 +押されたときだけ同意（`requestClassroomAccessToken()`）→ 上記の判定を実行し、`teacher` なら +共有を解禁する。**自動では同意ポップアップを出さない**方針は維持する（クラスを開いた時点の +判定は、すでに取得済みのトークンがあるときだけ走る）。同意が得られない / `notTeacher` の +ときはエラーバナーを出さず、ブロック表示のまま「確認できませんでした」に差し替える。 + +連携した先生本人は判定を一切経ずに共有でき、404 になったときの文言も「依頼してください」では +なく `googleCourseNotFound`（連携し直す案内）になる — 依頼先は本人自身だからである。 + +メールが分からないとき（既存データ等）は「連携した先生」と一般名詞で表示する +（推測で名前を出さない）。**メールドメインの一致は判定条件にしない** — Workspace の設定次第で +外部ドメインの先生もコースの教師になれるため。 クライアントが取得するアクセストークンは `classroom.coursework.students` を含む 3 スコープを要求するが、 Google の同意画面はスコープごとにチェックボックスを出すため**一部だけ許可される**ことがある。 diff --git a/docs/classroom/source-code.md b/docs/classroom/source-code.md index 1f3ec8b23fb..35d6df17f7a 100644 --- a/docs/classroom/source-code.md +++ b/docs/classroom/source-code.md @@ -55,7 +55,7 @@ smalruby3-editor/ │ │ │ ├── classroom-api.js ← API クライアント (リトライ付き) │ │ │ ├── classroom-assignment-utils.js ← 課題エディタの純粋ヘルパー │ │ │ ├── classroom-group-utils.js ← サイドバー組階層の純粋ヘルパー -│ │ │ ├── classroom-google-share.js ← GC 共有可否の判定 + 文言（純粋）#1184 +│ │ │ ├── classroom-google-share.js ← GC 共有の既定ブロック判定 + 解除導線の文言（純粋）#1184 │ │ │ ├── classroom-evaluation/ ← sb3-analyzer（静的解析）+ evaluation-utils（総合評価/CSV） │ │ │ ├── classroom-kick-request-storage.js ← 退室依頼の localStorage 永続化 (#692) │ │ │ ├── google-classroom-auth.js ← Google Classroom OAuth @@ -116,7 +116,7 @@ CDK スタック定義。以下のリソースを作成: | `handleCreateGroup()` / `handleListGroups()` / `handleUpdateGroup()` | 組（グループ）の作成・一覧・更新/アーカイブ | | `handleDuplicateClassroom()` | 課題の複製（課題コンテンツの S3 オブジェクトコピー含む） | | `handleListGoogleCourses()` | Google Classroom コース一覧 | -| `handleGetGoogleCourseAccess()` | 押した先生が紐付け先コースの GC 教師かの事前判定 | +| `handleGetGoogleCourseAccess()` | 押した先生が紐付け先コースの GC 教師か・連携者本人か（`linkedByMe`）の判定 | | `handleImportGoogleClassroom()` | Google Classroom コースインポート | | `handlePostAssignment()` | Google Classroom に課題投稿 | | `verifyTeacherToken()` | Google ID Token 検証 | diff --git a/docs/classroom/testing.md b/docs/classroom/testing.md index c5d85109d21..84204bc533a 100644 --- a/docs/classroom/testing.md +++ b/docs/classroom/testing.md @@ -151,7 +151,8 @@ Playwright MCP および Selenium integration tests で使用する `data-testid | data-testid | 要素 | 説明 | |------------|------|------| | `classroom-post-assignment` | button | 課題配信ボタン (詳細画面) | -| `classroom-post-assignment-blocked` | div | 共有できない理由 (GC コースの教師ではない共同担当) | +| `classroom-post-assignment-blocked` | div | 共有できない理由 (連携した先生以外) | +| `classroom-post-assignment-verify` | button | 「自分もこの Google Classroom のクラスの先生です（確認する）」 | | `classroom-post-assignment-title` | input | タイトル | | `classroom-post-assignment-description` | textarea | 説明 | | `classroom-post-assignment-submit` | button | 配信実行 | diff --git a/docs/classroom/ui-ux.md b/docs/classroom/ui-ux.md index f1d4e1b6ef1..7c78c5477f7 100644 --- a/docs/classroom/ui-ux.md +++ b/docs/classroom/ui-ux.md @@ -243,7 +243,8 @@ Google または Microsoft アカウントでサインインする画面。先 | クラス（学級）名 | 「技術 2026年度 / 2年1組」 | `classroom-detail-name` | 課題が属するクラスの表示ラベル（`formatClassLabel(group)`。v2 以前のデータでは `Classrooms.className`） | | 課題名入力 | 課題名（編集可） | `classroom-detail-assignment-name` | blur で保存 | | 課題配信ボタン | 「課題を配信」 | `classroom-post-assignment` | → teacher-post-assignment。**未配信のとき**表示 | -| 共有できない理由 | 「このクラスの Google Classroom は ◯◯ 先生が連携しています。…」 | `classroom-post-assignment-blocked` | GC コースの教師ではないときだけ表示（配信ボタンは `disabled`） | +| 共有できない理由 | 「このクラスの Google Classroom は ◯◯ 先生が連携しています。…」 | `classroom-post-assignment-blocked` | 連携した先生以外に表示（配信ボタンは `disabled`） | +| GC 教師の確認ボタン | 「自分もこの Google Classroom のクラスの先生です（確認する）」 | `classroom-post-assignment-verify` | 上の理由表示の中。押すと同意を求めて判定し、GC 教師なら共有が解禁される | | 課題確認リンク | 「課題を確認」 | `classroom-view-assignment` | 配信済みのとき表示（新しいタブ） | | 参加コード表示 | 「参加コード: 3cexm5」 | `classroom-detail-join-code` | 大きなフォントで中央表示 | | コード拡大ボタン | ⛶ アイコン（ツールチップ: 「全画面表示」） | `classroom-detail-expand-code` | 全画面コード表示 | @@ -260,11 +261,19 @@ Google または Microsoft アカウントでサインインする画面。先 **課題配信ボタンの表示条件:** Google Classroom 連携はクラス（group）単位に移行したため、配信ボタン（`classroom-post-assignment`）は **クラスが GC 連携済み（`group.googleClassroomCourseId`）** であれば、その課題自体に courseId が無くても表示されます（課題の投稿先はクラスのコース）。課題単位の `googleClassroomCourseId`（v2 以前のフォールバック）が有る場合も表示されます。どちらの courseId も無い（非連携クラス）ときは表示されません。配信済み（課題に `googleClassroomAlternateLink` が保存済み）になると「Google Classroom で確認」リンク（`classroom-view-assignment`）に切り替わります。 **課題配信ボタンを押せる条件 (#1184):** 配信は**押した先生自身の Google アカウント**で行われるため、 -紐付け先の Google Classroom コースの教師でなければ Google に拒否されます。事前判定 -（`GET /classroom-groups/{groupId}/google-course-access`）で「そのコースの教師ではない」と分かった -ときだけボタンを `disabled` にし、`classroom-post-assignment-blocked` に誰に依頼すればよいかを -表示します。判定できなかったとき（Google のトークンをまだ持っていない等）はボタンを無効化せず、 -失敗時のメッセージで同じ趣旨を伝えます（判定のために同意ポップアップは出しません）。 +紐付け先の Google Classroom コースの教師でなければ Google に拒否されます。**連携した先生以外は +既定で `disabled`** にし、`classroom-post-assignment-blocked` に誰に依頼すればよいかを表示します +（別ワークスペースの個人アカウントの共同担当は、そのコースの教師に**なれない**ため、押させてから +失敗を説明しても出口がありません）。 + +同じコースの GC 共同教師は、ブロック表示の中の「自分もこの Google Classroom のクラスの先生です +（確認する）」（`classroom-post-assignment-verify`）を 1 回押せば共有できます。押したときだけ +Google の同意を求め、`teacher` と確認できればボタンが有効になります（クラスを開いただけでは +同意ポップアップは出ません）。確認できなかったときはエラーバナーではなく、ブロック表示が +「…先生であることを確認できませんでした。共有は ◯◯ 先生に依頼してください。」に変わります。 + +**連携した先生本人**は確認導線を経ずに従来どおり共有でき、コースが削除・到達不能で失敗した +ときの文言も「連携し直してください」になります（自分に依頼させない）。 **座席グリッド:** diff --git a/infra/smalruby-classroom/lambda/handler.ts b/infra/smalruby-classroom/lambda/handler.ts index 22180ceebbe..151345b4925 100644 --- a/infra/smalruby-classroom/lambda/handler.ts +++ b/infra/smalruby-classroom/lambda/handler.ts @@ -435,7 +435,10 @@ export function parseGoogleApiErrorBody(raw: string): { googleStatus: string; go * not in behind a 404, so a 404 from `courseWork` creation does not mean "the * course is gone" — it means the teacher who pressed the button is not a * teacher of that course (issue #1184). That case gets its own reason so the - * UI can say who to ask instead of blaming the link. + * UI can say who to ask instead of blaming the link. The teacher who linked + * the course is the exception (`postCourseWorkAsLinker`): they are a teacher of + * it, so their 404 really is a missing/unreachable course and falls through to + * `googleCourseNotFound`, which tells them to link it again. */ export function googleApiErrorResponse( statusCode: number, @@ -2270,6 +2273,26 @@ export function readGoogleClassroomLinkedBy( return null; } +/** + * Is this teacher the one who linked the class to its GC course? + * + * The linker is the only teacher Smalruby knows to be a teacher of the course + * (they picked it from their own `teacherId=me` list), so they are the one + * teacher allowed to share without proving anything — and the one teacher who + * must never be told to "ask the teacher who linked it", which would be + * themselves (issue #1184). + * @param identity - the signed-in teacher + * @param item - the group item (or, for pre-v2 links, the classroom item) + * @returns true when this teacher linked the course + */ +export function isGoogleClassroomLinker( + identity: TeacherIdentity, + item: Record | undefined, +): boolean { + const linkedBy = readGoogleClassroomLinkedBy(item); + return Boolean(linkedBy && linkedBy.sub === identity.sub); +} + /** * Decide what a failed "am I a teacher of this course?" probe proves. * @@ -2302,9 +2325,13 @@ async function handleGetGoogleCourseAccess( const group = await getManageableGroup(identity, groupId); const courseId = typeof group.googleClassroomCourseId === 'string' ? group.googleClassroomCourseId : ''; const linkedBy = readGoogleClassroomLinkedBy(group); - const linkedByEmail = linkedBy && linkedBy.sub !== identity.sub ? linkedBy.email : null; + const linkedByMe = isGoogleClassroomLinker(identity, group); + const linkedByEmail = linkedBy && !linkedByMe ? linkedBy.email : null; if (!courseId) { - return { statusCode: 200, body: JSON.stringify({ courseId: null, status: 'notLinked', linkedByEmail: null }) }; + return { + statusCode: 200, + body: JSON.stringify({ courseId: null, status: 'notLinked', linkedByEmail: null, linkedByMe: false }), + }; } let status: 'teacher' | 'notTeacher' | 'unknown'; @@ -2315,7 +2342,7 @@ async function handleGetGoogleCourseAccess( if (!(err instanceof GoogleAPIError)) throw err; status = classifyCourseTeacherFailure(err.statusCode); } - return { statusCode: 200, body: JSON.stringify({ courseId, status, linkedByEmail }) }; + return { statusCode: 200, body: JSON.stringify({ courseId, status, linkedByEmail, linkedByMe }) }; } async function handleListGoogleCourses(accessToken: string): Promise { @@ -2478,13 +2505,21 @@ async function handlePostAssignment( // v2: the GC link lives on the class (group); the assignment's own field // remains as a pre-v2 fallback. Only look the group up when the assignment // itself carries no courseId. + // Attribution belongs to whichever item carries the link we are about to + // post to: the class (group) for v2 links, the assignment itself for pre-v2 + // ones (`readGoogleClassroomLinkedBy` then falls back to its owner, the only + // teacher who could have linked it back then). let courseId = resolveGoogleCourseId(result.Item, undefined); + let linkedByItem: Record = result.Item; if (!courseId && typeof result.Item.groupId === 'string' && result.Item.groupId) { const groupResult = await docClient.send(new GetCommand({ TableName: GROUPS_TABLE, Key: { groupId: result.Item.groupId }, })); courseId = resolveGoogleCourseId(result.Item, groupResult.Item); + if (courseId && groupResult.Item) { + linkedByItem = groupResult.Item; + } } if (!courseId) { throw new ValidationError('This classroom is not linked to Google Classroom'); @@ -2521,7 +2556,12 @@ async function handlePostAssignment( state: 'PUBLISHED', materials: [{ link: { url: link, title: 'スモウルビーで開く' } }], }, - 'postCourseWork', + // A 404 means something different depending on who pressed the button: for + // anyone but the linker it is "you are not a teacher of that course", but + // the linker *is* one, so for them it means the course is gone or no longer + // reachable — telling them to ask the teacher who linked it would point + // them at themselves (issue #1184). + isGoogleClassroomLinker(identity, linkedByItem) ? 'postCourseWorkAsLinker' : 'postCourseWork', ) as { id: string; alternateLink: string }; // Persist courseWork info to prevent duplicate posting @@ -2898,7 +2938,7 @@ async function ensureGroupTopic( })); } -function mapGroupSummary(item: Record) { +function mapGroupSummary(item: Record, identity?: TeacherIdentity) { return { groupId: item.groupId, name: item.name, @@ -2913,6 +2953,13 @@ function mapGroupSummary(item: Record) { googleClassroomLinkedByEmail: item.googleClassroomCourseId ? (readGoogleClassroomLinkedBy(item)?.email ?? null) : null, + // Whether *this* teacher linked it. The UI blocks sharing by default for + // everyone else (a Smalruby co-teacher is usually not in the GC course and + // can never become one across Workspaces), so it needs a server-decided + // boolean — matching email strings client-side breaks on case and aliases. + googleClassroomLinkedByMe: Boolean( + item.googleClassroomCourseId && identity && isGoogleClassroomLinker(identity, item), + ), studentCount: typeof item.studentCount === 'number' ? item.studentCount : null, coTeacherEmails: readCoTeacherEmails(item), createdAt: item.createdAt, @@ -2956,7 +3003,7 @@ async function handleCreateGroup(identity: TeacherIdentity, body: Record { @@ -2968,7 +3015,7 @@ async function handleListGroups(identity: TeacherIdentity): Promise ({ - ...mapGroupSummary(item), + ...mapGroupSummary(item, identity), role: item.teacherSub === identity.sub ? 'owner' : 'co-teacher', })); return { statusCode: 200, body: JSON.stringify({ groups }) }; @@ -3059,7 +3106,7 @@ async function handleUpdateGroup(identity: TeacherIdentity, groupId: string, bod await propagateStudentCountToClassrooms(groupId, updates.studentCount as number); } - return { statusCode: 200, body: JSON.stringify(mapGroupSummary(result.Attributes || {})) }; + return { statusCode: 200, body: JSON.stringify(mapGroupSummary(result.Attributes || {}, identity)) }; } /** @@ -3271,7 +3318,7 @@ async function handleUpdateGroupTopics( ExpressionAttributeValues: { ':topics': topics, ':now': now }, ReturnValues: 'ALL_NEW', })); - return { statusCode: 200, body: JSON.stringify(mapGroupSummary(result.Attributes || {})) }; + return { statusCode: 200, body: JSON.stringify(mapGroupSummary(result.Attributes || {}, identity)) }; } /** diff --git a/infra/smalruby-classroom/lambda/tests/handler-google-course-access.test.ts b/infra/smalruby-classroom/lambda/tests/handler-google-course-access.test.ts index 20fa6edb8e3..8fc11034a68 100644 --- a/infra/smalruby-classroom/lambda/tests/handler-google-course-access.test.ts +++ b/infra/smalruby-classroom/lambda/tests/handler-google-course-access.test.ts @@ -2,6 +2,7 @@ import { googleApiErrorResponse, buildGoogleClassroomLinkedBy, readGoogleClassroomLinkedBy, + isGoogleClassroomLinker, classifyCourseTeacherFailure, } from '../handler'; @@ -30,6 +31,40 @@ describe('googleApiErrorResponse with operation context', () => { 'googleCourseNotFound', ); }); + + // The teacher who linked the course *is* a teacher of it, so their 404 means + // the course is gone — "ask the teacher who linked it" would send them to + // themselves (issue #1184). + test('404 while the linking teacher posts means the course is gone, not a wrong teacher', () => { + const res = googleApiErrorResponse(404, 'NOT_FOUND', '', 'postCourseWorkAsLinker'); + expect(parse(res).reason).toBe('googleCourseNotFound'); + }); +}); + +describe('isGoogleClassroomLinker', () => { + const identity = { sub: 'linker-sub', email: 'linker@example.com' }; + + test('recognizes the recorded linker', () => { + const item = { teacherSub: 'owner-sub', googleClassroomLinkedBy: { sub: 'linker-sub', email: null } }; + expect(isGoogleClassroomLinker(identity, item)).toBe(true); + }); + + test('a co-teacher who did not link it is not the linker', () => { + const item = { teacherSub: 'owner-sub', googleClassroomLinkedBy: { sub: 'someone-else', email: null } }; + expect(isGoogleClassroomLinker(identity, item)).toBe(false); + }); + + // Classes linked before the attribution existed fall back to the owner — + // the only teacher who could have linked them under the old flow. + test('falls back to the class owner for rows written before the field existed', () => { + expect(isGoogleClassroomLinker(identity, { teacherSub: 'linker-sub' })).toBe(true); + expect(isGoogleClassroomLinker(identity, { teacherSub: 'owner-sub' })).toBe(false); + }); + + test('nothing to attribute the link to is not a match', () => { + expect(isGoogleClassroomLinker(identity, {})).toBe(false); + expect(isGoogleClassroomLinker(identity, undefined)).toBe(false); + }); }); describe('buildGoogleClassroomLinkedBy', () => { diff --git a/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css b/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css index 5e11f67eeb2..38e563822b3 100644 --- a/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css +++ b/packages/scratch-gui/src/components/classroom-modal/classroom-modal.css @@ -375,6 +375,25 @@ line-height: 1.4; } +/* 「自分も Google Classroom の先生です（確認する）」— ブロック表示の中の + オプトイン導線。押したときだけ同意を求める。#1184 */ +.join-code-gc-verify-button { + display: inline-block; + margin-top: 0.3rem; + padding: 0.2rem 0.6rem; + border: 1px solid #c3cad8; + border-radius: 0.5rem; + background-color: #fff; + color: #4c97ff; + font-size: 0.75rem; + cursor: pointer; +} + +.join-code-gc-verify-button:disabled { + color: #9aa0ad; + cursor: default; +} + /* コピー完了のリアクション: アイコンがチェックに変わり緑で強調 */ .join-code-action-copied { border-color: #4caf50; diff --git a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx index 2711b274502..dbb57394604 100644 --- a/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx +++ b/packages/scratch-gui/src/components/classroom-modal/teacher-class-detail.jsx @@ -9,7 +9,12 @@ import TeacherAssignmentEditor from './teacher-assignment-editor.jsx'; import TeacherMemberDetail from './teacher-member-detail.jsx'; import { formatClassLabel } from '../../lib/classroom-class-label.js'; -import { isGoogleShareBlocked, googleShareBlockedMessage } from '../../lib/classroom-google-share.js'; +import { + canVerifyGoogleShare, + googleShareBlockedMessage, + googleShareVerdict, + isGoogleShareBlocked, +} from '../../lib/classroom-google-share.js'; import { retentionLevel } from '../../lib/classroom-retention.js'; import googleClassroomIcon from '../classroom-teacher-modal/google-classroom-icon.png'; import styles from './classroom-modal.css'; @@ -49,6 +54,8 @@ const TeacherClassDetail = ({ assignmentEditor, group, googleCourseAccess, + isVerifyingGoogleCourseAccess, + onVerifyGoogleCourseAccess, }) => { // Destructured with handle-prefixed names for the embedded editor // (react/jsx-handler-names requires handler props to look like handlers). @@ -172,8 +179,9 @@ const TeacherClassDetail = ({ // Google Classroom share is posted with this teacher's own Google account, // so a class co-teacher who is not a teacher of the linked course cannot - // share it. Disable the button with the reason instead of letting Google - // answer with an opaque error (#1184). + // share it — and a co-teacher on a personal Google account never can. + // Block by default and let a genuine GC co-teacher lift it themselves, + // instead of letting Google answer with an opaque error (#1184). // // Only meaningful where the share button itself is: a class linked to a GC // course whose assignment has not been posted yet. Once posted the button @@ -183,16 +191,27 @@ const TeacherClassDetail = ({ (selectedClassroom.googleClassroomCourseId || (group && group.googleClassroomCourseId)) && !selectedClassroom.googleClassroomAlternateLink, ); - const gcShareBlocked = - gcShareAvailable && isGoogleShareBlocked(googleCourseAccess, group && group.groupId); + const gcVerdict = googleShareVerdict(googleCourseAccess, group); + const gcShareBlocked = gcShareAvailable && isGoogleShareBlocked(googleCourseAccess, group); const gcShareBlockedText = gcShareBlocked ? googleShareBlockedMessage( intl, - googleCourseAccess.linkedByEmail || + (gcVerdict && gcVerdict.linkedByEmail) || (group && group.googleClassroomLinkedByEmail) || null, + Boolean(gcVerdict && gcVerdict.verifyFailed), ) : ''; + // The way out for a teacher who is in the course but has not proven it yet + // (they have granted Google access to nothing so far). Asking costs a + // consent popup, so it only ever happens when they press it. + const gcCanVerify = Boolean( + gcShareBlocked && onVerifyGoogleCourseAccess && canVerifyGoogleShare(googleCourseAccess, group), + ); + + const handleVerifyGcAccess = useCallback(() => { + if (onVerifyGoogleCourseAccess) onVerifyGoogleCourseAccess(group); + }, [onVerifyGoogleCourseAccess, group]); const handleShowCode = useCallback(() => { setShowCodeDisplay(true); @@ -441,13 +460,37 @@ const TeacherClassDetail = ({ ))} {/* 共有できない理由（GC コースの教師ではない共同担当） - をボタンの近くに出す。#1184 */} + をボタンの近くに出す。本当に GC の共同教師である + 先生だけが「確認する」で自分で解除できる。#1184 */} {gcShareBlocked && (
{gcShareBlockedText} + {gcCanVerify && ( + + )}
)} @@ -924,9 +967,14 @@ TeacherClassDetail.propTypes = { assignmentEditor: PropTypes.object, group: PropTypes.object, googleCourseAccess: PropTypes.shape({ + groupId: PropTypes.string, status: PropTypes.string, linkedByEmail: PropTypes.string, + linkedByMe: PropTypes.bool, + verifyFailed: PropTypes.bool, }), + isVerifyingGoogleCourseAccess: PropTypes.bool, + onVerifyGoogleCourseAccess: PropTypes.func, selectedClassroom: PropTypes.object.isRequired, selectedMember: PropTypes.string, }; diff --git a/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx b/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx index 1690a9f242f..c197edd3a23 100644 --- a/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx +++ b/packages/scratch-gui/src/components/classroom-teacher-modal/classroom-teacher-modal.jsx @@ -93,6 +93,8 @@ const ClassroomTeacherModal = ({ containerProps, onClose }) => { downloadProgress, googleCourses, googleCourseAccess, + isVerifyingGoogleCourseAccess, + onVerifyGoogleCourseAccess, selectedGoogleCourse, googleSignInRef, googleFallbackReason, @@ -344,6 +346,7 @@ const ClassroomTeacherModal = ({ containerProps, onClose }) => { googleCourseAccess={googleCourseAccess} group={selectedGroup} isLoading={isLoading} + isVerifyingGoogleCourseAccess={isVerifyingGoogleCourseAccess} kickRequestsBySeat={kickRequestsBySeat} members={members} noBackButton @@ -365,6 +368,7 @@ const ClassroomTeacherModal = ({ containerProps, onClose }) => { shared={shared} onOpenShareSuggestion={handleOpenShareSuggestion} onShowPostAssignment={authProvider === 'google' ? onShowPostAssignment : null} + onVerifyGoogleCourseAccess={authProvider === 'google' ? onVerifyGoogleCourseAccess : null} onToggleCodeFullscreen={onToggleCodeFullscreen} onUpdateAssignmentName={onUpdateAssignmentName} /> diff --git a/packages/scratch-gui/src/containers/classroom-error-utils.js b/packages/scratch-gui/src/containers/classroom-error-utils.js index 2e14cbe4c15..05e0fb8bd02 100644 --- a/packages/scratch-gui/src/containers/classroom-error-utils.js +++ b/packages/scratch-gui/src/containers/classroom-error-utils.js @@ -45,10 +45,13 @@ const translateGoogleClassroomError = (intl, err) => { }); } if (reason === 'googleCourseNotFound') { + // This now reaches the teacher who linked the course (the server tells + // the two 404s apart), and they are a teacher of it — so the way out is + // to link it again, not to ask themselves for help (issue #1184). return intl.formatMessage({ defaultMessage: - 'The linked Google Classroom class could not be found. It may have been deleted, or the ' + - 'Google account you signed in with may not be a teacher of that class.', + 'The linked Google Classroom class could not be found. It may have been deleted or is no ' + + 'longer reachable. Please link the Google Classroom class again from class management.', description: 'Error when the linked Google Classroom course is missing or inaccessible', id: 'gui.classroom.error.googleCourseNotFound', }); diff --git a/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx b/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx index 37bec45dc5f..306f5f6f551 100644 --- a/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx +++ b/packages/scratch-gui/src/containers/classroom-teacher-modal.jsx @@ -139,6 +139,8 @@ const ClassroomTeacherModal = () => { downloadProgress: teacher.downloadProgress, googleCourses: teacher.googleCourses, googleCourseAccess: teacher.googleCourseAccess, + isVerifyingGoogleCourseAccess: teacher.isVerifyingGoogleCourseAccess, + onVerifyGoogleCourseAccess: teacher.handleVerifyGoogleCourseAccess, selectedGoogleCourse: teacher.selectedGoogleCourse, googleSignInRef: teacher.googleSignInRef, googleFallbackReason: teacher.googleFallbackReason, diff --git a/packages/scratch-gui/src/containers/use-google-classroom.js b/packages/scratch-gui/src/containers/use-google-classroom.js index f33cc4aa711..c439ef6b983 100644 --- a/packages/scratch-gui/src/containers/use-google-classroom.js +++ b/packages/scratch-gui/src/containers/use-google-classroom.js @@ -42,17 +42,21 @@ const useGoogleClassroom = ({ const [googleAccessToken, setGoogleAccessToken] = useState(null); const [googleCourses, setGoogleCourses] = useState([]); const [selectedGoogleCourse, setSelectedGoogleCourse] = useState(null); - // Pre-flight for the share button: {groupId, status, linkedByEmail}. + // Share-button verdict: {groupId, status, linkedByEmail, linkedByMe, + // verifyFailed}. Sharing is blocked unless this says otherwise (#1184). const [googleCourseAccess, setGoogleCourseAccess] = useState(null); + const [isVerifyingGoogleCourseAccess, setIsVerifyingGoogleCourseAccess] = useState(false); /** * Find out whether this teacher can post to the class's linked Google - * Classroom course, so the share button can be disabled with an - * explanation instead of failing inside Google (issue #1184). + * Classroom course, so a Google Classroom co-teacher who already granted + * access this session gets the share button without having to ask for a + * check (issue #1184). * * Deliberately silent: it uses a Google token the teacher already granted - * and gives up (leaving the button enabled) when there is none, so the - * check never triggers a consent popup on its own. + * and gives up when there is none, so the check never triggers a consent + * popup on its own. Giving up leaves the button blocked — the teacher can + * lift that themselves with the explicit check below. * @param {?object} group - the class (group) whose link to check * @returns {Promise} resolves once the state has been updated */ @@ -62,30 +66,85 @@ const useGoogleClassroom = ({ setGoogleCourseAccess(null); return; } + const unknown = { + groupId: group.groupId, + status: GOOGLE_COURSE_ACCESS.UNKNOWN, + linkedByEmail: group.googleClassroomLinkedByEmail || null, + linkedByMe: Boolean(group.googleClassroomLinkedByMe), + verifyFailed: false, + }; const accessToken = googleAccessToken || getCachedClassroomAccessToken(); if (!accessToken) { - setGoogleCourseAccess({ - groupId: group.groupId, - status: GOOGLE_COURSE_ACCESS.UNKNOWN, - linkedByEmail: group.googleClassroomLinkedByEmail || null, - }); + setGoogleCourseAccess(unknown); return; } try { const data = await classroomAPI.getGoogleCourseAccess(idToken, accessToken, group.groupId); setGoogleCourseAccess({ - groupId: group.groupId, + ...unknown, status: data.status || GOOGLE_COURSE_ACCESS.UNKNOWN, linkedByEmail: data.linkedByEmail || group.googleClassroomLinkedByEmail || null, + linkedByMe: Boolean(data.linkedByMe) || unknown.linkedByMe, }); } catch { - // A failed probe must not block sharing, and must not raise an - // error banner either — nothing the teacher did has failed yet. + // Nothing the teacher did has failed yet, so this must not + // raise an error banner. + setGoogleCourseAccess(unknown); + } + }, + [idToken, googleAccessToken], + ); + + /** + * Run the same probe at the teacher's request, asking for Google access + * when they have none. + * + * This is the way out of the default block for a teacher who really is a + * Google Classroom co-teacher of the linked course: they press "check", + * grant access once, and sharing opens up. Dismissing consent or being + * refused by Google is not an error the teacher needs a banner for — it is + * simply the block standing, so the reason is reported in place (#1184). + * @param {?object} group - the class (group) whose link to check + * @returns {Promise} resolves once the state has been updated + */ + const handleVerifyGoogleCourseAccess = useCallback( + async (group) => { + if (!group || !group.groupId || !group.googleClassroomCourseId) return; + const base = { + groupId: group.groupId, + linkedByEmail: group.googleClassroomLinkedByEmail || null, + linkedByMe: Boolean(group.googleClassroomLinkedByMe), + }; + setIsVerifyingGoogleCourseAccess(true); + try { + let accessToken = googleAccessToken || getCachedClassroomAccessToken(); + if (!accessToken) { + accessToken = await requestClassroomAccessToken(); + setGoogleAccessToken(accessToken); + } + const data = await classroomAPI.getGoogleCourseAccess(idToken, accessToken, group.groupId); + const status = data.status || GOOGLE_COURSE_ACCESS.UNKNOWN; setGoogleCourseAccess({ - groupId: group.groupId, + ...base, + status, + linkedByEmail: data.linkedByEmail || base.linkedByEmail, + linkedByMe: Boolean(data.linkedByMe) || base.linkedByMe, + verifyFailed: status !== GOOGLE_COURSE_ACCESS.TEACHER, + }); + } catch (err) { + // A refused or expired token is useless for the next attempt, + // so drop it and let a retry re-run consent (#1175). + if (err && (err.status === 401 || err.status === 403)) { + clearClassroomAccessToken(); + setGoogleAccessToken(null); + } + setGoogleCourseAccess({ + ...base, status: GOOGLE_COURSE_ACCESS.UNKNOWN, - linkedByEmail: group.googleClassroomLinkedByEmail || null, + verifyFailed: true, }); + } finally { + setIsVerifyingGoogleCourseAccess(false); } }, [idToken, googleAccessToken], @@ -216,7 +275,9 @@ const useGoogleClassroom = ({ return { googleCourses, googleCourseAccess, + isVerifyingGoogleCourseAccess, handleCheckGoogleCourseAccess, + handleVerifyGoogleCourseAccess, selectedGoogleCourse, handleShowGoogleCourses, handleLoadGoogleCourses, diff --git a/packages/scratch-gui/src/containers/use-teacher-classroom.js b/packages/scratch-gui/src/containers/use-teacher-classroom.js index 8c82f5728a4..6916f57b77b 100644 --- a/packages/scratch-gui/src/containers/use-teacher-classroom.js +++ b/packages/scratch-gui/src/containers/use-teacher-classroom.js @@ -346,6 +346,8 @@ const useTeacherClassroom = ({ // Google Classroom googleCourses: google.googleCourses, googleCourseAccess: google.googleCourseAccess, + isVerifyingGoogleCourseAccess: google.isVerifyingGoogleCourseAccess, + handleVerifyGoogleCourseAccess: google.handleVerifyGoogleCourseAccess, selectedGoogleCourse: google.selectedGoogleCourse, handleShowGoogleCourses: google.handleShowGoogleCourses, handleLoadGoogleCourses: google.handleLoadGoogleCourses, diff --git a/packages/scratch-gui/src/lib/classroom-google-share.js b/packages/scratch-gui/src/lib/classroom-google-share.js index ac4a1c8d798..4fd4cca9770 100644 --- a/packages/scratch-gui/src/lib/classroom-google-share.js +++ b/packages/scratch-gui/src/lib/classroom-google-share.js @@ -5,8 +5,15 @@ * so a co-teacher who is not in the Google Classroom course used to hit a raw * `Google Classroom API error` (issue #1184). * - * These helpers turn the server's pre-flight answer into "disable the button?" - * plus the sentence that explains who to ask. + * The teacher this issue is about cannot be rescued by a better error message: + * they are on a personal Google account with no Workspace, so they can never + * become a teacher of the course. Sharing is therefore **blocked by default** + * for everyone except the teacher who linked the course, and the probe is used + * the other way around — as the evidence that *unblocks* a genuine Google + * Classroom co-teacher, run only when they ask for it. + * + * These helpers decide "disable the button?" and produce the sentence that + * explains who to ask. */ import { defineMessages } from 'react-intl'; @@ -39,40 +46,104 @@ const messages = defineMessages({ 'when that teacher cannot be named', id: 'gui.classroom.googleShare.blocked', }, + verifyFailedBy: { + defaultMessage: + 'We could not confirm that you are a teacher of this Google Classroom class. ' + + 'Please ask {email} to share it.', + description: 'Shown when the teacher asked us to check their Google Classroom access and it did not confirm', + id: 'gui.classroom.googleShare.verifyFailedBy', + }, + verifyFailed: { + defaultMessage: + 'We could not confirm that you are a teacher of this Google Classroom class. ' + + 'Please ask the teacher who linked it to share it.', + description: + 'Shown when the teacher asked us to check their Google Classroom access and it did not confirm, ' + + 'when the linking teacher cannot be named', + id: 'gui.classroom.googleShare.verifyFailed', + }, }); +/** + * The probe answer that is about the class on screen, if any. + * + * The probe is a network round trip and the teacher can switch classes during + * it (answers can even arrive out of order), so a verdict must be matched to + * the class it was about before it is allowed to decide anything. + * @param {?object} access - the probe result, or null when it has not run + * @param {?object} group - the class (group) currently on screen + * @returns {?object} the verdict for this class, or null + */ +const googleShareVerdict = (access, group) => { + const groupId = group && group.groupId; + if (!access || !groupId || access.groupId !== groupId) return null; + return access; +}; + /** * Should the "share to Google Classroom" button be disabled? * - * Only a proven `notTeacher` blocks. Anything else — including "we could not - * tell", which is the normal state before the teacher has granted Google - * access this session — leaves the button enabled so a teacher who is in fact - * allowed is never locked out; the failure path then explains the same thing. + * Blocked unless one of two things is true: this teacher linked the course + * (server-decided, so no email string matching), or the probe proved they are + * a teacher of it. "We could not tell" blocks — the teacher this issue is + * about can never be told apart from a silent failure, and the block carries + * its own way out (the verify affordance) for a genuine GC co-teacher. + * @param {?object} access - the probe result, or null when it has not run + * @param {?object} group - the class (group) currently on screen + * @returns {boolean} true when sharing should be prevented + */ +const isGoogleShareBlocked = (access, group) => { + if (!group || !group.groupId) return false; + if (group.googleClassroomLinkedByMe) return false; + const verdict = googleShareVerdict(access, group); + if (verdict && verdict.status === GOOGLE_COURSE_ACCESS.TEACHER) return false; + if (verdict && verdict.linkedByMe) return false; + // The class's own link is what the default block is about. A class that is + // linked only through the assignment (pre-v2) carries no attribution, so + // keep the old behaviour there: block only what Google proved. + if (group.googleClassroomCourseId) return true; + return Boolean(verdict && verdict.status === GOOGLE_COURSE_ACCESS.NOT_TEACHER); +}; + +/** + * Can this teacher still prove they are a Google Classroom co-teacher? * - * The verdict is about one class, so it must match the class on screen. The - * probe is a network round trip and the teacher can switch classes during it - * (and answers can arrive out of order), which would otherwise let a - * `notTeacher` verdict about the previous class disable a class they can - * perfectly well share. + * Only worth offering while we have no answer for them: once Google has said + * "not a teacher" for this account, asking again replays the same answer. * @param {?object} access - the probe result, or null when it has not run - * @param {?string} groupId - the class currently on screen - * @returns {boolean} true when sharing is known to be impossible + * @param {?object} group - the class (group) currently on screen + * @returns {boolean} true when the verify affordance should be offered */ -const isGoogleShareBlocked = (access, groupId) => - Boolean(access) && - access.status === GOOGLE_COURSE_ACCESS.NOT_TEACHER && - Boolean(groupId) && - access.groupId === groupId; +const canVerifyGoogleShare = (access, group) => { + const verdict = googleShareVerdict(access, group); + return !verdict || verdict.status !== GOOGLE_COURSE_ACCESS.NOT_TEACHER; +}; /** * The sentence shown instead of letting the teacher press the button. + * + * After the teacher asked us to check (and it did not confirm them) the + * sentence says so, rather than repeating an explanation they just acted on. * @param {object} intl - react-intl intl object * @param {?string} linkedByEmail - email of the teacher who linked the course + * @param {boolean} [verifyFailed] - a check this teacher asked for came back negative * @returns {string} localized explanation naming who to ask, when known */ -const googleShareBlockedMessage = (intl, linkedByEmail) => - linkedByEmail +const googleShareBlockedMessage = (intl, linkedByEmail, verifyFailed = false) => { + if (verifyFailed) { + return linkedByEmail + ? intl.formatMessage(messages.verifyFailedBy, { email: linkedByEmail }) + : intl.formatMessage(messages.verifyFailed); + } + return linkedByEmail ? intl.formatMessage(messages.blockedBy, { email: linkedByEmail }) : intl.formatMessage(messages.blocked); +}; -export { GOOGLE_COURSE_ACCESS, isGoogleShareBlocked, googleShareBlockedMessage }; +export { + GOOGLE_COURSE_ACCESS, + googleShareVerdict, + isGoogleShareBlocked, + canVerifyGoogleShare, + googleShareBlockedMessage, +}; diff --git a/packages/scratch-gui/src/locales/ja-Hira.js b/packages/scratch-gui/src/locales/ja-Hira.js index 6acc3ed1a13..a66dff892a6 100644 --- a/packages/scratch-gui/src/locales/ja-Hira.js +++ b/packages/scratch-gui/src/locales/ja-Hira.js @@ -167,13 +167,19 @@ export default { 'gui.classroom.error.googlePermissionDenied': 'このクラスの Google Classroom にかだいをはいしんするけんげんがありません。もういちどログインしてすべてのこうもくをきょかし、そのクラスのせんせいとしてさんかしているかごかくにんください。', 'gui.classroom.error.googleCourseNotFound': - 'ひもづいている Google Classroom のクラスがみつかりませんでした。クラスがさくじょされたか、ログインちゅうの Google アカウントがそのクラスのせんせいではないかのうせいがあります。', + 'ひもづいている Google Classroom のクラスがみつかりませんでした。クラスがさくじょされたか、アクセスできなくなっているかのうせいがあります。クラスかんりのがめんで Google Classroom をれんけいしなおしてください。', 'gui.classroom.error.googleNotCourseTeacher': 'ログインちゅうの Google アカウントは、ひもづいている Google Classroom のクラスのせんせいではないためきょうゆうできません。れんけいしたせんせいにきょうゆうをいらいしてください。', 'gui.classroom.googleShare.blockedBy': 'このクラスの Google Classroom は {email} せんせいがれんけいしています。きょうゆうは {email} せんせいにいらいしてください。', 'gui.classroom.googleShare.blocked': 'このクラスの Google Classroom はれんけいしたせんせいのアカウントにひもづいています。きょうゆうはれんけいしたせんせいにいらいしてください。', + 'gui.classroom.googleShare.verifyFailedBy': + 'ログインちゅうの Google アカウントが、この Google Classroom のクラスのせんせいであることをかくにんできませんでした。きょうゆうは {email} せんせいにいらいしてください。', + 'gui.classroom.googleShare.verifyFailed': + 'ログインちゅうの Google アカウントが、この Google Classroom のクラスのせんせいであることをかくにんできませんでした。きょうゆうはれんけいしたせんせいにいらいしてください。', + 'gui.classroom.googleShare.verify': 'じぶんもこの Google Classroom のクラスのせんせいです（かくにんする）', + 'gui.classroom.googleShare.verifying': 'かくにんちゅう…', 'gui.classroom.error.googleApiFailedDetail': 'Google Classroom にはいしんできませんでした。（Google: {detail}）', 'gui.classroom.error.googleApiFailed': 'Google Classroom にはいしんできませんでした。しばらくまってからもういちどおためしください。', diff --git a/packages/scratch-gui/src/locales/ja.js b/packages/scratch-gui/src/locales/ja.js index 59aff3637b2..793b6bd7abe 100644 --- a/packages/scratch-gui/src/locales/ja.js +++ b/packages/scratch-gui/src/locales/ja.js @@ -163,13 +163,19 @@ export default { 'gui.classroom.error.googlePermissionDenied': 'このクラスの Google Classroom に課題を配信する権限がありません。もう一度ログインしてすべての項目を許可し、そのクラスの先生として参加しているかご確認ください。', 'gui.classroom.error.googleCourseNotFound': - 'ひもづいている Google Classroom のクラスが見つかりませんでした。クラスが削除されたか、ログイン中の Google アカウントがそのクラスの先生ではない可能性があります。', + 'ひもづいている Google Classroom のクラスが見つかりませんでした。クラスが削除されたか、アクセスできなくなっている可能性があります。クラス管理の画面で Google Classroom を連携し直してください。', 'gui.classroom.error.googleNotCourseTeacher': 'ログイン中の Google アカウントは、ひもづいている Google Classroom のクラスの先生ではないため共有できません。連携した先生に共有を依頼してください。', 'gui.classroom.googleShare.blockedBy': 'このクラスの Google Classroom は {email} 先生が連携しています。共有は {email} 先生に依頼してください。', 'gui.classroom.googleShare.blocked': 'このクラスの Google Classroom は連携した先生のアカウントにひもづいています。共有は連携した先生に依頼してください。', + 'gui.classroom.googleShare.verifyFailedBy': + 'ログイン中の Google アカウントが、この Google Classroom のクラスの先生であることを確認できませんでした。共有は {email} 先生に依頼してください。', + 'gui.classroom.googleShare.verifyFailed': + 'ログイン中の Google アカウントが、この Google Classroom のクラスの先生であることを確認できませんでした。共有は連携した先生に依頼してください。', + 'gui.classroom.googleShare.verify': '自分もこの Google Classroom のクラスの先生です（確認する）', + 'gui.classroom.googleShare.verifying': '確認中…', 'gui.classroom.error.googleApiFailedDetail': 'Google Classroom に配信できませんでした。（Google: {detail}）', 'gui.classroom.error.googleApiFailed': 'Google Classroom に配信できませんでした。しばらく待ってからもう一度お試しください。', diff --git a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx index 7c6a4ba6175..369de3c9ab7 100644 --- a/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx +++ b/packages/scratch-gui/test/unit/components/teacher-class-detail.test.jsx @@ -83,21 +83,34 @@ describe('TeacherClassDetail — Google Classroom post button', () => { }); // Sharing posts course work with the pressing teacher's own Google account, so -// a class co-teacher who is not a teacher of the linked course cannot share. +// a class co-teacher who is not a teacher of the linked course cannot share — +// and one on a personal Google account never can. Blocked by default, with a +// check the genuine Google Classroom co-teacher can run themselves. describe('TeacherClassDetail — Google Classroom share gate (issue #1184)', () => { - const linkedProps = (googleCourseAccess) => ({ + const linkedProps = (googleCourseAccess, groupOver = {}) => ({ selectedClassroom: classroom({ googleClassroomCourseId: null }), - group: { groupId: 'g1', googleClassroomCourseId: 'course-123' }, + group: { groupId: 'g1', googleClassroomCourseId: 'course-123', ...groupOver }, googleCourseAccess: googleCourseAccess && { groupId: 'g1', ...googleCourseAccess }, + onVerifyGoogleCourseAccess: jest.fn(), }); const postButton = () => document.querySelector('[data-testid="classroom-post-assignment"]'); const blockedNote = () => document.querySelector('[data-testid="classroom-post-assignment-blocked"]'); + const verifyButton = () => document.querySelector('[data-testid="classroom-post-assignment-verify"]'); - test('disables the share button and names who to ask when the teacher is not a course teacher', () => { - renderDetail(linkedProps({ status: 'notTeacher', linkedByEmail: 'owner@example.com' })); + // The teacher in the bug report had never granted Google access, so the + // probe could not run at all: this is the state they open the class in. + test('disables the share button from the start for a teacher who did not link the course', () => { + renderDetail(linkedProps(null, { googleClassroomLinkedByEmail: 'owner@example.com' })); expect(postButton()).toBeDisabled(); expect(blockedNote()).toBeInTheDocument(); expect(blockedNote().textContent).toContain('owner@example.com'); + expect(verifyButton()).toBeInTheDocument(); + }); + + test('keeps it disabled when the silent check could not tell', () => { + renderDetail(linkedProps({ status: 'unknown', linkedByEmail: 'owner@example.com' })); + expect(postButton()).toBeDisabled(); + expect(verifyButton()).toBeInTheDocument(); }); test('stays generic rather than guessing when the linking teacher is unknown', () => { @@ -106,45 +119,79 @@ describe('TeacherClassDetail — Google Classroom share gate (issue #1184)', () expect(blockedNote().textContent).toContain('another teacher'); }); - test('keeps the share button usable for a teacher of the course', () => { - renderDetail(linkedProps({ status: 'teacher', linkedByEmail: 'owner@example.com' })); + test('keeps the share button usable for the teacher who linked the course', () => { + renderDetail(linkedProps(null, { googleClassroomLinkedByMe: true })); expect(postButton()).not.toBeDisabled(); expect(blockedNote()).not.toBeInTheDocument(); }); - test('keeps the share button usable when the check was inconclusive', () => { - renderDetail(linkedProps({ status: 'unknown', linkedByEmail: null })); + test('keeps the share button usable for a proven teacher of the course', () => { + renderDetail(linkedProps({ status: 'teacher', linkedByEmail: 'owner@example.com' })); expect(postButton()).not.toBeDisabled(); expect(blockedNote()).not.toBeInTheDocument(); }); - test('keeps the share button usable when the check never ran', () => { - renderDetail(linkedProps(null)); - expect(postButton()).not.toBeDisabled(); - expect(blockedNote()).not.toBeInTheDocument(); + test('the check asks the container to verify this class', () => { + const props = linkedProps(null); + renderDetail(props); + fireEvent.click(verifyButton()); + expect(props.onVerifyGoogleCourseAccess).toHaveBeenCalledWith(props.group); + }); + + // A check that came back negative must read as a result, and must not + // offer the same popup again — Google's answer will not change. + test('reports a failed check in place and stops offering it', () => { + renderDetail(linkedProps({ status: 'notTeacher', linkedByEmail: 'owner@example.com', verifyFailed: true })); + expect(blockedNote().textContent).toContain('could not confirm'); + expect(blockedNote().textContent).toContain('owner@example.com'); + expect(verifyButton()).not.toBeInTheDocument(); + }); + + // Consent dismissed / Google unreachable: still blocked, still retryable, + // and reported here rather than in an error banner. + test('keeps the check available when it failed without an answer', () => { + renderDetail(linkedProps({ status: 'unknown', linkedByEmail: null, verifyFailed: true })); + expect(blockedNote().textContent).toContain('could not confirm'); + expect(verifyButton()).toBeInTheDocument(); + }); + + test('shows the check as running while it is in flight', () => { + renderDetail({ ...linkedProps(null), isVerifyingGoogleCourseAccess: true }); + expect(verifyButton()).toBeDisabled(); + expect(verifyButton().textContent).toContain('Checking'); }); test('falls back to the class attribution when the probe carries no email', () => { - renderDetail({ - selectedClassroom: classroom({ googleClassroomCourseId: null }), - group: { - groupId: 'g1', - googleClassroomCourseId: 'course-123', - googleClassroomLinkedByEmail: 'linker@example.com', - }, - googleCourseAccess: { groupId: 'g1', status: 'notTeacher', linkedByEmail: null }, - }); + renderDetail( + linkedProps( + { status: 'notTeacher', linkedByEmail: null }, + { + googleClassroomLinkedByEmail: 'linker@example.com', + }, + ), + ); expect(blockedNote().textContent).toContain('linker@example.com'); }); // The probe is async, so switching classes leaves the previous class's // verdict in state until the new answer lands (and answers can arrive out - // of order). It must not gate a class it was never about. + // of order). It must not decide anything about a class it was not about — + // including lifting the block. test('ignores a verdict that was about a different class', () => { renderDetail({ selectedClassroom: classroom({ googleClassroomCourseId: null }), group: { groupId: 'gB', googleClassroomCourseId: 'course-B' }, - googleCourseAccess: { groupId: 'gA', status: 'notTeacher', linkedByEmail: 'a@example.com' }, + googleCourseAccess: { groupId: 'gA', status: 'teacher', linkedByEmail: 'a@example.com' }, + }); + expect(postButton()).toBeDisabled(); + }); + + // Pre-v2 classes link Google Classroom on the assignment with nobody + // recorded as the linker, so the default block would lock out the owner. + test('keeps the old proof-only rule when only the assignment is linked', () => { + renderDetail({ + selectedClassroom: classroom({ googleClassroomCourseId: 'course-123' }), + group: { groupId: 'g1', googleClassroomCourseId: null }, }); expect(postButton()).not.toBeDisabled(); expect(blockedNote()).not.toBeInTheDocument(); diff --git a/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js index 5e7a0583247..e80801e5e1a 100644 --- a/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js +++ b/packages/scratch-gui/test/unit/lib/classroom-google-share.test.js @@ -1,7 +1,9 @@ import { GOOGLE_COURSE_ACCESS, - isGoogleShareBlocked, + canVerifyGoogleShare, googleShareBlockedMessage, + googleShareVerdict, + isGoogleShareBlocked, } from '../../../src/lib/classroom-google-share.js'; const intl = { @@ -9,49 +11,101 @@ const intl = { values && values.email ? `${descriptor.id}:${values.email}` : descriptor.id, }; +// The class on screen: linked to a GC course by somebody else. +const linkedGroup = (over = {}) => ({ + groupId: 'g1', + googleClassroomCourseId: 'course-123', + ...over, +}); + describe('isGoogleShareBlocked', () => { const verdict = (status, groupId = 'g1') => ({ groupId, status }); - test('blocks only when Google proved the teacher is not a course teacher', () => { - expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), 'g1')).toBe(true); + // The teacher this issue is about is on a personal Google account and can + // never join the course, so "we could not tell" has to block — otherwise + // they keep pressing a button that always fails (issue #1184). + test('blocks by default when another teacher linked the course', () => { + expect(isGoogleShareBlocked(null, linkedGroup())).toBe(true); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.UNKNOWN), linkedGroup())).toBe(true); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), linkedGroup())).toBe(true); }); - test('allows a teacher of the course', () => { - expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.TEACHER), 'g1')).toBe(false); + test('never blocks the teacher who linked the course', () => { + const mine = linkedGroup({ googleClassroomLinkedByMe: true }); + expect(isGoogleShareBlocked(null, mine)).toBe(false); + // Not even a negative probe: their 404 means the course is gone, and + // they are the person everyone else is told to ask. + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), mine)).toBe(false); }); - // The check runs only with an already-granted Google token, so "we could - // not tell" is the normal state — it must never disable the button, or a - // teacher who is allowed would be locked out (issue #1184). - test('does not block when the check was inconclusive', () => { - expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.UNKNOWN), 'g1')).toBe(false); + test('lifts the block once the probe proves the teacher is in the course', () => { + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.TEACHER), linkedGroup())).toBe(false); }); - test('does not block when the class is not linked at all', () => { - expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_LINKED), 'g1')).toBe(false); + test('lifts the block when the probe reports this teacher as the linker', () => { + const access = { groupId: 'g1', status: GOOGLE_COURSE_ACCESS.UNKNOWN, linkedByMe: true }; + expect(isGoogleShareBlocked(access, linkedGroup())).toBe(false); }); - test('does not block when the check has not run', () => { - expect(isGoogleShareBlocked(null, 'g1')).toBe(false); - expect(isGoogleShareBlocked(undefined, 'g1')).toBe(false); - expect(isGoogleShareBlocked({}, 'g1')).toBe(false); + // Pre-v2 classes carry the GC link on the assignment, with nobody recorded + // as the linker — blocking everyone there would lock out the owner. + test('keeps the proof-only rule when the class itself is not linked', () => { + const unlinked = { groupId: 'g1', googleClassroomCourseId: null }; + expect(isGoogleShareBlocked(null, unlinked)).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.UNKNOWN), unlinked)).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), unlinked)).toBe(true); }); // The probe is a network round trip, so switching classes while it is in // flight (or answers arriving out of order) leaves a verdict about another - // class in state. Applying it would disable sharing for a class the - // teacher is perfectly allowed to share. + // class in state. It must not decide anything about the class on screen — + // in either direction. test('ignores a verdict that belongs to a different class', () => { - expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER, 'gA'), 'gB')).toBe(false); + expect( + isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.TEACHER, 'gA'), linkedGroup({ groupId: 'gB' })), + ).toBe(true); }); test('ignores a verdict with no class attached', () => { - expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.NOT_TEACHER }, 'g1')).toBe(false); + expect(isGoogleShareBlocked({ status: GOOGLE_COURSE_ACCESS.TEACHER }, linkedGroup())).toBe(true); }); test('does not block when there is no class on screen', () => { expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), null)).toBe(false); expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), undefined)).toBe(false); + expect(isGoogleShareBlocked(verdict(GOOGLE_COURSE_ACCESS.NOT_TEACHER), {})).toBe(false); + }); +}); + +describe('googleShareVerdict', () => { + test('returns the verdict only when it is about the class on screen', () => { + const access = { groupId: 'g1', status: GOOGLE_COURSE_ACCESS.TEACHER }; + expect(googleShareVerdict(access, linkedGroup())).toBe(access); + expect(googleShareVerdict(access, linkedGroup({ groupId: 'g2' }))).toBeNull(); + expect(googleShareVerdict(null, linkedGroup())).toBeNull(); + expect(googleShareVerdict(access, null)).toBeNull(); + }); +}); + +describe('canVerifyGoogleShare', () => { + test('offers the check while no answer exists for this teacher', () => { + expect(canVerifyGoogleShare(null, linkedGroup())).toBe(true); + expect(canVerifyGoogleShare({ groupId: 'g1', status: GOOGLE_COURSE_ACCESS.UNKNOWN }, linkedGroup())).toBe( + true, + ); + }); + + // Asking again just replays Google's answer, at the cost of another popup. + test('stops offering it once Google said this account is not a teacher', () => { + expect(canVerifyGoogleShare({ groupId: 'g1', status: GOOGLE_COURSE_ACCESS.NOT_TEACHER }, linkedGroup())).toBe( + false, + ); + }); + + test('ignores an answer about a different class', () => { + expect(canVerifyGoogleShare({ groupId: 'gA', status: GOOGLE_COURSE_ACCESS.NOT_TEACHER }, linkedGroup())).toBe( + true, + ); }); }); @@ -66,4 +120,13 @@ describe('googleShareBlockedMessage', () => { expect(googleShareBlockedMessage(intl, null)).toBe('gui.classroom.googleShare.blocked'); expect(googleShareBlockedMessage(intl, '')).toBe('gui.classroom.googleShare.blocked'); }); + + // After the teacher asked us to check, repeating the generic explanation + // would read as if nothing happened. + test('reports a failed check instead of repeating the explanation', () => { + expect(googleShareBlockedMessage(intl, 'owner@example.com', true)).toBe( + 'gui.classroom.googleShare.verifyFailedBy:owner@example.com', + ); + expect(googleShareBlockedMessage(intl, null, true)).toBe('gui.classroom.googleShare.verifyFailed'); + }); });