-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[PM-15015] Add Country Name to auth request from request headers #5471
base: main
Are you sure you want to change the base?
Conversation
Added Requesting country name to pending auth requests for organizations.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5471 +/- ##
==========================================
+ Coverage 44.45% 47.50% +3.04%
==========================================
Files 1530 1534 +4
Lines 71003 70513 -490
Branches 6373 6317 -56
==========================================
+ Hits 31564 33496 +1932
+ Misses 38072 35621 -2451
- Partials 1367 1396 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
New Issues (10)Checkmarx found the following issues in this Pull Request
Fixed Issues (27)Great job! The following issues were fixed in this Pull Request
|
fix : updated sql project stored procedures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional changes look great! If you could take a look at some of the tabbing on your sql files that'd be great, maybe I'm mistaken about some of the tabbing but a once over looks like a good idea.
Did you test this on all the supported dbs?
[dbo].[AuthRequest] | ||
SET | ||
[UserId] = @UserId, | ||
[Type] = @Type, | ||
[OrganizationId] = @OrganizationId, | ||
[RequestDeviceIdentifier] = @RequestDeviceIdentifier, | ||
[RequestDeviceType] = @RequestDeviceType, | ||
[RequestIpAddress] = @RequestIpAddress, | ||
[RequestCountryName] = @RequestCountryName, | ||
[ResponseDeviceId] = @ResponseDeviceId, | ||
[AccessCode] = @AccessCode, | ||
[PublicKey] = @PublicKey, | ||
[Key] = @Key, | ||
[MasterPasswordHash] = @MasterPasswordHash, | ||
[Approved] = @Approved, | ||
[CreationDate] = @CreationDate, | ||
[ResponseDate] = @ResponseDate, | ||
[AuthenticationDate] = @AuthenticationDate | ||
WHERE | ||
[Id] = @Id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 Is this correct tabbing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
( | ||
@Id, | ||
@UserId, | ||
@OrganizationId, | ||
@Type, | ||
@RequestDeviceIdentifier, | ||
@RequestDeviceType, | ||
@RequestIpAddress, | ||
@RequestCountryName, | ||
@ResponseDeviceId, | ||
@AccessCode, | ||
@PublicKey, | ||
@Key, | ||
@MasterPasswordHash, | ||
@Approved, | ||
@CreationDate, | ||
@ResponseDate, | ||
@AuthenticationDate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 Is this the correct tabbing you want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this file, yes. I'm using prettier in vscode and the auto-formatting does this with the 4-space tab as the file default.
@@ -0,0 +1,168 @@ | |||
ALTER TABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 This sql file uses 2 space tabbing while I think most of the others use 4. Do we want to keep it consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
🎟️ Tracking
PM-15015
Client PR
📔 Objective
We are adding country name from the request header to the
AuthRequest
for better readability for users trying to decide to approve an auth request.📸 Screenshots
Firefox request with header value
firefox_JArBa59XbO.mp4
Video shows the addition of the
country-name
header value and how the value is saved to the database.Client PR
Screen shots can also be seen in the Client PR
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes