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.

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

fix: revert changes to auth functions #1633

Closed
wants to merge 4 commits into from
Closed

Conversation

J0
Copy link
Contributor

@J0 J0 commented Jun 24, 2024

What kind of change does this PR introduce?

The changes in Auth v2.154.1 are not compatible with the current version of Postgrest and associated features on dashboard, including PostgREST

This is because of changes which affect auth.* functions (e.g. auth.uid()). Ostensibly, newer versions of PostgREST set store claims under request.jwt.claims rather than request.jwt.claim.sub which might have lead to auth.uid() etc returning null instead of the user ID as expected.

We revert the change by reinstating the older version of the functions. We preserve search_path='' though.

For more context see: https://supabase.slack.com/archives/C07A55TKL3S/p1719237535404369

Functions taken from:
auth.jwt() -

comment on function {{ index .Options "Namespace" }}.role() is 'Deprecated. Use auth.jwt() -> ''role'' instead.';

auth.uid()-

create or replace function {{ index .Options "Namespace" }}.email()

auth.email() -

create or replace function {{ index .Options "Namespace" }}.email()

Tested by running:

SELECT
    n.nspname as schema_name,
    p.proname as function_name,
    pg_catalog.pg_get_functiondef(p.oid) as function_definition
FROM
    pg_catalog.pg_proc p
    LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
WHERE
    n.nspname = 'auth'
ORDER BY
    schema_name,
    function_name;

Against the v2.152.0 and doing a diff. Also tested against a staging instance after substituting the placeholder and creating the new functions.

@coveralls
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9647790488

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.674%

Totals Coverage Status
Change from base Build 9596939146: 0.0%
Covered Lines: 8699
Relevant Lines: 15083

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9647826417

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.674%

Totals Coverage Status
Change from base Build 9596939146: 0.0%
Covered Lines: 8699
Relevant Lines: 15083

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9647965343

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.674%

Totals Coverage Status
Change from base Build 9596939146: 0.0%
Covered Lines: 8699
Relevant Lines: 15083

💛 - Coveralls

@J0 J0 marked this pull request as ready for review June 24, 2024 15:24
@J0 J0 requested a review from a team as a code owner June 24, 2024 15:24
Comment on lines +46 to +47
nullif(current_setting('request.jwt.claim', true), ''),
nullif(current_setting('request.jwt.claims', true), '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is it like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the lack of search_path ? Set that to '' the rest should be accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not actually sure that we need to replace .jwt() at all let me check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry just wanted to validate this...

create or replace function {{ index .Options "Namespace" }}.jwt()

@hf
Copy link
Contributor

hf commented Jun 24, 2024

Ugh... what about we just drop this whole search path shenaningans...

I'm worried that replacing a function may drop some grants we can't track.

@J0
Copy link
Contributor Author

J0 commented Jun 24, 2024

As discussed, closing in favour of a revert.

@J0 J0 closed this Jun 24, 2024
@hf hf deleted the j0/update_auth_functions branch June 24, 2024 15:38
@coveralls
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9648187697

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.674%

Totals Coverage Status
Change from base Build 9596939146: 0.0%
Covered Lines: 8699
Relevant Lines: 15083

💛 - Coveralls

J0 added a commit that referenced this pull request Jun 24, 2024
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants