Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(account): add possibility to block #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+474 −97
Open

feat(account): add possibility to block #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d685519
feat(account): add possibility to block
sthelemann Oct 10, 2024
0819b9d
feat(account-blocking): enforce blocking
sthelemann Oct 28, 2024
b74a487
Merge remote-tracking branch 'maevsi/master' into feat/account_block
sthelemann Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
226 changes: 199 additions & 27 deletions schema/schema.definition.sql
View file
Open in desktop

Large diffs are not rendered by default.

31 changes: 21 additions & 10 deletions src/deploy/function_events_invited.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,43 @@
-- requires: schema_public
-- requires: table_invitation
-- requires: table_contact
-- requires: table_account_block
-- requires: role_account
-- requires: role_anonymous

BEGIN;

CREATE FUNCTION maevsi_private.events_invited()
RETURNS TABLE (event_id UUID) AS $$
CREATE OR REPLACE FUNCTION maevsi_private.events_invited()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why OR REPLACE? The way migrations are applied or reverted currently does not allow for any case where this function could be replaced, I think.

RETURNS TABLE(event_id uuid)
AS $$
DECLARE
jwt_account_id UUID;
BEGIN
jwt_account_id := NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID;

RETURN QUERY
SELECT invitation.event_id FROM maevsi.invitation
SELECT event_id FROM maevsi.invitation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a few places it's necessary to keep this prefix or the column name would be ambiguous, but maybe this is not necessary here any more if you've checked it and it works without. If this function is confirmed to still be working, you can resolve this comment.

WHERE
invitation.contact_id IN (
(
contact_id IN (
SELECT id
FROM maevsi.contact
WHERE
jwt_account_id IS NOT NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the IS NOT NULL check removed here? I think it's likely necessary to be kept as I once found it necessary to add the comment regarding it below.

The contact selection does not return rows where account_id "IS" null due to the equality comparison.

AND
contact.account_id = jwt_account_id
) -- The contact selection does not return rows where account_id "IS" null due to the equality comparison.
OR invitation.id = ANY (maevsi.invitation_claim_array());
account_id = jwt_account_id
-- The contact selection does not return rows where account_id "IS" null due to the equality comparison.
AND
-- contact not created by a blocked account
author_account_id NOT IN (
SELECT account_block_id
FROM maevsi.account_block
WHERE b.author_account_id = jwt_account_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is b defined?

)
)
)
OR id = ANY (maevsi.invitation_claim_array());
END
$$ LANGUAGE PLPGSQL STRICT STABLE SECURITY DEFINER;
$$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER
;
Comment on lines +42 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER
;
$$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER;

Let's keep the formatting as is and change it only in a separate PR if wanted.


COMMENT ON FUNCTION maevsi_private.events_invited() IS 'Add a function that returns all event ids for which the invoker is invited.';

Expand Down
28 changes: 24 additions & 4 deletions src/deploy/function_invitation_claim_array.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,40 @@
-- Deploy maevsi:function_invitation_claim_array to pg
-- requires: privilege_execute_revoke
-- requires: schema_public
-- requires: table_invitation
-- requires: table_contact
-- requires: table_account_block
-- requires: role_account
-- requires: role_anonymous

BEGIN;

CREATE FUNCTION maevsi.invitation_claim_array()
CREATE OR REPLACE FUNCTION maevsi.invitation_claim_array()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to remove invitation ids that a user has collected when the event, an invitation was created for, is already made inaccessible for the user?

In case a user is unblocked, this user would need to reclaim all invitation codes. If we just leave this method as is, the user would still be unable to access events by the user who blocked but would also be able to access the events again when unblocked.

This is a highly theoretical question though, as this function is used for users without accounts primarily as far as I know.

RETURNS UUID[] AS $$
DECLARE
_arr UUID[];
_result_arr UUID[] := ARRAY[]::UUID[];
_id UUID;
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's continue using names here that immediately make clear how the variables are used 👍

Suggested change
_arr UUID[];
_result_arr UUID[] := ARRAY[]::UUID[];
_id UUID;
_invitation_ids UUID[];
_invitation_ids_unblocked UUID[] := ARRAY[]::UUID[];
_invitation_id UUID;

BEGIN
RETURN string_to_array(replace(btrim(current_setting('jwt.claims.invitations', true), '[]'), '"', ''), ',')::UUID[];
_arr := string_to_array(replace(btrim(current_setting('jwt.claims.invitations', true), '[]'), '"', ''), ',')::UUID[];
FOREACH _id IN ARRAY arr
LOOP
-- omit invitations authored by a blocked account
IF NOT EXISTS(
SELECT 1
FROM maevsi.invitation i
JOIN maevsi.contact c ON i.contact_id = c.contact_id
JOIN maevsi.account_block b ON c.author_account_id = b.blocked_account_id
WHERE i.id = _id and b.author_account_id = current_setting('jwt.claims.account_id', true)
) THEN
_result_arr := append_array(result_arr, _id);
END IF;
END LOOP;

