Skip to content

Fix context sql #2344

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
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix context sql #2344

wants to merge 11 commits into from

Conversation

M4Al
Copy link

@M4Al M4Al commented Aug 20, 2024

Why make this change?

What is this change?

The changes itself are quite simple and should break nothing, see the diff :-)

How was this tested?

I have not created/adapted any tests yet. Just looking for comments before I go there.

Sample Request(s)

Sample of a JWT token (only the relevant part)

{
  "aud": "api://ddcf6b31-5d01-407d-97cf-8efefc455d32",
  "iss": "https://sts.windows.net/9215c785-95c3-49b0-bdba-2062df5aedb5/",
  "roles": [
    "user",
    "Allow_Customer_OPS025235",
    "Allow_Customer_OPS004095"
  ],
  "ver": "1.0"
}

X-MS-API-ROLE: user

before my change the extra 'roles' that do not match the X-MS-API-ROLE header would never reach the database context.
With my change you can do things like this in SQL Predicates to filter out only subsets of the data:

CREATE FUNCTION dbo.ops_fact_order_Predicate(@CustomerNo varchar(max))
RETURNS TABLE
WITH SCHEMABINDING
AS RETURN SELECT 1 AS fn_securitypredicate_result
WHERE @CustomerNo in (
		select trim(replace(replace(replace([value], '"', ''), ']', ''), 'Allow_Customer_', '')) 
		from STRING_SPLIT ( 
			CAST(SESSION_CONTEXT(N'original_roles') as varchar(max)) 
			, ',' 
			, 0) 
			where trim(replace(replace([value], '"', ''), ']', '')) like 'Allow_Customer%'
		)

CREATE SECURITY POLICY dbo.ops_fact_order_Policy
ADD FILTER PREDICATE dbo.ops_fact_order_Predicate(CustomerNo)
ON [gold_ops].[ops_fact_order];

Aniruddh25
Aniruddh25 reviewed Aug 30, 2024
View reviewed changes
Aniruddh25
Aniruddh25 reviewed Aug 30, 2024
View reviewed changes
src/Core/Resolvers/MsSqlQueryExecutor.cs
// Append statement to set read only param value - can be set only once for a connection.
string statementToSetReadOnlyParam = "EXEC sp_set_session_context " + $"'{claimType}', " + paramName + ", @read_only = 1;";
string statementToSetReadOnlyParam = "EXEC sp_set_session_context " + $"'{claimType}', " + paramName + ", @read_only = 0;";
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, did you experience - your mutation operations being blocked because of this setting?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the SQL Endpoint did reject the second query as described in #2341

Aniruddh25
Aniruddh25 approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Thanks for this smart change to provide the original roles for additional filtering. Left a few nit comments and some questions, looks good to merge otherwise!

@Aniruddh25
Copy link
Contributor

/azp run

Aniruddh25
Aniruddh25 reviewed Sep 6, 2024
View reviewed changes
src/Core/Parsers/RequestParser.cs Outdated
@@ -30,7 +30,7 @@ public class RequestParser
/// <summary>
/// Prefix used for specifying limit in the query string of the URL.
/// </summary>
public const string FIRST_URL = "$first";
public const string FIRST_URL = "$top";
Copy link
Contributor

Choose a reason for hiding this comment

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

We will provide a synonym $top soon. Lets not make this change in this PR. Thank you for your patience.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related #2474

Aniruddh25
Aniruddh25 requested changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

The original PR contained fewer changes, please restrict the changes to the purpose of the PR and lets not add additional changes in the same PR. Waiting for removing the unnecessary changes

Aniruddh25
Aniruddh25 reviewed Sep 9, 2024
View reviewed changes
@JerryNixon JerryNixon mentioned this pull request Nov 25, 2024
Open
@abhishekkumams
Copy link
Contributor

@M4Al , let us know if you would be able to address the comments...

@JerryNixon
Copy link
Contributor

