Skip to content

Commit

Permalink
[SUSTAIN-632] Do not update the users table to contain the public key.
Browse files Browse the repository at this point in the history
The keys table should be the only source of truth
New insert/updates to the users table will insert a sentinel value in the public_key column.
The old values are left as is.
Triggers will act only on the values in the users.public_key that are not the sentinel value.

Signed-off-by: Prajakta Purohit <[email protected]>
  • Loading branch information
PrajaktaPurohit committed Sep 1, 2017
1 parent 3c916eb commit e1a5066
Show file tree
Hide file tree
Showing 15 changed files with 295 additions and 69 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
define_upgrade do

if Partybus.config.bootstrap_server
must_be_data_master

# schema update no longer insert public_key in users table.
# the keys table is the only source of truth
run_sqitch('@sentinel_public_key_for_users', 'oc_erchef')
end
end
44 changes: 4 additions & 40 deletions src/oc_erchef/apps/chef_db/priv/pgsql_statements.config
Original file line number Diff line number Diff line change
Expand Up @@ -484,17 +484,10 @@
%% User-related
%%
{insert_user,
<<"INSERT INTO users (id, authz_id, username, email, hashed_password, salt,
hash_type, last_updated_by, created_at, updated_at, external_authentication_uid,
recovery_authentication_enabled, serialized_object, admin, pubkey_version) VALUES
($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, false, 0)">>}.
<<"SELECT add_user($1, $2, $3, $4, 'this_is_not_a_key', 0, $5, $6, $7, $8, $9, $10, $11, $12, $13, false)">>}.

{insert_user_v0,
<<"INSERT INTO users (id, authz_id, username, email, public_key, pubkey_version,
hashed_password, salt, hash_type, last_updated_by, created_at,
updated_at, external_authentication_uid,
recovery_authentication_enabled, serialized_object, admin) VALUES
($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, false)">>}.
<<"SELECT add_user($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, false)">>}.

{find_user_by_username_v0,
<<"SELECT u.id, authz_id, username, email, k.public_key, k.key_version pubkey_version,
Expand Down Expand Up @@ -560,22 +553,7 @@
{list_users_by_ext_auth_uid, <<"SELECT username FROM users WHERE external_authentication_uid = $1">>}.

{update_user_by_id_v0,
<<"UPDATE users
SET pubkey_version = $1,
public_key = $2,
hashed_password = $3,
salt = $4,
hash_type = $5,
serialized_object = $6,
external_authentication_uid = $7,
recovery_authentication_enabled = $8,
email = $9,
username = $10,
last_updated_by = $11,
updated_at = $12,
admin = false
WHERE id = $13">>
}.
<<"SELECT update_user($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, false, $13)">>}.

{list_common_orgs_for_users,
<<"SELECT o.name as name, o.full_name as full_name, o.id as guid"
Expand All @@ -587,18 +565,4 @@
}.

{update_user_by_id,
<<"UPDATE users
SET hashed_password = $1,
salt = $2,
hash_type = $3,
serialized_object = $4,
external_authentication_uid = $5,
recovery_authentication_enabled = $6,
email = $7,
username = $8,
last_updated_by = $9,
updated_at = $10,
admin = false,
pubkey_version = 0
WHERE id = $11">>
}.
<<"SELECT update_user(0, 'this_is_not_a_key', $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, false, $11)">>}.
3 changes: 2 additions & 1 deletion src/oc_erchef/apps/chef_db/src/chef_db.erl
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ make_context(ApiVersion, ReqId) ->
make_context(ApiVersion, ReqId, Darklaunch) ->
#context{server_api_version = ApiVersion, reqid = ReqId, darklaunch = Darklaunch}.


