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

Device Access Validator #117

Merged
merged 33 commits into from
Apr 27, 2020
Merged

Device Access Validator #117

merged 33 commits into from
Apr 27, 2020

Conversation

joe94
Copy link
Member

@joe94 joe94 commented Jan 17, 2020 •
edited
Loading

Changes introduced in this PR:

  • JWT Validation for device registration is done through bascule now
  • Device Access Component

TODO:

  • contains (a must for partnerIDs), intersect and equals might be enough for this first PR. Let me know if that's not the case.

  • Update metrics accordingly

  • Unit test updates

@joe94 joe94 added the partner-id work related to config-driven api access restriction to devices connected to XMiDT label Jan 17, 2020
@joe94
Copy link
Member Author

joe94 commented Jan 17, 2020

this fixes #116

@joe94
Copy link
Member Author

joe94 commented Jan 31, 2020

fixes #106

@joe94 joe94 force-pushed the feature/deviceAccessChecks branch from deaec22 to 21331a1 Compare March 2, 2020 21:33
@joe94 joe94 force-pushed the feature/deviceAccessChecks branch from 87a2d0c to 0b5ad52 Compare April 13, 2020 19:28
@joe94 joe94 self-assigned this Apr 13, 2020
@joe94 joe94 changed the title (Work in Progress) Device Access Validator (Work in progress) Device Access Validator Apr 13, 2020
@joe94 joe94 linked an issue Apr 13, 2020 that may be closed by this pull request
Validate partners for a device request #106
Closed
@joe94 joe94 marked this pull request as draft April 13, 2020 19:39
kristinapathak
kristinapathak reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@kristinapathak kristinapathak left a comment

Choose a reason for hiding this comment

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

Looks good so far! We've talked about some stuff and I left some comments in the code.

checks.go Outdated Show resolved Hide resolved
checks.go Outdated Show resolved Hide resolved
deviceAccess.go Outdated Show resolved Hide resolved
deviceAccess.go Outdated Show resolved Hide resolved
deviceAccess.go Outdated Show resolved Hide resolved
deviceAccess.go Outdated
// values presented by API users against those of the device
type deviceAccessCheck struct {
Name string
//UserCredentialPath is the Sep-delimited path to the credential value
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a space between the // and the beginning of the comment for easier readability (not marking all of them in this file).

deviceAccess.go Outdated Show resolved Hide resolved
deviceAccess.go Show resolved Hide resolved
deviceAccess.go Outdated Show resolved Hide resolved
primaryHandler.go Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 17, 2020 •
edited
Loading

Codecov Report

Merging #117 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   58.44%   58.44%           
=======================================
  Files          14       14           
  Lines         977      977           
=======================================
  Hits          571      571           
  Misses        397      397           
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8326d9...d8326d9. Read the comment docs.

@joe94 joe94 changed the title (Work in progress) Device Access Validator Device Access Validator Apr 24, 2020
@joe94 joe94 requested a review from johnabass April 24, 2020 16:56
@joe94 joe94 marked this pull request as ready for review April 24, 2020 16:56
kristinapathak
kristinapathak reviewed Apr 25, 2020
View reviewed changes
Copy link
Contributor

@kristinapathak kristinapathak left a comment

Choose a reason for hiding this comment

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

Looks great! Just one tiny thing.

main.go Outdated
Comment on lines 194 to 195
// We are getting bombarded with SIGURGS due to Go1.14's new way to async preempt goroutines https://github.com/golang/go/issues/37942
// Don't log as info as it will fill log with unnecessary entries
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make an issue in other service repos to fix this there as well.

primaryHandler.go Outdated
parsedChecks = append(parsedChecks, parsedCheck)
}

if config.Type == "enforce" || config.Type == "monitor" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code could be flatter by checking at the beginning of this function to verify that the type is "enforce" or "monitor", and returning an error if it's not either. Then, the positive case will be the one returned at the end of this function. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, thanks!

@joe94 joe94 linked an issue Apr 25, 2020 that may be closed by this pull request
Many SIGURG signals sent with go1.14 #126
Closed
johnabass
johnabass approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@johnabass johnabass left a comment

Choose a reason for hiding this comment

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

Looks good!

@joe94 joe94 merged commit e6e83c9 into master Apr 27, 2020
@joe94 joe94 deleted the feature/deviceAccessChecks branch April 27, 2020 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristinapathak kristinapathak kristinapathak left review comments

@johnabass johnabass johnabass approved these changes

Assignees

@joe94 joe94

Labels
partner-id work related to config-driven api access restriction to devices connected to XMiDT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Many SIGURG signals sent with go1.14 Validate partners for a device request
4 participants
@joe94 @codecov-io @johnabass @kristinapathak