@Aniruddh25 is this something we could wrap?

@JerryNixon JerryNixon requested a review from Aniruddh25 March 26, 2025 20:43
@Aniruddh25
Copy link
Contributor

Aniruddh25 commented Mar 28, 2025 •
edited
Loading

@Aniruddh25 is this something we could wrap?

The original PR was useful. It now contains additional changes which are not really necessary like $top, DwSQL builder changes. We need to clean those up before merging this in.
@RubenCerna2079, could you please take this task up?

@Aniruddh25
Copy link
Contributor

@M4Al, we will take this PR forward and merge from here.

Mu4all and others added 7 commits May 21, 2025 17:44
@Mu4all @RubenCerna2079
pp
60dd852
@Mu4all @RubenCerna2079
Preserve original roles claim and allow session context updates
0d86a3f 
Added a new constant `ORIGINAL_ROLE_CLAIM_TYPE` in `AuthenticationOptions.cs` to store the original roles claim type. Modified `AuthorizationResolver` to preserve the original 'roles' claim by adding it to the `resolvedClaims` dictionary under the new key. Changed `MsSqlQueryExecutor` to set session context parameters with `@read_only = 0` to allow modifications.
@Mu4all @RubenCerna2079
Revert "pp"
bc468ed 
This reverts commit 08f741c.
@Mu4all @RubenCerna2079
Adjust the relevant tests
ab4fe75
@Mu4all @RubenCerna2079
Add recordcount
93a27dc
@M4Al @Aniruddh25 @RubenCerna2079
Update src/Core/Resolvers/MsSqlQueryExecutor.cs
ec0365b 
remove trailing space

Co-authored-by: Aniruddh Munde <[email protected]>
@Mu4all @RubenCerna2079
Update dockerfile to .net8
2cecda7
@Mu4all @RubenCerna2079
Rename FIRST_URL constant value from "$first" to "$top"
de8752e 
Updated the constant `FIRST_URL` in `RequestParser.cs` within the
`Azure.DataApiBuilder.Core.Parsers` namespace to use the value `"$top"`
instead of `"$first"`. This change aligns with naming conventions or
standards used elsewhere in the codebase or API, ensuring consistency
and improving clarity for developers.
@RubenCerna2079
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JerryNixon JerryNixon JerryNixon left review comments

@seantleonard seantleonard Awaiting requested review from seantleonard

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@abhishekkumams abhishekkumams Awaiting requested review from abhishekkumams

@RubenCerna2079 RubenCerna2079 Awaiting requested review from RubenCerna2079 RubenCerna2079 is a code owner

@ravishetye ravishetye Awaiting requested review from ravishetye ravishetye is a code owner

@rohkhann rohkhann Awaiting requested review from rohkhann

@neeraj-sharma2592 neeraj-sharma2592 Awaiting requested review from neeraj-sharma2592 neeraj-sharma2592 is a code owner

@sourabh1007 sourabh1007 Awaiting requested review from sourabh1007 sourabh1007 is a code owner

@Aniruddh25 Aniruddh25 Awaiting requested review from Aniruddh25 Aniruddh25 is a code owner

@sezal98 sezal98 Awaiting requested review from sezal98 sezal98 is a code owner

@souvikghosh04 souvikghosh04 Awaiting requested review from souvikghosh04 souvikghosh04 is a code owner

@vadeveka vadeveka Awaiting requested review from vadeveka vadeveka is a code owner

@Alekhya-Polavarapu Alekhya-Polavarapu Awaiting requested review from Alekhya-Polavarapu Alekhya-Polavarapu is a code owner

@rusamant rusamant Awaiting requested review from rusamant rusamant is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@RubenCerna2079 RubenCerna2079

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: MSSQL (and related) - EXEC sp_set_session_context is executed with @read_only = 1
6 participants
@M4Al @Aniruddh25 @abhishekkumams @JerryNixon @RubenCerna2079 @Mu4all