RETURN _result_arr;
END
$$ LANGUAGE PLPGSQL STRICT STABLE SECURITY INVOKER;

COMMENT ON FUNCTION maevsi.invitation_claim_array() IS 'Returns the current invitation claims as UUID array.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this comment go?


GRANT EXECUTE ON FUNCTION maevsi.invitation_claim_array() TO maevsi_account, maevsi_anonymous;

COMMIT;
16 changes: 14 additions & 2 deletions src/deploy/function_invitation_contact_ids.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,32 @@
-- requires: privilege_execute_revoke
-- requires: schema_public
-- requires: table_invitation
-- requires: table_contact
-- requires: table_account_block
-- requires: function_invitation_claim_array
-- requires: function_events_organized
-- requires: role_account
-- requires: role_anonymous

BEGIN;

CREATE FUNCTION maevsi.invitation_contact_ids()
CREATE OR REPLACE FUNCTION maevsi.invitation_contact_ids()
RETURNS TABLE (contact_id UUID) AS $$
BEGIN
RETURN QUERY
SELECT invitation.contact_id FROM maevsi.invitation
WHERE id = ANY (maevsi.invitation_claim_array())
OR event_id IN (SELECT maevsi.events_organized());
OR (
event_id IN (SELECT maevsi.events_organized())
AND
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this AND be a suffix to the 2 ORs? So that the brackets are places around the OR, ike (this OR that) AND not_blocked?

-- omit contacts authored by a blocked account or referring to a blocked account
contact_id NOT IN (
SELECT c.id
FROM maevsi.contact c
JOIN maevsi.account_block b ON c.author_account_id = b.blocked_account_id OR c.account_id = b.blocked_account_id
WHERE b.author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
);
END;
$$ LANGUAGE PLPGSQL STRICT STABLE SECURITY DEFINER;

Expand Down
22 changes: 22 additions & 0 deletions src/deploy/table_account_block.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
-- Deploy maevsi:table_account_block to pg
-- requires: schema_public
-- requires: table_account_public

BEGIN;

CREATE TABLE maevsi.account_block (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
author_account_id UUID NOT NULL REFERENCES maevsi.account(id),
blocked_account_id UUID NOT NULL REFERENCES maevsi.account(id),
created_at TIMESTAMPTZ NOT NULL DEFAULT CURRENT_TIMESTAMP,
UNIQUE (author_account_id, blocked_account_id),
CHECK (author_account_id != blocked_account_id)
);

COMMENT ON TABLE maevsi.account_block IS E'@omit update,delete\nBlocking of an account by another account.';
COMMENT ON COLUMN maevsi.account_block.id IS '@omit create\nThe blocking''s internal id.';
COMMENT ON COLUMN maevsi.account_block.author_account_id IS 'The id of the user who created the blocking.';
COMMENT ON COLUMN maevsi.account_block.blocked_account_id IS 'The id of the account to be blocked.';
COMMENT ON COLUMN maevsi.account_block.created_at IS E'@omit update,delete\nThe timestamp when the blocking was created.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
COMMENT ON COLUMN maevsi.account_block.created_at IS E'@omit update,delete\nThe timestamp when the blocking was created.';
COMMENT ON COLUMN maevsi.account_block.created_at IS E'@omit create,update,delete\nThe timestamp when the blocking was created.';

The created_at value should be set programmatically and not by the user.


COMMIT;
26 changes: 26 additions & 0 deletions src/deploy/table_account_block_policy.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
-- Deploy maevsi:table_account_block_policy to pg
-- requires: schema_public
-- requires: table_account_block
-- requires: role_account

BEGIN;

GRANT INSERT, SELECT ON TABLE maevsi.account_block TO maevsi_account;

ALTER TABLE maevsi.account_block ENABLE ROW LEVEL SECURITY;

-- Only allow inserts for blocked accounts authored by the current user.
CREATE POLICY account_block_insert ON maevsi.account_block FOR INSERT WITH CHECK (
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
AND
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
);

-- Only allow selects for blocked accounts authored by the current user.
CREATE POLICY account_block_select ON maevsi.account_block FOR SELECT USING (
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
AND
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
);

COMMIT;
51 changes: 35 additions & 16 deletions src/deploy/table_contact_policy.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
-- Deploy maevsi:table_contact_policy to pg
-- requires: schema_public
-- requires: table_account_block
-- requires: table_contact
-- requires: role_account
-- requires: role_anonymous
Expand All @@ -12,34 +13,52 @@ GRANT INSERT, UPDATE, DELETE ON TABLE maevsi.contact TO maevsi_account;

ALTER TABLE maevsi.contact ENABLE ROW LEVEL SECURITY;

-- Only display contacts referencing the invoker's account.
-- Only display contacts authored by the invoker's account.
-- Only display contacts for which an accessible invitation exists.
-- Only display contacts referencing the invoker's account, omit contacts authored by a blocked account.
-- Only display contacts authored by the invoker's account, omit contacts referring to a blocked account.
-- Only display contacts for which an accessible invitation exists, omit contacts auhored by a blocked account or referring to a blocked account.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- Only display contacts for which an accessible invitation exists, omit contacts auhored by a blocked account or referring to a blocked account.
-- Only display contacts for which an accessible invitation exists.

The last case is not covered here, it's implicitly covered by the word accessible in the current description. We should just describe the code as it is below here. When we were to change something about the way invitation_contact_ids works, we would surely not remember to update the description here as well.

CREATE POLICY contact_select ON maevsi.contact FOR SELECT USING (
(
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing this IS NOT NULL check allows anonymous users to see all contacts that have no account id assigned, no?

AND (
account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
OR
account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
AND
author_account_id NOT IN (
SELECT blocked_account_id
FROM maevsi.account_block
WHERE author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
)
OR
(
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
)
OR
id IN (SELECT maevsi.invitation_contact_ids())
AND
account_id NOT IN (
SELECT blocked_account_id
FROM maevsi.account_block
WHERE author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
)
OR id IN (SELECT maevsi.invitation_contact_ids())
);

-- Only allow inserts for contacts authored by the invoker's account.
-- Only allow inserts for contacts authored by the invoker's account
-- No blocked account can be invited
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invited?

Suggested change
-- No blocked account can be invited
-- Disallow inserts for contacts that refer to a blocked account.

CREATE POLICY contact_insert ON maevsi.contact FOR INSERT WITH CHECK (
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
AND
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
AND account_id NOT IN (
SELECT blocked_account_id
FROM maevsi.account_block
WHERE author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
);

-- Only allow updates for contacts authored by the invoker's account.
-- No contact referring to a blocked account can be updated
CREATE POLICY contact_update ON maevsi.contact FOR UPDATE USING (
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
AND
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
AND account_id NOT IN (
SELECT blocked_account_id
FROM maevsi.account_block
WHERE author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
);

-- Only allow deletes for contacts authored by the invoker's account except for the own account's contact.
Expand Down
36 changes: 21 additions & 15 deletions src/deploy/table_event_policy.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
-- Deploy maevsi:table_event_policy to pg
-- requires: schema_public
-- requires: table_event
-- requires: table_account_block
-- requires: role_account
-- requires: role_anonymous
-- requires: schema_private
Expand All @@ -13,26 +14,31 @@ GRANT INSERT, UPDATE, DELETE ON TABLE maevsi.event TO maevsi_account;

ALTER TABLE maevsi.event ENABLE ROW LEVEL SECURITY;

-- Only display events that are public and not full.
-- Only display events that are public and not full and not organized by a blocked account
-- Only display events that are organized by oneself.
-- Only display events to which oneself is invited.
-- Only display events to which oneself is invited, but not by an invitation authored by a blocked account
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- Only display events to which oneself is invited, but not by an invitation authored by a blocked account
-- Only display events to which oneself is invited, but not by an invitation authored by a blocked account.

Let's not start changing the format of comments (same for the line above).

CREATE POLICY event_select ON maevsi.event FOR SELECT USING (
-- Below copied to function `maevsi.event_invitee_count_maximum`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still true now?

(
visibility = 'public'
AND
(
invitee_count_maximum IS NULL
OR
invitee_count_maximum > (maevsi.invitee_count(id)) -- Using the function here is required as there would otherwise be infinite recursion.
)
)
OR (
NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL
(
visibility = 'public'
AND
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
(
invitee_count_maximum IS NULL
OR
invitee_count_maximum > (maevsi.invitee_count(id)) -- Using the function here is required as there would otherwise be infinite recursion.
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fyi: changing the indentation while also having changes to the code makes it hard to read the difference for reviewers as you can see in above. The code stays mostly the same here, but there are way more green and red lines than necessary for an addition. Therefore, it's better to change formatting in a separate PR. You can resolve this comment once understood.

AND author_account_id NOT IN (
SELECT blocked_account_id
FROM maevsi.account_block
WHERE author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
)
OR id IN (SELECT maevsi_private.events_invited())
OR (
author_account_id = NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID
)
OR (
id IN (SELECT maevsi_private.events_invited())
)
);

-- Only allow inserts for events authored by the current user.
Expand Down
Loading
Loading