-spec create(object_rec(), #context{}, object_id()) -> ok | {conflict, term()} | {error, term()}.
create(#chef_cookbook_version{} = Record, DbContext, ActorId) ->
create_object(DbContext, create_cookbook_version, Record, ActorId);
Expand All @@ -168,6 +167,7 @@ create(ObjectRec0, #context{server_api_version = ApiVersion, reqid = ReqId}, Act
case stats_hero:ctime(ReqId, {chef_sql, create_object},
fun() -> chef_sql:create_object(QueryName, Fields) end) of
{ok, 1} -> ok;
{ok, 0} -> <<"Record not created '", ObjectRec2/binary, "'.">>;
{conflict, Msg}-> {conflict, Msg};
{error, Why} -> {error, Why}
end.
Expand Down Expand Up @@ -258,6 +258,7 @@ update(#chef_cookbook_version{org_id =OrgId} = Record, #context{reqid = ReqId} =
update(ObjectRec, #context{reqid = ReqId}, ActorId) ->
case stats_hero:ctime(ReqId, {chef_sql, do_update},
fun() -> chef_sql:update(ObjectRec, ActorId) end) of
0 -> {error, <<"Record not updated '", ObjectRec/binary, "'.">>};
N when is_integer(N), N > 0 -> ok;
not_found -> not_found;
{conflict, Message} -> {conflict, Message};
Expand Down
24 changes: 23 additions & 1 deletion src/oc_erchef/apps/chef_db/src/chef_sql.erl
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,10 @@ create_object(#chef_sandbox{id=SandboxId,
%% the failure back up
Error
end.
-spec create_object(atom(), tuple() | list()) -> {ok, non_neg_integer()} | {error, term()} | {conflict, term()}.
-spec create_object(atom(), tuple() | list()) -> {ok, non_neg_integer()} |
{ok,[[{binary(),<<>>}]]} |
{error, term()} |
{conflict, term()}.
%% okay, that's pretty ugly, but no more so than all the hacks in here and
%% chef_db for plain old cookbooks, their versions, and their checksums
%% and at least, it's in a transaction
Expand All @@ -1003,6 +1006,15 @@ create_object(insert_cookbook_artifact_version = QueryName, Args) when is_list(A
{error, _Why} = Error -> Error;
{conflict, _Why} = Conflict -> Conflict
end;
create_object(QueryName, Args) when QueryName =:= insert_user;
QueryName =:= insert_user_v0,
is_list(Args) ->
Foo = sqerl:select(QueryName, Args, first_as_scalar, [add_user]),
case Foo of
{ok, 1} -> {ok, 1};
{ok, 0} -> {error, <<"Record not created '", QueryName/binary, "'.">>};
Error -> Error
end;
create_object(QueryName, Args) when is_atom(QueryName), is_list(Args) ->
sqerl:statement(QueryName, Args, count);
create_object(QueryName, Record) when is_atom(QueryName) ->
Expand Down Expand Up @@ -1081,6 +1093,16 @@ delete_object(delete_cookbook_by_orgid_name = Query, OrgId, Name) ->
update(ObjectRec, ActorId) ->
chef_object:update(ObjectRec, ActorId, fun select_rows/1).

do_update(QueryName, UpdateFields) when QueryName =:= update_user_by_id;
QueryName =:= update_user_by_id_v0,
is_list(UpdateFields) ->
case sqerl:select(QueryName, UpdateFields, first_as_scalar, [update_user]) of
{ok, 1} -> {ok, 1};
{ok, 0} -> {error, <<"Record not updated '", QueryName/binary, "'.">>};
{ok, none} -> {ok, not_found};
Error ->
Error
end;
do_update(QueryName, UpdateFields) ->
case sqerl:statement(QueryName, UpdateFields) of
{ok, 1} -> {ok, 1};
Expand Down
6 changes: 4 additions & 2 deletions src/oc_erchef/apps/chef_objects/src/chef_user.erl
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
set_updated/2,
set_api_version/2,
type_name/1,
update/2,
update_from_ejson/2,
validate_user_name/1,
serialized_field_value/2,
Expand All @@ -62,8 +63,6 @@
update_query/1
]).

-mixin([{chef_object_default_callbacks, [ update/2 ]}]).

-behaviour(chef_object).

%% Whitelist of fields we will persist to serialized_object. Fields not in the list
Expand Down Expand Up @@ -376,6 +375,9 @@ set_password_data(#chef_user{}=User, {HashedPassword, Salt, HashType}) ->
salt = Salt,
hash_type = HashType}.

update(Rec, CallbackFun) ->
CallbackFun({update_query(Rec), fields_for_update(Rec), {first_as_scalar, [update_user]}}).

%% @doc Return a new `chef_user()' record updated according to the specified EJSON
%% terms. This provides behavior similar to chef_objects:update_from_ejson()
-spec update_from_ejson(#chef_user{}, ejson_term()) -> #chef_user{}.
Expand Down
5 changes: 3 additions & 2 deletions src/oc_erchef/apps/chef_test/src/chef_test_db_helper.erl
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,10 @@ start_db(Config, DbName) ->

%% FIXME: we should fail explicitely here if any of the commands above
%% fail
CmdsResult = chef_test_suite_helper:run_cmds(CMDS),
chef_test_suite_helper:run_cmds(CMDS),
% make sure it's seen in output, don't use lager.
ct:pal("db_start: ~n~p~n", [CmdsResult]),
%CmdsResult = chef_test_suite_helper:run_cmds(CMDS),
%io:format(user, "db_start: ~n~p~n", [CmdsResult]),

Statements = case ?config(statements, Config) of
undefined ->
Expand Down
104 changes: 104 additions & 0 deletions src/oc_erchef/schema/deploy/create_and_update_users.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
-- Deploy enterprise_chef:create_and_update_users to pg
-- Inserts a sentinel value into the public_key field of a user table.
-- Ensures keys table is the only source of truth for public_key

BEGIN;

DROP FUNCTION If EXISTS add_user(character,character,text,text,text,integer,text,text,password_hash_type,character,timestamp without time zone,timestamp without time zone,text,boolean,text,boolean);

CREATE OR REPLACE FUNCTION add_user(p_id users.id%TYPE,
p_authz_id users.authz_id%TYPE,
p_username users.username%TYPE,
p_email users.email%TYPE,
p_public_key users.public_key%TYPE,
p_pubkey_version users.pubkey_version%TYPE,
p_hashed_password users.hashed_password%TYPE,
p_salt users.salt%TYPE,
p_hash_type users.hash_type%TYPE,
p_last_updated_by users.last_updated_by%TYPE,
p_created_at users.created_at%TYPE,
p_updated_at users.updated_at%TYPE,
p_external_authentication_uid users.external_authentication_uid%TYPE,
p_recovery_authentication_enabled users.recovery_authentication_enabled%TYPE,
p_serialized_object users.serialized_object%TYPE,
p_admin users.admin%TYPE) RETURNS integer AS $$
DECLARE
inserteduser integer;
BEGIN
IF p_public_key != 'this_is_not_a_key' THEN
INSERT INTO keys
(id, key_name, public_key, key_version, created_at, expires_at)
VALUES
(p_id, 'default', p_public_key, p_pubkey_version, now(), 'infinity'::timestamp);
END IF;
WITH createduser AS
(INSERT INTO users
(id, authz_id, username, email, public_key, hashed_password, salt, hash_type,
last_updated_by, created_at, updated_at, external_authentication_uid,
recovery_authentication_enabled, serialized_object, admin,
pubkey_version)
VALUES (p_id, p_authz_id, p_username, p_email, 'this_is_not_a_key',
p_hashed_password, p_salt, p_hash_type, p_last_updated_by, p_created_at, p_updated_at,
p_external_authentication_uid, p_recovery_authentication_enabled,
p_serialized_object, p_admin, p_pubkey_version) RETURNING 1)
SELECT count(*) FROM createduser INTO inserteduser;
RETURN inserteduser;
END;
$$ LANGUAGE plpgsql;

DROP FUNCTION IF EXISTS update_user(integer,text,text,text,password_hash_type,text,text,boolean,text,text,character,timestamp without time zone,boolean,character);

CREATE OR REPLACE FUNCTION update_user(p_pubkey_version users.pubkey_version%TYPE,
p_public_key users.public_key%TYPE,
p_hashed_password users.hashed_password%TYPE,
p_salt users.salt%TYPE,
p_hash_type users.hash_type%TYPE,
p_serialized_object users.serialized_object%TYPE,
p_external_authentication_uid users.external_authentication_uid%TYPE,
p_recovery_authentication_enabled users.recovery_authentication_enabled%TYPE,
p_email users.email%TYPE,
p_username users.username%TYPE,
p_last_updated_by users.last_updated_by%TYPE,
p_updated_at users.updated_at%TYPE,
p_admin users.admin%TYPE,
p_id character(32)) RETURNS integer AS $$
DECLARE
updateduser integer;
BEGIN
IF p_public_key != 'this_is_not_a_key' THEN
IF p_public_key IS NOT NULL THEN
UPDATE keys SET public_key = p_public_key,
key_version = p_pubkey_version,
expires_at = 'infinity'::timestamp
WHERE id = p_id AND key_name = 'default';
INSERT INTO keys (id, key_name, public_key, key_version, created_at, expires_at)
SELECT p_id, 'default', p_public_key, p_pubkey_version, now(), 'infinity'::timestamp
WHERE NOT EXISTS (SELECT 1 FROM keys WHERE id = p_id AND key_name = 'default');
END IF;
-- NULL comparision evaluates to NULL
-- if p_public_key == NULL; fall through to the ELSE case
ELSE
DELETE FROM keys WHERE id = p_id AND key_name = 'default';
END IF;
WITH changeduser AS
(UPDATE users SET
username = p_username,
email = p_email,
public_key = 'this_is_not_a_key',
hashed_password = p_hashed_password,
salt = p_salt,
hash_type = p_hash_type,
last_updated_by = p_last_updated_by,
updated_at = p_updated_at,
external_authentication_uid = p_external_authentication_uid,
recovery_authentication_enabled = p_recovery_authentication_enabled,
serialized_object = p_serialized_object,
admin = p_admin,
pubkey_version = p_pubkey_version
WHERE id = p_id RETURNING 1)
SELECT count(*) FROM changeduser INTO updateduser;
RETURN updateduser;
END
$$ LANGUAGE plpgsql;

COMMIT;
46 changes: 26 additions & 20 deletions src/oc_erchef/schema/deploy/keys_update_trigger.sql
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,35 @@
BEGIN;

CREATE OR REPLACE FUNCTION add_key() RETURNS TRIGGER AS $add_key$
BEGIN
IF NEW.public_key IS NOT NULL THEN
INSERT INTO keys
(id, key_name, public_key, key_version, created_at, expires_at) VALUES
(NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp);
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
BEGIN
IF NEW.public_key != 'thisisnotakey' THEN
IF NEW.public_key IS NOT NULL THEN
INSERT INTO keys
(id, key_name, public_key, key_version, created_at, expires_at) VALUES
(NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp);
END IF;
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$add_key$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION update_key() RETURNS TRIGGER AS $update_key$
BEGIN
IF NEW.public_key IS NOT NULL THEN
UPDATE keys SET public_key = NEW.public_key, key_version = NEW.pubkey_version, expires_at = 'infinity'::timestamp
WHERE id = NEW.id AND key_name = 'default';
INSERT INTO keys (id, key_name, public_key, key_version, created_at, expires_at)
SELECT NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp
WHERE NOT EXISTS (SELECT 1 FROM keys WHERE id = NEW.id AND key_name = 'default');
ELSE
DELETE FROM keys WHERE id = NEW.id AND key_name = 'default';
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
BEGIN
IF NEW.public_key != 'thisisnotakey' THEN
IF NEW.public_key IS NOT NULL THEN
UPDATE keys SET public_key = NEW.public_key,
key_version = NEW.pubkey_version,
expires_at = 'infinity'::timestamp
WHERE id = NEW.id AND key_name = 'default';
INSERT INTO keys (id, key_name, public_key, key_version, created_at, expires_at)
SELECT NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp
WHERE NOT EXISTS (SELECT 1 FROM keys WHERE id = NEW.id AND key_name = 'default');
ELSE
DELETE FROM keys WHERE id = NEW.id AND key_name = 'default';
END IF;
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$update_key$ LANGUAGE plpgsql;

DROP TRIGGER IF EXISTS add_key ON clients;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
-- Deploy keys_update_trigger
--
-- This keeps the keys table consistent when the clients and user tables update their keys
--
BEGIN;

CREATE OR REPLACE FUNCTION add_key() RETURNS TRIGGER AS $add_key$
BEGIN
IF NEW.public_key IS NOT NULL THEN
INSERT INTO keys
(id, key_name, public_key, key_version, created_at, expires_at) VALUES
(NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp);
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$add_key$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION update_key() RETURNS TRIGGER AS $update_key$
BEGIN
IF NEW.public_key IS NOT NULL THEN
UPDATE keys SET public_key = NEW.public_key, key_version = NEW.pubkey_version, expires_at = 'infinity'::timestamp
WHERE id = NEW.id AND key_name = 'default';
INSERT INTO keys (id, key_name, public_key, key_version, created_at, expires_at)
SELECT NEW.id, 'default', NEW.public_key, NEW.pubkey_version, now(), 'infinity'::timestamp
WHERE NOT EXISTS (SELECT 1 FROM keys WHERE id = NEW.id AND key_name = 'default');
ELSE
DELETE FROM keys WHERE id = NEW.id AND key_name = 'default';
END IF;
RETURN NULL; -- result is ignored since this is an AFTER trigger
END;
$update_key$ LANGUAGE plpgsql;

DROP TRIGGER IF EXISTS add_key ON clients;
DROP TRIGGER IF EXISTS update_key ON clients;

DROP TRIGGER IF EXISTS add_key ON users;
DROP TRIGGER IF EXISTS update_key ON users;

CREATE TRIGGER add_key AFTER INSERT ON clients FOR EACH ROW EXECUTE PROCEDURE add_key();
CREATE TRIGGER update_key AFTER UPDATE ON clients FOR EACH ROW EXECUTE PROCEDURE update_key();

CREATE TRIGGER add_key AFTER INSERT ON users FOR EACH ROW EXECUTE PROCEDURE add_key();
CREATE TRIGGER update_key AFTER UPDATE ON users FOR EACH ROW EXECUTE PROCEDURE update_key();

COMMIT;
Loading

0 comments on commit e1a5066

Please sign in to comment.