-
Notifications
You must be signed in to change notification settings - Fork 88
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
add auth page with code snippets #512
Conversation
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.
Some wording comments + code snippet suggestions. None of these are "must fix" or anything, so if you don't agree, feel free to disregard.
public async System.Threading.Tasks.Task LogsOnManyWays() | ||
{ | ||
// :code-block-start: logon_anon | ||
Realms.Sync.User anonUser = await app.LogInAsync(Credentials.Anonymous()); |
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.
Not sure if that's the style of the docs, but I feel that using the fully qualified name is overly verbose and doesn't convey much useful information. It's also not very copy-pasteable as in my experience most projects use var
s for variables and a few use the type name (without the namespace).
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.
@nirinchev - the reason I had the FQDN here was because we have a user-defined class called "User", so the compiler complained. I've updated the custom class names because they are semi-arbitrary.
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.
My suggestion would be to always use var
as the variable name should be indicative enough. E.g. anonUser
is obviously a User, so explicitly specifying the type is redundant. This also lets you avoid some of the type collisions.
source/dotnet/authenticate.txt
Outdated
.. _dotnet-login: | ||
{+service+} provides an API for authenticating users using any enabled | ||
authentication provider. Instantiate a ``Credentials`` object and pass | ||
it to the ``app.login()`` method to authenticate a user and create a ``User`` |
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.
The .NET method is LogInAsync
- I don't think anyone would get confused over it, but it's always better to use the exact method names.
"to authenticate a user and create a User
object" sounds a bit repetitive. Additionally, the "create a User" wording is a bit imprecise as it implies constructing the instance as opposed to obtaining a reference to it by calling a method. Don't have suggestions for avoiding the repetition, but "create a User
object" can probably be reworded as "obtain a User
instance".
source/dotnet/authenticate.txt
Outdated
object. Each authentication provider corresponds to a method used to | ||
instantiate ``Credentials`` objects using that authentication provider. |
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 sounds a bit heavy - perhaps if we inverted the flow, it would be easier to follow. I.e. something like "The Credentials
class exposes factory methods for each authentication provider."
- Credentials Generation Method | ||
|
||
* - :ref:`Anonymous <dotnet-login-anonymous>` | ||
- ``Credentials.Anonymous()`` |
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.
Would be nice if those linked to the API docs for that method as those typically contain valuable information about parameters, return types, etc.
I can send you a .zip containing the API docs for the v10 SDK if that would help.
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, please...or a link would be fine, too.
var functionParameters = new Dictionary<string, string>() | ||
{ | ||
{ "username", "caleb" }, | ||
{ "password", "shhhItsASektrit!" }, | ||
{ "someOtherProperty", "cheesecake" } | ||
}; | ||
Realms.Sync.User functionUser = | ||
await app.LogInAsync(Credentials.Function(functionParameters)); |
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 will work, but the code can be simplified by using anonymous objects:
var functionParameters = new
{
username = "caleb",
password = "shhhItsASektrit",
someOtherProperty = "cheesecake"
};
var functionUser = await app.LogInAsync(Credentials.Function(functionParameters));
Note that when using the anonymous object syntax, you can also hint that properties don't have to be strings only. Any serializable .NET type, including embedded objects will work:
var functionParameters = new
{
biometricsValidated = false,
todaysDate = DateTime.UtcNow
}
Of course, don't have to go overboard with the extra properties, just FYI.
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.
niiiiice
{ | ||
// :code-block-start: logon_google | ||
Realms.Sync.User googleUser = | ||
await app.LogInAsync(Credentials.Google("<google_token>")); |
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.
I think we're calling this google auth code now as using the access token flow doesn't work.
} | ||
catch (Exception e) | ||
{ | ||
Assert.AreEqual("http error code considered fatal: Client Error: 401", e.Message); |
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.
FYI: this message will change when you upgrade to the latest v10 package - the generic wording was due to a bug that has now been addressed and the message should be more meaningful (I don't think that's exposed in the docs, but will break your test, so don't be surprised).
// :code-block-end: | ||
user = app.LogInAsync(Credentials.EmailPassword("[email protected]", "foobar")).Result; | ||
config = new SyncConfiguration("My Project", user); | ||
Realm realm = await Realm.GetInstanceAsync(config); |
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.
Probably move realm
to a class-level variable as well to avoid the Realm realm = await Realm...
repetition.
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.
Since these code snippets live "alone" within the docs, and the user won't be seeing this unit test file as a whole, I think it's useful to have the Realm
there to be clear what we are building.
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.
Right, but it's in the variable name as well - i.e. knowing that I have a variable called realm
of type Realm
isn't that much more useful than just knowing that I have a variable called realm
.
examples/dotnet/Examples/Examples.cs
Outdated
config = new SyncConfiguration("My Project", user); | ||
Realm realm = await Realm.GetInstanceAsync(config); | ||
// :code-block-stat: create | ||
RealmTask testTask = new RealmTask() |
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.
Same here - RealmTask testTask = new RealmTask
is fairly repetitive. Also, you can drop the ()
from the constructor when using the property initializer syntax - there are different opinions about it, but most high profile .NET people seem to omit them.
examples/dotnet/Examples/Examples.cs
Outdated
Realm realm = await Realm.GetInstanceAsync(config); | ||
// :code-block-end: | ||
// :code-block-start: read-all | ||
var tasks = realm.All<RealmTask>().ToList(); |
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.
We shouldn't encourage people to .ToList()
query results as that removes the "liveness" from the collection.
examples/dotnet/Examples/Examples.cs
Outdated
// :code-block-end: | ||
Assert.AreEqual(1, tasks.Count); | ||
// :code-block-start: read-some | ||
tasks = realm.All<RealmTask>().Where(t => t.Status == "Open").ToList(); |
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.
Same here - materializing the collection is generally viewed as a bad practice unless you have very specific needs. Instead, you should operate on the query as much as possible.
{ | ||
username= "caleb", | ||
password = "shhhItsASektrit!", | ||
IQ = 42, |
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.
Hahaha... I guess explains the password 😅
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.
Can you tell I was getting punchy?
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.
Strange, my review seems to never have gone through. Sorry about that.
I'm certain that I typed a message, selected Approve, and submitted, but I don't see that message nor the approval.
Anyway, hope these are helpful.
await functionUser.LogOutAsync(); | ||
// :code-block-start: logon_JWT | ||
Realms.Sync.User jwtUser = | ||
await app.LogInAsync(Credentials.JWT("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkNhbGViIiwiaWF0IjoxNjAxNjc4ODcyLCJleHAiOjI1MTYyMzkwMjIsImF1ZCI6InR1dHMtdGlqeWEifQ.LHbeSI2FDWrlUVOBxe-rasuFiW-etv2Gu5e3eAa6Y6k")); |
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.
Would it be better to call it <JWT_token> similar to the fb and google examples? And is this a real credential that should be removed prior to commit?
|
||
.. _dotnet-login-api-key: | ||
|
||
API Key | ||
~~~~~~~ | ||
If you have enabled :ref:`API Key authentication <api-key-authentication>`, |
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.
Since you've added Authentication to Anonymous and Email/Password headers, should you also update the rest of the authn method headers?
|
||
.. _dotnet-login-custom-function: | ||
|
||
Custom Function | ||
~~~~~~~~~~~~~~~ | ||
If you have enabled the |
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.
Nitpick: I think we prefer past tense over past perfect. E.g. "If you enabled ..." instead of "If you have enabled..." Applies to all instances.
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.
Final nitpick from me.
public async System.Threading.Tasks.Task LogsOnManyWays() | ||
{ | ||
// :code-block-start: logon_anon | ||
User anonUser = await app.LogInAsync(Credentials.Anonymous()); |
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.
I don't think we should prefix the variable name with the provider type - in the iOS/Android docs, we're using user
for all the examples. Since that will result in compilation errors here, you can add a scope around each snippet:
public async System.Threading.Tasks.Task LogsOnManyWays()
{
{
// :code-block-start: logon_anon
var user = await app.LogInAsync(CredentialAnonymous());
// :code-block-end:
Assert.AreEqual(UserState.LoggedIn, user.State);
await user.LogOutAsync();
}
{
// :code-block-start: logon_EP
var user = await app.LogInAsync(
Credentials.EmailPassword("[email protected]", "shhhItsASektrit!"));
// :code-block-end:
Assert.AreEqual(UserState.LoggedIn, user.State);
await user.LogOutAsync();
}
// ...
}
<h3>Snyk has created this PR to upgrade bson from 4.7.0 to 4.7.1.</h3> :information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project. <hr/> - The recommended version is **1 version** ahead of your current version. - The recommended version was released **21 days ago**, on 2023-01-05. <details> <summary><b>Release notes</b></summary> <br/> <details> <summary>Package name: <b>bson</b></summary> <ul> <li> <b>4.7.1</b> - <a href="https://snyk.io/redirect/github/mongodb/js-bson/releases/tag/v4.7.1">2023-01-05</a></br><p>The MongoDB Node.js team is pleased to announce version v4.7.1 of the bson package!</p> <h3>Bug Fixes</h3> <ul> <li><strong><a class="issue-link js-issue-link notranslate" rel="noopener noreferrer nofollow" href="https://jira.mongodb.org/browse/NODE-4905">NODE-4905</a>:</strong> double precision accuracy in canonical EJSON (<a href="https://snyk.io/redirect/github/mongodb/js-bson/issues/549" data-hovercard-type="pull_request" data-hovercard-url="/mongodb/js-bson/pull/549/hovercard">#549</a>) (<a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/d86bd52661e7f5d26479f6b63acac7950f505d69">d86bd52</a>)</li> </ul> <h2>Documentation</h2> <ul> <li>API: <a href="https://snyk.io/redirect/github/mongodb/js-bson#readme">https://github.com/mongodb/js-bson#readme</a></li> <li>Changelog: <a href="https://snyk.io/redirect/github/mongodb/js-bson/blob/4.0/HISTORY.md#change-log">https://github.com/mongodb/js-bson/blob/4.0/HISTORY.md#change-log</a></li> </ul> <p>We invite you to try the bson library immediately, and report any issues to the <a href="https://jira.mongodb.org/projects/NODE" rel="nofollow">NODE project</a>.</p> </li> <li> <b>4.7.0</b> - <a href="https://snyk.io/redirect/github/mongodb/js-bson/releases/tag/v4.7.0">2022-08-18</a></br><p>The MongoDB Node.js team is pleased to announce version 4.7.0 of the bson package!</p> <h2>Release Highlights</h2> <p>This release adds <em>automatic</em> UUID support. Now when serializing or deserializing BSON you can work directly with the UUID type without explicit conversion methods. The UUID class is now a subclass of binary so all existing code will continue to work (including the explicit conversion methods <code>.toUUID</code>/<code>.toBinary</code>). The same automatic support for UUID is also present in EJSON <code>.parse</code>/<code>.stringify</code>.</p> <p>Take a look at the following for the expected behavior:</p> <div class="highlight highlight-source-ts notranslate position-relative overflow-auto" data-snippet-clipboard-copy-content="const document = BSON.deserialize(bytes) // { uuid: UUID('xxx') } BSON.serialize(document) // Buffer < document with uuid (binary subtype 4) >"><pre><span class="pl-k">const</span> <span class="pl-smi">document</span> <span class="pl-c1">=</span> <span class="pl-smi">BSON</span><span class="pl-kos">.</span><span class="pl-en">deserialize</span><span class="pl-kos">(</span><span class="pl-s1">bytes</span><span class="pl-kos">)</span> <span class="pl-c">// { uuid: UUID('xxx') }</span> <span class="pl-smi">BSON</span><span class="pl-kos">.</span><span class="pl-en">serialize</span><span class="pl-kos">(</span><span class="pl-smi">document</span><span class="pl-kos">)</span> <span class="pl-c">// Buffer < document with uuid (binary subtype 4) ></span></pre></div> <p>Special thanks to <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/aditi-khare-mongoDB/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/aditi-khare-mongoDB">@ aditi-khare-mongoDB</a> for all her hard work on this feature!! <g-emoji class="g-emoji" alias="tada" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f389.png">🎉</g-emoji></p> <h3>Features</h3> <ul> <li><strong><a class="issue-link js-issue-link notranslate" rel="noopener noreferrer nofollow" href="https://jira.mongodb.org/browse/NODE-4405">NODE-4405</a>:</strong> support serializing UUID class (<a href="https://snyk.io/redirect/github/mongodb/js-bson/issues/508" data-hovercard-type="pull_request" data-hovercard-url="/mongodb/js-bson/pull/508/hovercard">#508</a>) (<a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/f5dc9edf915cc119f02f53ec84d1c640695dced7">f5dc9ed</a>)</li> <li><strong><a class="issue-link js-issue-link notranslate" rel="noopener noreferrer nofollow" href="https://jira.mongodb.org/browse/NODE-4419">NODE-4419</a>:</strong> UUID class deserialization (<a href="https://snyk.io/redirect/github/mongodb/js-bson/issues/509" data-hovercard-type="pull_request" data-hovercard-url="/mongodb/js-bson/pull/509/hovercard">#509</a>) (<a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/ff2b97585848730fcf90cd21c14ba2a18a0ed016">ff2b975</a>)</li> <li><strong><a class="issue-link js-issue-link notranslate" rel="noopener noreferrer nofollow" href="https://jira.mongodb.org/browse/NODE-4506">NODE-4506</a>:</strong> Make UUID a subclass of binary (<a href="https://snyk.io/redirect/github/mongodb/js-bson/issues/512" data-hovercard-type="pull_request" data-hovercard-url="/mongodb/js-bson/pull/512/hovercard">#512</a>) (<a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/e9afa9dcfc295da8ff53b28658835fc76cde557c">e9afa9d</a>)</li> <li><strong><a class="issue-link js-issue-link notranslate" rel="noopener noreferrer nofollow" href="https://jira.mongodb.org/browse/NODE-4535">NODE-4535</a>:</strong> automatically promote UUIDs when deserializing and parsing UUIDs (<a href="https://snyk.io/redirect/github/mongodb/js-bson/issues/513" data-hovercard-type="pull_request" data-hovercard-url="/mongodb/js-bson/pull/513/hovercard">#513</a>) (<a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/1dc7eaea6a61924be66ae5b8a05b74d5dd9c7b1e">1dc7eae</a>)</li> </ul> <hr> <h2>Documentation</h2> <ul> <li>API: <a href="https://snyk.io/redirect/github/mongodb/js-bson#readme">https://github.com/mongodb/js-bson#readme</a></li> <li>Changelog: <a href="https://snyk.io/redirect/github/mongodb/js-bson/blob/main/HISTORY.md#change-log">https://github.com/mongodb/js-bson/blob/main/HISTORY.md#change-log</a></li> </ul> <p>We invite you to try the bson library immediately, and report any issues to the <a href="https://jira.mongodb.org/projects/NODE" rel="nofollow">NODE project</a>.</p> </li> </ul> from <a href="https://snyk.io/redirect/github/mongodb/js-bson/releases">bson GitHub release notes</a> </details> </details> <details> <summary><b>Commit messages</b></summary> </br> <details> <summary>Package name: <b>bson</b></summary> <ul> <li><a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/5465c33b356ceaed05c1759007acdf3ab077ee33">5465c33</a> chore(release): 4.7.1</li> <li><a href="https://snyk.io/redirect/github/mongodb/js-bson/commit/d86bd52661e7f5d26479f6b63acac7950f505d69">d86bd52</a> fix(NODE-4905): double precision accuracy in canonical EJSON (#549)</li> </ul> <a href="https://snyk.io/redirect/github/mongodb/js-bson/compare/853bbb0441b0e29e5277cd191b515d5a884d8d21...5465c33b356ceaed05c1759007acdf3ab077ee33">Compare</a> </details> </details> <hr/> **Note:** *You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.* For more information: <img src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiIzOTdmMzA0MS1kMTJmLTQ4MDMtODIyNC1iNDY4MmQ0YzU4NjgiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjM5N2YzMDQxLWQxMmYtNDgwMy04MjI0LWI0NjgyZDRjNTg2OCJ9fQ==" width="0" height="0"/> 🧐 [View latest project report](https://app.snyk.io/org/sandbox-2ba/project/852e6e4f-be96-45c8-b370-1060f5ebee55?utm_source=github&utm_medium=referral&page=upgrade-pr) 🛠 [Adjust upgrade PR settings](https://app.snyk.io/org/sandbox-2ba/project/852e6e4f-be96-45c8-b370-1060f5ebee55/settings/integration?utm_source=github&utm_medium=referral&page=upgrade-pr) 🔕 [Ignore this dependency or unsubscribe from future upgrade PRs](https://app.snyk.io/org/sandbox-2ba/project/852e6e4f-be96-45c8-b370-1060f5ebee55/settings/integration?pkg=bson&utm_source=github&utm_medium=referral&page=upgrade-pr#auto-dep-upgrades) <!--- (snyk:metadata:{"prId":"397f3041-d12f-4803-8224-b4682d4c5868","prPublicId":"397f3041-d12f-4803-8224-b4682d4c5868","dependencies":[{"name":"bson","from":"4.7.0","to":"4.7.1"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/sandbox-2ba/project/852e6e4f-be96-45c8-b370-1060f5ebee55?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"852e6e4f-be96-45c8-b370-1060f5ebee55","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":1,"publishedDate":"2023-01-05T15:16:00.352Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]}) ---> --------- Co-authored-by: snyk-bot <[email protected]>
Pull Request Info
Issue JIRA link:
https://jira.mongodb.org/browse/DOCSP-12591
Docs staging link (requires sign-in on MongoDB Corp SSO):
https://docs-mongodbcom-staging.corp.mongodb.com/realm/docsworker-xlarge/12591/dotnet/authenticate.html