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.

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

Remove Discourse admin API key and user. #1431

Merged
merged 1 commit into from Nov 8, 2019
Merged

Conversation

@Beanow
Copy link
Member

Beanow commented Oct 30, 2019

This removes all usage of and reference to the admin API key and username. Instead relying on anonymous access of the Discourse API.

This enables anyone to deploy an instance with discourse support, and is much safer, since the admin API key isn't used for this purpose anymore. Once merged I would encourage revoking any admin API keys used in the past.

The only notable remaining reference of the discourse username is in the project file.
Which goes from 0.3.0 to 0.3.1 in a backwards-compatible way here, simply ignoring the username if present. For #1426 I'm expecting a 0.4.0 version, so this is to prevent having to change project files twice.

Test plan: updated the snapshots to their latest anonymous versions. Ran yarn test and anonymous discourse loading from CLI numerous times.

@Beanow Beanow requested review from decentralion and wchargin Oct 30, 2019
@Beanow Beanow force-pushed the dev-discourse-anonymous branch from 558b373 to 209b112 Oct 30, 2019
Copy link
Member

decentralion left a comment

Nice work! I think this is a very valuable PR. It's interesting to think about this from a Cred perspective: this isn't the kind of work that other pulls/initiatives will explicitly depend on. It adds value via "via negativa", removing something that would have been a big impediment for users of the system.

Not sure how we can account for this in Cred. Having a "usability" artifact and creating edges towards things that improve usability? (Ideally these artifacts would be composable so we could have discourse ^ usability.)

@@ -42,7 +44,7 @@ export function projectToJSON(p: Project): ProjectJSON {
}

export function projectFromJSON(j: ProjectJSON): Project {
return fromCompat(COMPAT_INFO, j);
return fromCompat(COMPAT_INFO, j, upgrades);

This comment has been minimized.

Copy link
@decentralion

decentralion Oct 30, 2019

Member

Nice! I think this is the first use of the upgrades API. @wchargin and I have gone back and forth a bit on the quality of this API, I'd be curious to hear what you thought of this interface.

@decentralion

This comment has been minimized.

Copy link
Member

decentralion commented Oct 30, 2019

Question: are any of the snapshot updates actually a result of the changes in this PR, or are they just drift on upstream? (The snapshot tests are never consistent from snapshot to snapshot because, among other things, the view count is returned by the server.)

In general I try to separate irrelevant / noisy snapshot updates from the PRs that touch DIscourse, for easier and clearer review. My ideal pattern would be:

  • PR with pre-snapshot update (trivial approve)
  • PR with the code changes, with another snapshot update included (but this one will only reveal differences due to the code change, as opposed to change on the upstream)
@Beanow

This comment has been minimized.

Copy link
Member Author

Beanow commented Oct 30, 2019

@decentralion

Question: are any of the snapshot updates actually a result of the changes in this PR

There are actual changes. Because the requesting user (anonymous) has fewer privileges. It removes some "can do this action: true" style fields, "have you read this yet" and data like user creation dates. None of which we actively used internally, but they're different between using and not using the API key.

@decentralion

This comment has been minimized.

Copy link
Member

decentralion commented Oct 30, 2019

Great. I never wanted that user-specific data to begin with.

@decentralion decentralion merged commit 64834c6 into master Nov 8, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
wchargin added a commit that referenced this pull request Nov 11, 2019
Summary:
Generated with `./scripts/update_snapshots.sh`. This fixes failures
introduced in #1431.

Test Plan:
Running `yarn test --full` now passes. Inspecting the diff shows that
this only includes a compat version number change, which is appropriate.

wchargin-branch: fix-1431-failures
wchargin added a commit that referenced this pull request Nov 11, 2019
Summary:
Generated with `./scripts/update_snapshots.sh`. This fixes failures
introduced in #1431.

Test Plan:
Running `yarn test --full` now passes. Inspecting the diff shows that
this only includes a compat version number change, which is appropriate.

wchargin-branch: fix-1431-failures
@Beanow Beanow deleted the dev-discourse-anonymous branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.