Feat/memberships rbac#29
Conversation
New ADRs documenting the architectural decisions behind the multi-tenant SaaS shift: - 0011: Per-company RBAC (memberships with owner/accountant/reviewer roles) - 0012: User-scoped KSeF certificates (cert belongs to person, not company) - 0013: Email/password auth (replaces Google Sign-In + ALLOWED_EMAILS) - 0014: Company invitation system (owner invites members by email) Updated existing ADRs 0006 and 0008 with supersession notes pointing to the new decisions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce `memberships` join table linking users to companies with roles (owner, accountant, invoice_reviewer). Update LiveAuth to load only the user's companies via memberships and assign current_role. Update CompanySwitchController to verify membership before allowing company switch. Add data migration to seed owner memberships for existing users. Implements ADR 0011 (per-company RBAC) steps 1.1-1.5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…berships - Certificates and API Tokens nav items are owner-only in the sidebar - CompanyLive.Index now uses create_company_with_owner/2 and scopes company list to the current user's memberships - All existing LiveView tests updated to create memberships in setup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-company RBAC and multi-tenant scaffolding: new Membership schema and migrations (including seeding), membership-aware Companies APIs, membership checks in controllers/LiveViews/layouts, many tests, and updated ADRs/docs for auth, certificates, invitations, and an implementation plan. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User (Browser)
participant LiveView as Company LiveView
participant Controller as CompanySwitchController
participant Companies as Companies Context
participant Repo as Repo/DB
Browser->>LiveView: Submit "create company" (attrs)
LiveView->>Companies: create_company_with_owner(user, attrs)
Companies->>Repo: insert company (transaction)
Repo-->>Companies: company inserted
Companies->>Repo: insert membership (owner)
Repo-->>Companies: membership inserted
Companies-->>LiveView: {:ok, %{company: c, membership: m}}
LiveView-->>Browser: Redirect / show success
Browser->>Controller: POST /switch-company (company_id)
Controller->>Companies: get_membership(user_id, company_id)
Companies->>Repo: query membership
Repo-->>Companies: membership found/none
alt membership exists
Controller->>Browser: put_session(current_company_id) + redirect
else no membership
Controller->>Browser: flash "Company not found." + redirect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/adr/0012-user-scoped-certificates.md`:
- Around line 36-45: The fenced code block showing the ksef_credentials (after
migration) table is missing a language specifier; update the triple-backtick
fence around the block that contains "ksef_credentials (after migration)" to
include a language such as "text" (e.g., ```text) so the block is rendered
correctly and syntax-highlighted consistently in the ADR document.
- Around line 21-32: The fenced code block showing the user_certificates table
is missing a language specifier; update the opening fence from ``` to ```text
(or ```plaintext) so the block is recognized as plain text; locate the block
that begins with the triple backticks and the "user_certificates" table (the
block containing id, user_id, certificate_data_encrypted, etc.) and change the
fence accordingly.
In `@docs/ksef-certificates.md`:
- Around line 50-54: Add explicit language identifiers (e.g., ```text) to all
fenced code blocks in the docs/ksef-certificates.md file; specifically update
the block that starts with "Identyfikator podmiotu: PESEL: xxxxxxxxxxx", the
block beginning "1. App sends: getChallenge(...)", the block with "Company A
(NIP: 1234567890) → Credential row..." and the block starting "User → One
credential row with cert data" (and the other similar blocks mentioned at ranges
98-110, 136-139, 145-149) by changing opening fences from ``` to ```text so
markdownlint MD040 no longer warns.
In `@lib/ksef_hub/companies.ex`:
- Around line 102-118: The Membership changeset currently allows mass-assignment
of user_id and company_id; in create_company_with_owner you must build the
Membership struct with those fields set directly and only pass attrs that are
allowed (e.g., role) into Membership.changeset so ids are not cast from user
input. Update create_company_with_owner to call
Membership.changeset(%Membership{user_id: user.id, company_id: company.id},
%{role: "owner"}) (or otherwise merge only safe keys) and
audit/create_membership to ensure it also sets user_id/company_id on the struct
(not via incoming attrs) and only casts permitted fields in
Membership.changeset.
In `@lib/ksef_hub/companies/membership.ex`:
- Around line 5-9: The module docstring in KsefHub.Companies.Membership lists
duplicate descriptions for the roles `accountant` and `invoice_reviewer`; update
the docstring so each role has a distinct, accurate description (e.g.,
`accountant` — handles bookkeeping and financial records, viewing invoices and
submitting approvals; `invoice_reviewer` — responsible for validating invoice
content and approving/rejecting specific expense items), ensuring the `Roles:`
block in the Membership module clearly differentiates `accountant` and
`invoice_reviewer`.
- Around line 36-47: The changeset currently casts user_id and company_id from
params—remove them from cast/2 so only :role is cast, update def
changeset(membership, attrs) to cast(attrs, [:role]) and keep the
validations/constraints; change the API so create_membership/1 (or its caller)
passes user_id and company_id explicitly to the changeset call by setting them
on the membership struct (e.g., put_change/put_assoc or Map.put before
changeset/2) rather than including them in attrs; ensure validate_required still
lists [:user_id, :company_id, :role] and unique_constraint name and
foreign_key_constraint calls remain as-is.
In `@test/ksef_hub_web/live/live_auth_test.exs`:
- Around line 84-98: The test "assigns current_role from membership" creates an
"accountant" membership but never verifies the role effect; update the test
(after live("/dashboard") and html = render(view)) to assert that owner-only UI
is hidden and/or the socket assigns current_role as "accountant" by checking the
rendered HTML for absence of the owner-only nav label (e.g., the string your app
uses for owner-only nav) and/or by inspecting the LiveView assigns via the test
helpers around view (use render/view assertions to confirm current_role ==
"accountant"); keep the existing use of init_test_session, live, view, and
render(view) to locate where to add the assertion.
In `@test/ksef_hub_web/live/role_based_nav_test.exs`:
- Around line 13-52: Tests currently pattern-match the rendered HTML string (
{:ok, _view, html} ) and use html =~ "Certificates"/"API Tokens"; instead
capture the LiveView socket as view (e.g., {:ok, view, _html} = ... ), then
replace string assertions with element/2 + has_element?/2 checks against stable
selectors such as has_element?(view, "a[href='/certificates']") and
has_element?(view, "a[href='/api_tokens']") for positive cases and refute
has_element?(...) for negative cases; update the three tests (the ones creating
membership roles "accountant" and "invoice_reviewer" and the default case)
accordingly and keep the other asserts (e.g., "Dashboard", "Invoices") converted
to selector checks like has_element?(view, "a[href='/dashboard']") or
has_element?(view, "a[href='/invoices']").
🧹 Nitpick comments (7)
docs/adr/0011-per-company-rbac.md (1)
19-25: Consider adding a language identifier to the code block.The static analysis tool flagged this fenced code block as missing a language specification. While this is a pseudo-schema diagram rather than executable code, you could use
textorplaintextto satisfy linters and maintain consistency.📝 Suggested fix
-``` +```text memberships ├── user_id (FK → users)priv/repo/migrations/20260210000001_create_memberships.exs (1)
16-17: Consider adding an index onuser_idfor user-scoped queries.The unique index on
[:user_id, :company_id]optimizes lookups by both columns together, and thecompany_idindex supports company-scoped queries. However,list_companies_for_user(user_id)queries will filter byuser_idalone. While PostgreSQL can use the leading column of a composite index, an explicituser_idindex may improve query planning for user-centric operations.📝 Suggested addition
create unique_index(:memberships, [:user_id, :company_id]) create index(:memberships, [:company_id]) + create index(:memberships, [:user_id])Based on learnings: "Use database indexes on frequently queried columns"
docs/implementation-plan.md (1)
367-373: Consider adding a language identifier to the dependency graph code block.Similar to other ADR documents, this ASCII diagram code block could use
textorplaintextto satisfy markdown linters.📝 Suggested fix
-``` +```text PR 1 (Memberships) ──→ PR 2 (User Certificates)docs/adr/0014-company-invitation-system.md (1)
17-28: Consider adding a language identifier to the schema code block.Same lint recommendation as other ADR documents - use
textorplaintextfor the pseudo-schema diagram.📝 Suggested fix
-``` +```text invitations ├── idlib/ksef_hub_web/controllers/company_switch_controller.ex (1)
14-27: Consider handling the case whereuser_idis nil.If the session doesn't contain a
user_id(e.g., session expired or corrupted),get_membership(nil, uuid)will be called. While this will returnniland fall through to the error case correctly, an explicit guard or early return would make the intent clearer and could provide a more appropriate error message or redirect (e.g., to login).📝 Suggested improvement
def update(conn, %{"id" => id} = params) do user_id = get_session(conn, :user_id) - with {:ok, uuid} <- Ecto.UUID.cast(id), + with user_id when not is_nil(user_id) <- user_id, + {:ok, uuid} <- Ecto.UUID.cast(id), %{} <- Companies.get_membership(user_id, uuid) dolib/ksef_hub_web/live/live_auth.ex (1)
42-49: Redundantresolve_rolecall whencurrent_companyfallback triggers.When
resolve_companyreturnsnil(e.g., session contains an invalidcurrent_company_id), line 48 falls back toList.first(companies). However, line 43 already computedcurrent_roleasnilfor that case, triggering the fallback on line 49 to callresolve_roleagain with the sameList.first(companies).This works correctly but performs two DB lookups for
get_membershipwhen the session company is invalid. Consider computing both values from the resolved company in one pass:♻️ Suggested simplification
defp assign_user_and_companies(socket, user, session) do companies = Companies.list_companies_for_user(user.id) - current_company = resolve_company(companies, session["current_company_id"]) - current_role = resolve_role(user.id, current_company) + current_company = resolve_company(companies, session["current_company_id"]) || List.first(companies) + current_role = resolve_role(user.id, current_company) socket |> assign(:current_user, user) |> assign(:companies, companies) - |> assign(:current_company, current_company || List.first(companies)) - |> assign(:current_role, current_role || resolve_role(user.id, List.first(companies))) + |> assign(:current_company, current_company) + |> assign(:current_role, current_role) |> assign(:current_path, nil)test/ksef_hub_web/controllers/company_switch_controller_test.exs (1)
30-33: Consider stronger assertion for session state.The assertion
refute get_session(conn, :current_company_id) == company.idpasses if the session key is nil or set to any other value. If the intent is to verify the key wasn't set at all, consider:assert is_nil(get_session(conn, :current_company_id))Or if the key might have a previous value that should remain unchanged, the current assertion is appropriate but could benefit from a comment explaining the intent.
- Add `text` language identifier to all fenced code blocks in ADR 0012 and ksef-certificates.md (MD040) - Harden Membership changeset: only cast :role, set user_id/company_id on struct to prevent mass-assignment of foreign keys - Update create_membership/1 and create_company_with_owner/2 to set IDs on struct before calling changeset - Differentiate role descriptions in Membership moduledoc - LiveAuth test now verifies accountant role hides owner-only nav via has_element? on stable selectors - Role-based nav tests use has_element?/refute has_element? with href selectors instead of string matching Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add explicit user_id index on memberships for user-scoped queries - Guard against nil user_id in CompanySwitchController - Eliminate redundant resolve_role call in LiveAuth by resolving company fallback before computing role - Use assert is_nil/1 for stronger session state assertion in CompanySwitchController test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ksef_hub_web/live/live_auth_test.exs (1)
29-56:⚠️ Potential issue | 🟡 MinorAvoid raw HTML string assertions in LiveView tests.
Lines 39, 54-55, and 80-81 use
render(view)with string matching, which is brittle and sensitive to formatting changes. Use selector-based assertions instead:
- Line 39: Replace
assert html =~ "Dashboard"withassert has_element?(view, "a[href='/dashboard']")(tests the nav link)- Lines 54-55: Replace text assertions with selectors targeting the company dropdown, e.g.,
assert has_element?(view, "button", text: "My Company")or adddata-testidattributes to the company items in the layout- Line 81: Similarly replace
assert html =~ "Mine"with selector-based assertionConsider adding
data-testidattributes to key elements inlib/ksef_hub_web/components/layouts.ex(the company dropdown and nav items) to make tests more maintainable and explicit about what they're verifying.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/ksef_hub_web/controllers/company_switch_controller_test.exs`:
- Around line 47-58: Add a test to verify open-redirect protection by ensuring
external URLs in the return_to param are rejected: in
test/ksef_hub_web/controllers/company_switch_controller_test.exs add a case
similar to the existing "respects return_to parameter" but post a malicious
value like "https://evil.com" (and consider testing protocol-relative
"//evil.com") and assert the redirect falls back to the safe default
("/dashboard"); this ensures the controller's safe_return_to/1 logic in
CompanySwitchController is exercised and external hosts are not allowed.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/live_auth.ex (1)
57-65: Refine type spec to useCompany.t()instead of genericmap().The role field in the Membership schema is correctly defined as
:string, so the return typeString.t()is accurate. However, thecompanyparameter should be typed asCompanies.Company.t()for better type safety and dialyzer coverage, since that's what is actually passed to the function.♻️ Type refinement
- `@spec` resolve_role(Ecto.UUID.t(), map() | nil) :: String.t() | nil + `@spec` resolve_role(Ecto.UUID.t(), Companies.Company.t() | nil) :: String.t() | nilSimilarly, consider refining
resolve_company/2(line 77) to returnCompanies.Company.t() | nilinstead ofmap() | nilfor consistency.
- Add data-testid attributes to company selector for stable test selectors - Use has_element? with CSS selectors in LiveAuth tests instead of string matching - Add open-redirect protection tests for CompanySwitchController - Refine type specs in LiveAuth to use Company.t() instead of map() - Add Common Mistakes section to implementation plan with lessons from all reviews Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/components/layouts.ex (1)
114-123:⚠️ Potential issue | 🔴 CriticalAdd owner authorization checks to CertificateLive and TokenLive mount functions.
The routes are protected by authentication but lack authorization enforcement. Both
CertificateLiveandTokenLiveshould verify@current_role == "owner"in theirmount/3callbacks and redirect unauthorized users. Currently, non-owners can bypass the UI navigation and access these pages directly via URL.CertificateLive.mount/3 (line 1): Add a guard check:
defp mount(_params, _session, socket) do if socket.assigns.current_role != "owner" do raise Phoenix.Router.NoRouteError, conn: socket, router: KsefHubWeb.Router end # ... rest of mount endTokenLive.mount/3 (line 1): Add the same authorization check.
Also verify that
handle_eventcallbacks likecreate,revoke, andremove_certificatecannot be exploited by unauthorized users with valid sessions.
🤖 Fix all issues with AI agents
In `@docs/implementation-plan.md`:
- Around line 367-373: The fenced code block containing the PR diagram should
include a language identifier; change the opening triple-backticks for the block
that contains "PR 1 (Memberships) ──→ PR 2 (User Certificates) ..." to use
```text (i.e., replace ``` with ```text) so markdownlint and docs guidelines
recognize the language for that code fence.
🧹 Nitpick comments (2)
test/ksef_hub_web/controllers/company_switch_controller_test.exs (1)
35-45: Consider adding session state assertion for consistency.The "user without membership" test (line 32) verifies that
current_company_idremains nil, but this test doesn't include the same assertion. Adding it would ensure consistent verification of session state on failure paths.♻️ Suggested improvement
assert redirected_to(conn) == "/dashboard" assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Company not found." + assert is_nil(get_session(conn, :current_company_id)) endlib/ksef_hub_web/components/layouts.ex (1)
19-19: Add documentation for thecurrent_roleattribute for consistency.Other attributes like
current_scopeinclude adocfield. Consider adding documentation to maintain consistency:attr :current_role, :string, default: nil, doc: "the user's role for the current company membership"
| ``` | ||
| PR 1 (Memberships) ──→ PR 2 (User Certificates) | ||
| │ | ||
| ├──────────→ PR 3 (Email/Password Auth) ──→ PR 4 (Invitations) | ||
| │ | ||
| └──────────→ PR 5 (API Token Scoping) | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
Markdownlint and our docs guideline require a language specifier for fenced blocks. Use text here. As per coding guidelines: “Always add language identifiers to fenced code blocks in markdown files.”
✍️ Proposed fix
-```
+```text
PR 1 (Memberships) ──→ PR 2 (User Certificates)
│
├──────────→ PR 3 (Email/Password Auth) ──→ PR 4 (Invitations)
│
└──────────→ PR 5 (API Token Scoping)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 367-367: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/implementation-plan.md around lines 367 - 373, The fenced code block
containing the PR diagram should include a language identifier; change the
opening triple-backticks for the block that contains "PR 1 (Memberships) ──→ PR
2 (User Certificates) ..." to use text (i.e., replace with ```text) so
markdownlint and docs guidelines recognize the language for that code fence.
</details>
<!-- fingerprinting:phantom:triton:eagle -->
<!-- This is an auto-generated comment by CodeRabbit -->
- Add session state assertion to non-existent company test for consistency - Add doc field to current_role attribute in layouts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
User-facing changes
Documentation
Chores
Tests