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.

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

CP-1104 Add noCredentials config option. #6

Merged
merged 2 commits into from
Nov 11, 2015
Merged

CP-1104 Add noCredentials config option. #6

merged 2 commits into from
Nov 11, 2015

Conversation

evanweible-wf
Copy link
Contributor

Depends on #3

Issue

Changes

Source:

  • Add a config option to specify whether or not the SockJS client needs to set withCredentials = true

Tests:

  • Existing tests updated to set noCredentials to true since they are not dealing with credentials

Areas of Regression

  • Any XHR request

Testing

  • pub run dart_dev test --integration passes

Code Review

@trentgrover-wf
@maxwellpeterson-wf
@dustinlessard-wf
@jayudey-wf

@rmconsole-wf
Copy link

General Information

Ticket(s): CP-1104
Code Review(s): #6
CI:
Automation: Could not find automation run associated with this pull.

SOC 1 Checklist

  • All commits reviewed
  • QA Review
  • Security review completed for all commits if applicable: (N/A)

RM Checklist

  • Ticket(s) Verified
  • Smithy build successful (no build info found)
  • Smithy run in past 24 hours (no build info found)
  • Smithy run against most recent commit (no build info found)
  • Total danger score is less than 500 and each file has danger score less than 100
  • Unit Tests Updated
    • Tests updated for .dart code changes
    • Tests updated for .js code changes

Additional Information

Reviewers: dustinlessard-wf, jayudey-wf, maxwellpeterson-wf, trentgrover-wf
Watchlist Notifications: None

Automatically Identified Dependencies: smithy not yet complete

    When this pull is merged I will use the following information:
    Version: sockjs-dart-client 1.0.0
    Release Ticket(s): CP-1096

Last updated on Wednesday, November 11 04:20 PM CST

@maxwellpeterson-wf
Copy link
Member

+1

@evanweible-wf evanweible-wf changed the title Add close() to client. Add integration tests. Add noCredentials config option. Nov 11, 2015
@dustinlessard-wf
Copy link

+1

trentgrover-wf
trentgrover-wf reviewed Nov 11, 2015
View reviewed changes
lib/src/ajax.dart
XHRCorsObject(method, url, payload, {noCredentials, headers} ) {
Timer.run(() =>_start(method, url, payload, noCredentials: false));
XHRCorsObject(method, url, {headers, noCredentials, payload}) {
Timer.run(() =>_start(method, url, payload, noCredentials: noCredentials != null ? noCredentials : false));
Copy link

Choose a reason for hiding this comment

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

I think you can use this null aware op for this bit -> x ?? y : z

Copy link

Choose a reason for hiding this comment

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

if i read that right -> noCredentials: noCredentials ?? false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently this project has an SDK constraint of >=1.0.0 <2.0.0. If we still want to try to PR this into the main one, that would be a breaking change. I'm okay with doing it though, especially if we just end up using this fork

Copy link

Choose a reason for hiding this comment

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

let's skip it for now

@trentgrover-wf
Copy link

+1 unless you want to change the null aware bit

@trentgrover-wf
Copy link

@jayudey-wf ready for merge after #3

@jayudey-wf jayudey-wf changed the title Add noCredentials config option. CP-1104 Add noCredentials config option. Nov 11, 2015
@jayudey-wf
Copy link
Contributor

QA Resource Approval: +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • verified that test and coverage tasks work as expected
  • Unit test created/updated
  • All unit tests pass

Merging into master.

jayudey-wf added a commit that referenced this pull request Nov 11, 2015
@jayudey-wf
Merge pull request #6 from Workiva/cors
fb1e684 
CP-1104 Add `noCredentials` config option.
@jayudey-wf jayudey-wf merged commit fb1e684 into master Nov 11, 2015
This was referenced Nov 25, 2015
Closed
Merged
@travisreed-wf
Copy link

Infosec has requested a security review on this pull request

@evanweible-wf @jayudey-wf @maxwellpeterson-wf can you please coordinate that?

@rmconsole3-wf
Copy link
Contributor

@evanweible-wf

  • A security review has not been completed, but one is required. Please request a security review from one of the approved security reviewers listed here.

@jayudey-wf
Copy link
Contributor

@michaeldavis-wf @richklein-wf @chadknight-wf could one of you provide a security review of this?

@michaeldavis-wf
Copy link

+1 security

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Security Review Required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@evanweible-wf @rmconsole-wf @maxwellpeterson-wf @dustinlessard-wf @trentgrover-wf @jayudey-wf @travisreed-wf @rmconsole3-wf @michaeldavis-wf