Skip to content

Save a ServiceProvider.find query during authentication#6091

Merged
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/save-a-query-5
Mar 18, 2022
Merged

Save a ServiceProvider.find query during authentication#6091
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/save-a-query-5

Conversation

@mitchellhenke
Copy link
Contributor

Eventually we'll find them all (followup to #6058, #6061, and #6087)

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we could move sp_from_request_issuer_logout to the SAML controller, since it depends on saml_request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to untangle a bit, and wasn't sure where to start safely, but that's a good idea. Made an attempt in e45a5c3

Fwiw, this implementation stems from #3460 and #3461, so my understanding is the only thing depending on sp_from_request_issuer_logout should be the redirect uris in unauthenticated SAML logout contexts

Copy link
Contributor

Choose a reason for hiding this comment

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

just to check my understanding, this isn't saving a query, this is just making a method definition more explicit, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I got a bit frustrated trying to find where sp_redirect_uris was defined since it's a few layers beneath decorated_session

@aduth
Copy link
Contributor

aduth commented Mar 18, 2022

Thought: Could we have a spec which asserts that only X queries (1?) are made during an authentication?

Mitchell Henke added 2 commits March 18, 2022 12:27
changelog: Internal, Performance, Re-use existing database query results to avoid duplicative work
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/save-a-query-5 branch from 66c4043 to be8a3fa Compare March 18, 2022 17:27
@zachmargolis
Copy link
Contributor

Thought: Could we have a spec which asserts that only X queries (1?) are made during an authentication?

one thing I've seen other projects use is hooking into ActiveSupport notifications and tracking queries that way

Maybe we combine the raw SQL with something like this https://github.com/pganalyze/pg_query#extracting-tables-from-a-query

@mitchellhenke mitchellhenke marked this pull request as ready for review March 18, 2022 17:35
@mitchellhenke
Copy link
Contributor Author

Thought: Could we have a spec which asserts that only X queries (1?) are made during an authentication?

one thing I've seen other projects use is hooking into ActiveSupport notifications and tracking queries that way

Maybe we combine the raw SQL with something like this https://github.com/pganalyze/pg_query#extracting-tables-from-a-query

I love this idea. I do have some minor worries about making tests too restrictive, but it'd be cool to explore this and see what's possible.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/save-a-query-5 branch from e45a5c3 to 3a405b4 Compare March 18, 2022 17:43
@mitchellhenke mitchellhenke merged commit b813c8c into main Mar 18, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/save-a-query-5 branch March 18, 2022 18:04
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